[PATCH 2/3] winegstreamer: Use decodebin to initialize media streams.

[Zebediah Figura]

On 9/15/20 8:47 AM, Derek Lesho wrote:
> 
> On 9/14/20 4:21 PM, Zebediah Figura wrote:
>>
>> On 9/11/20 12:23 PM, Derek Lesho wrote:
>>> On 9/11/20 11:24 AM, Zebediah Figura wrote:
>>>> On 9/11/20 10:18 AM, Derek Lesho wrote:
>>>>> On 9/10/20 6:43 PM, Zebediah Figura wrote:
>>>>>
>>>>>> I got the impression from some past communication that the reason for
>>>>>> using appsink in the first place (instead of just a solitary sink pad)
>>>>>> is so that we can buffer, or is there some other reason?
>>>>> Well, buffering is necessary, as media streams operate in a sort of
>>>>> pull/push mode where we can only send them samples if they have
>>>>> requested one with IMFMediaStream::RequestSample.
>>>> Sure, but it also seems potentially simple enough to just wait on a
>>>> semaphore in the chain function.
>>> It's definitely more complicated for a system like that (I used to have
>>> it that way).  With appsink, we just insert the request sample call into
>>> a command queue, and pull the buffer from the appsink in the async
>>> command.  The command queue is completely synchronous so we don't have
>>> to worry about stuff like seeking or stopping interfering.  With the
>>> semaphore solution, I'm not sure how much we have to worry about those
>>> cases, but I remember having all sorts of strange bugs.
>> Could you please describe in more detail the problems you encountered?
> To be honest, I don't remember them too clearly, but back then I had a 
> system where the appsink was accessed by both RequestSample and the 
> new-sample callback. When there were outstanding requests, the 
> new-sample callback would dispatch MEMediaSample, and when there were 
> buffered samples, RequestSample would pop one and dispatch 
> MEMediaSample.  The problems occurred around the state of pending 
> samples being invalidated upon actions performed on the gstreamer 
> pipeline.  Of course, if we redid it now, I'm sure we could avoid this, 
> but it just seems like it would be more tricky.
>> To be sure you'd need an extra event in there to handle flushes, but
>> that doesn't seem like a deal-breaker to me either.
> Flushing is what caused me the most trouble earlier on, yeah.
>>
>>>>    If it makes sense (for latency reasons)
>>>> to buffer more than that, then there's a potential argument for appsink
>>>> (but also, perhaps, just an argument for appending a queue element after
>>>> each stream). I don't know/remember the relevant details for Media
>>>> Foundation timing.
>>>>
>>>>> Also, appsink is just
>>>>> more convenient than manually setting up a pad, I've also considered
>>>>> changing the source pad to appsrc, but if it isn't broken....
>>>>>
>>>> Is it, though? I don't know what all the things you need to hook up are,
>>>> but it looks to me like you essentially have to set up the same
>>>> callbacks
>>> We don't set up any callbacks for appsink, just pull a buffer when we
>>> need one.
>> Well, you do, actually, to get current caps (and, eventually, for caps
>> negotiation). appsink does save you a chain function, but I don't
>> immediately see anything else that it saves you.
> https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/winegstreamer/gstdemux.c#l2200
> 
> Seems like it saves us from needing an event and query function as well, 
> going off the quartz equivalent.  

Not really, since you need to add a probe function instead. Note that
you also don't need to handle most of the events that quartz does.

> And it looks to me like there's a 
> significant enough amount of boilerplate in them for it to be undesirable.

I'm not sure what in those functions qualifies as boilerplate?

>>
>>> And given how much boilerplate and how many naming problems
>>> callbacks cause in winegstreamer, it's probably better this way.
>> Wiring up GStreamer callbacks is annoying, yes, but that annoyance may
>> be worthwhile. In particular adding another callback doesn't actually
>> increase code complexity; it's just copying an existing pattern.
>>
>> winegstreamer has some bad names, as I've complained in this thread, but
>> that's a preëxisting problem that wouldn't be exacerbated by adding a
>> chain function.
>>
>>> https://github.com/Guy1524/wine/commit/366946876d7a53f80d91749f290f511294415cf5#diff-2873f4b2d2de6a23dbc67b2960e28f88
>>>
>>>> , just in a different form. In the case of caps it makes things
>>>> more than a little uglier
>>> What does, appsink or a custom pad?  With appsink all you need to do is
>>> set the desired caps property.
>>>> , especially if you later end up doing more
>>>> complicated caps negotiation. Note also that GstPad has default handling
>>>> of EOS events and GST_PAD_IS_EOS(), which I'm inclined to understand is
>>>> more useful for MF than handling GST_EVENT_EOS directly.
>>> I'm not sure what you're referring to here.  All we need to do in terms
>>> of end of stream is read the "eos" property on the appsink during the
>>> RequestSample-derived command, and dispatch the relevant events.
>> Yes, my point is that it's not really any different for GstPad; quartz
>> waits for GST_EVENT_EOS but that's only because it matches quartz's
>> threading model much better.
>>
>>
>> To be quite clear, I'm not trying to argue that appsink shouldn't be
>> used here; I'm just trying to understand all the potential factors
>> behind the decision, and essentially stress-test it. So far as I see
>> appsink provides a built-in buffer that matches mfplat's model nicely,
>> which is good, but it also forces the use of probes and thereby some
>> tricky parameter passing, which is not good.
> Well, the probe isn't strictly necessary, another option is to wait on 
> the paused state, then read the caps from the pads, but the pad probe 
> way would be useful in a case where we want to enumerate all the fixed 
> types to expose on the stream's media type handler.

Wait for what, exactly?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200915/47a0a9b8/attachment.sig>