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

[Zebediah Figura]

On 9/21/21 9:36 AM, Francois Gouget wrote:
> On Wed, 15 Sep 2021, Zebediah Figura wrote:
> [...]
>> +static gboolean src_seek_data(GstElement *appsrc, guint64 offset, gpointer user)
>> +{
>> +    struct wg_parser *parser = user;
>>   
>> -    ret = parser->read_request.ret;
>> -    gst_buffer_set_size(*buffer, parser->read_request.size);
>> +    pthread_mutex_lock(&parser->mutex);
>> +
>> +    assert(!parser->read_request.pending);
> 
> quartz:mpegsplit randomly triggers this assertion.
> 
> https://bugs.winehq.org/show_bug.cgi?id=51774
> 
> https://test.winehq.org/data/patterns.html#quartz:mpegsplit
> 
> 

I had this brought to my attention by Gijs and ended up debugging it 
myself. The short version is that appsrc has even more problems than we 
thought. Unfortunately I did not carefully check the flushing code when 
reviewing.

The problem is that a flushing seek can both start and finish between 
wg_parser_get_next_read_offset and wg_parser_push_data. This will cause 
seek-data and need-data to be sent, then the flush will interrupt 
appsrc, after which it will send the same signals again. This isn't a 
problem in itself—we can just remove the assertion as it is in fact 
bogus—except that it means that, once again, we will be sending the 
wrong data.

Note that we can't validate the buffers sent ourself (i.e. do appsrc's 
job for it), because these buffers can be validated and sent before we 
even get seek-data, and yet can still be wrong.

There are two ways I see to fix this while still using appsrc, and both 
of them seem very janky:

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.

2. Send buffers synchronously from within need-data. Honestly I have to 
assume that the developers of appsrc only ever tried this case, even 
though they have documented that you can send data "from a separate 
thread". In this case we'd have to start flushing the read thread before 
and after seeking.

The other options we have are to remove appsrc. I had originally wanted 
to use appsrc for three reasons: first, it would abstract away most of 
the difference between push and pull mode; second, it would deal with 
pad activation and state change logic for us; and third, it would take 
care of all the nonsense surrounding flushing and synchronization so 
that we didn't have to do that. Unfortunately it is quickly turning out 
that the third part doesn't hold, and the first part holds less and less 
as well.

Based on that I see two more options:

3. Build our own element using basesrc. For seekable parsers this does 
still abstract away the difference between push and pull mode. For 
PE-level push mode (e.g. MFTs) it doesn't, and I'm not sure what the 
best move is there. We'd have to do about the same amount of work as 
solution #2, but at least we'd be working within clearly documented 
parameters, plus we actually have the ability to properly deal with 
flushes using the unlock/unlock_stop callbacks. The main downside here 
is that we have to deal with GLib OOP nonsense.

4. Go back to using a custom pad.