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

[Zebediah Figura (she/her)]

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.

>> 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.