[PATCH v6 5/5] winegstreamer: Replace source pad interface with GstAppSrc.

[Derek Lesho]

On 9/29/21 17:30, Zebediah Figura (she/her) wrote:
> On 9/29/21 02:59, Derek Lesho wrote:
>> On 9/29/21 00:18, Zebediah Figura (she/her) wrote:
>>
>>> On 9/28/21 03:08, Derek Lesho wrote:
>>>>
>>>> On 9/28/21 07:23, Zebediah Figura (she/her) wrote:
>>>>> On 9/24/21 14:23, Derek Lesho wrote:
>>>>>>
>>>>>> On 9/24/21 12:38, Zebediah Figura wrote:
>>>>>>> On 9/24/21 3:05 AM, Derek Lesho wrote:
>>>>>>>> On 9/21/21 13:02, Zebediah Figura wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 1. Rely on the fact that seek-data is sent during a seek, and 
>>>>>>>>> use it
>>>>>>>>> to effectively invalidate any buffers sent before the seek. 
>>>>>>>>> Unlike the
>>>>>>>>> aforementioned problem with validation, this will actually 
>>>>>>>>> work: at
>>>>>>>>> the time that seek-data is sent appsrc should be flushing, so any
>>>>>>>>> buffers we send before it will be discarded by appsrc, and any 
>>>>>>>>> buffers
>>>>>>>>> we send afterward can be discarded by us if they aren't valid. 
>>>>>>>>> This is
>>>>>>>>> very fragile, though. There's really no reason for appsrc to send
>>>>>>>>> seek-data for random-access streams in the first place, and 
>>>>>>>>> this kind
>>>>>>>>> of synchronization is easy to get wrong if I haven't already.
>>>>>>>>
>>>>>>>> This sounds like the best way forward in my opinion, how is it 
>>>>>>>> fragile?
>>>>>>>
>>>>>>> It's fragile in general because we're making a lot of 
>>>>>>> assumptions, that aren't documented, about when and from which 
>>>>>>> thread gstappsrc will send these signals, and what 
>>>>>>> synchronization guarantees it applies when doing so.
>>>>>>
>>>>>> My perspective is that if we can get it working, documenting the 
>>>>>> two required quirks to the GStreamer project, eventually the 
>>>>>> problem will be be cleared up and/or fixed, and we can remove the 
>>>>>> quirks then.
>>>>>
>>>>> The problem is that we're still going to have to keep those 
>>>>> workarounds for a long time. Not to mention that clearly filing a 
>>>>> bug is not enough to get the attention of the GStreamer developers.
>>>>>
>>>>> As the maintainer of this code, I don't think I feel very 
>>>>> comfortable relying this much on the internals of appsrc, whether 
>>>>> as workarounds or not.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> It seems that if we just ensure that a pushed buffer takes into 
>>>>>>>> account
>>>>>>>> the latest seek/need data pair, not much can go wrong.
>>>>>>>>
>>>>>>>
>>>>>>> That's not enough by itself. We can, generally speaking, send a 
>>>>>>> buffer after a seek-data/need-data is triggered but before it is 
>>>>>>> actually sent. 
>>>>>> Before what is sent?
>>>>>
>>>>> Before the seek-data or need-data signal is sent. There is a 
>>>>> window there, however short.
>>>>
>>>> I'm not sure exactly what the difference between triggering a 
>>>> signal and sending a signal is, but assuming you mean that if 
>>>> seek-data doesn't acquire the mutex, it would be possible to 
>>>> push-buffer to not see the seek, then send the buffer after 
>>>> seek-data, then yeah, we do need to put seek-data and push-buffer 
>>>> in the same mutex to make sure that push-buffer is responding to 
>>>> the latest seek/need data callbacks.
>>>>
>>>>>
>>>>>>> This happens in practice with flushes.
>>>>>>>
>>>>>>> In order to get around that, you need the hack I mentioned: 
>>>>>>> assume that a flush will be accompanied by a seek-data signal. 
>>>>>>> This is especially fragile because there's no reason for appsrc 
>>>>>>> to even be sending this signal in the first place (it only 
>>>>>>> really makes sense in "seekable" mode), and they could easily 
>>>>>>> decide to not do that.
>>>>>>
>>>>>> I'm not sure I understand, whenever the next buffer app source 
>>>>>> wants is not consecutively after the last, seek-data is required 
>>>>>> (and sent). What does "assume the flush will be accompanied by a 
>>>>>> seek-data signal" mean? The app source client doesn't have any 
>>>>>> conception of a flush, just seek-data and need-data, and to fix 
>>>>>> this problem all we need to do is add a quite rational check that 
>>>>>> push_data is responding to the latest seek/need data pair.
>>>>>
>>>>> Let me try to explain more clearly. At least one race, the one 
>>>>> I've been trying to describe, looks like this:
>>>>>
>>>>>
>>>>> read thread         main thread
>>>>> -------------------------------------
>>>>>                     push seek event
>>>>> retrieve data
>>>>> validate offset
>>>>>                     send flush-start
>>>>>                     emit seek-data
>>>>>                     send flush-stop
>>>>> push-buffer
>>>>>
>>>>>
>>>>> At which point appsrc will get the wrong buffer.
>>>>>
>>>>> Solution 1 has us rely on the seek-data signal to effectively wait 
>>>>> for the read thread (most likely using the parser mutex) and, in a 
>>>>> sense, put a barrier between reads occuring before and after the 
>>>>> flush.
>>>>
>>>>
>>>> I mean, yeah, but from the perspective of the interface, all we are 
>>>> doing is making sure that buffers that are responding to requests 
>>>> from before a seek-data don't get sent, which I really don't think 
>>>> should be too controversial.  That the seek happens to be caused by 
>>>> a flush shouldn't matter, we're putting a barrier between buffers 
>>>> sent for a different offset, and the current offset we've gotten 
>>>> from seek-data. Honestly, I'm not even sure this is a GStreamer 
>>>> bug, should they really have to handle a case where the client 
>>>> responds with an incorrect buffer after a seek?
>>>
>>> I don't think this is obvious at all. It's certainly not called out 
>>> in the documentation.
>> The documentation does say "After receiving the seek-data signal, the 
>> application should push-buffers from the new position."
>
> There's quite a difference between that and "make sure that any old 
> buffers are flushed out,
Old buffers don't need to be flushed out, app source does that.
> and don't send any new buffers from the old position".
"don't send any new buffers from the old position" logically has the 
same meaning as "should push-buffers from the new position".
>
>>> And, moreover, it's a lot of work, and it doesn't even look like a 
>>> remotely idiomatic solution.
>> As I've already said, making sure we don't push buffers to an old 
>> offset after receiving a seek seems quite idiomatic to me.
>>> Not to mention that, as I've said, appsrc really has no reason to 
>>> send seek-data at all here.
>> It sends it here to ensure that pre-flush buffers are discarded. 
>> After it calls seek-data, it clears the queue, because of the 
>> aforementioned expectation, namely that the app will respond to 
>> seek-data, this works.
>
> Well, no, I don't think it does. I think it sends seek-data here for 
> "seekable" mode, because that actually makes sense, and ends up also 
> sending it for "random-access" mode unnecessarily.
Sending it for "random-access" mode does makes sense, because downstream 
wants all future(post flush) buffers to be from a new offset, so it 
needs to tell the app to stop sending buffers from the old location, and 
start from the new location, then discard the queued buffer/s for the 
old location if needed.  As mentioned above this, according to the 
documentation, when a seek-data is sent, buffers sent afterwards must be 
responsive to the new offset.
>
>>>
>>> I'm pretty sure that you're correct that putting the whole of 
>>> wg_parser_push_data inside a mutex, and the seek-data callback 
>>> inside the same mutex, is actually sufficient to fix this race. But 
>>> that stops being true if appsrc stops sending seek-data here.
>> What reason would appsrc have to stop sending seek-data here, it has 
>> to as far as I can see.
>>>
>>> Fundamentally the API contract here is that you don't send old 
>>> buffers after a flush stops. DirectShow and GStreamer are both built 
>>> on that idea. I believe that Media Foundation does away with it, 
>>> which is one of the good things about Media Foundation. appsrc 
>>> doesn't expose flushes to us, and it doesn't seem to do a good 
>>> enough job of taking care of them itself.
>> I think that requiring us not to send old buffers after seek-data and 
>> sending seek-data when they flush, then discarding the rest of the 
>> queue is a pretty good way of taking care of flushes.
>>> Trying to infer when they happen is too fragile for my taste.
>> We don't need to infer when they happen, all we need to do is follow 
>> the guideline not to send buffers to a previous offset after seek-data.