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