On 9/2/21 11:06 AM, Derek Lesho wrote:
So I've thought about this patch some more, and since the goal here is to make adding push-mode support more streamlined, I'm thinking that a bigger rework than this would be preferred. In the current patch, we simply merge _create and _connect, having the read thread spin until the parser is ready (just like previously it spinned until sink_connect was true). However, in push mode, we need to return from the creation function and move on to ::ProcessInput and ::ProcessOutput, so maybe it would be a better idea to have _create() handle everything to gst_element_set_state(pipeline, PAUSED), and restructure other functions (i.e. get_stream_count, get_stream, get_stream_duration) to block on the corresponding step of initialization.
This would keep the pull mode functions fundamentally the same, since they query for all this data right after the parser is initialized, but allow push-mode transforms to yield control to the consumer of the transform when gstreamer wants data.
I'm going to start work on this now, but if there are any better ideas, I'd love to hear them.
Hmm...
So I originally advocated for merging create/connect and disconnect/destroy. Unfortunately I then realized that you can't just do this.
On the create side, we need to block until all the GStreamer elements are done initializing, which means we of course need data. That's been pretty well described.
On the destroy side, we really need to terminate the read thread before we destroy the wg_parser object. There are multiple ways to do this, but ultimately I suspect keeping connect/disconnect around is the best one. I haven't looked in detail at patch 4/5 but I suspect it's either racy, or ends up being too complex. Sorry about that.
With regard to push mode, we don't have the race at destruction, which is nice. We do have weirdness at creation, though, as you describe.
I think what you've come up with is probably the only sensible solution, although it's also not clear to me what mfplat's actual requirements are.
I also think we want it to end up being *only* get_stream_count that suffers from this (which is honestly a bit weird given its name, but meh, whatever). We can't even construct stream objects until we're fully initialized.