[RFC PATCH 5/5] winegstreamer: Merge parser creation functions and wg_parser_connect.

[Zebediah Figura]

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.