Can you provide a testcase for this and give more details about what exactly goes wrong inside libav, like the actual error message that happens?

The file from https://bugzilla.gnome.org/show_bug.cgi?id=702152 has that issue (just try to play it back)

100 % reproducible test case :

gst-launch-1.0 gnlurisource uri=file:///any/file/with/h264 duration=1000000000 ! xvimagesink

What follows is a long list of :

0:00:00.492981052 18979 0x7fea10001aa0 ERROR                  libav :0:: get_buffer() failed (stride changed)
0:00:00.492999754 18979 0x7fea10001aa0 ERROR                  libav :0:: decode_slice_header error
0:00:00.493006290 18979 0x7fea10001aa0 ERROR                  libav :0:: no frame!

I suggest raising the severity of this bug, as it's a regression and other people seem affected.

Oh and by the way my gnonlin patch had some issues, and trying to solve them in gnl would have led to a succession of hacks, that would have been quite a bad idea to merge.

Correct me if I'm wrong, but seeking as soon as we go in PAUSED should not be a problem as far as I can see ?

Sorry for the succession of messages, but would disabling alignment for the while in gstavviddec be acceptable, as it's not very well supported ?

(In reply to comment #4)
> Oh and by the way my gnonlin patch had some issues, and trying to solve them in
> gnl would have led to a succession of hacks, that would have been quite a bad
> idea to merge.
>
> Correct me if I'm wrong, but seeking as soon as we go in PAUSED should not be a
> problem as far as I can see ?

It is, but it's also a very good idea to wait until the caps arrived in the srcpads that you're blocking, just so you know already what is going to come out of this pad and have the negotiation finished already. What exactly is the problem in gnonlin? What difference does it make to block on stream-start vs. segment?


(In reply to comment #5)
> Sorry for the succession of messages, but would disabling alignment for the
> while in gstavviddec be acceptable, as it's not very well supported ?

I guess so, it would mean that we won't do real zerocopy with libav most of the time... but slower is probably better than stuff crashing or failing in interesting ways. Edward?

Hm quite difficult to explain, I guess I'm better off first explaining the sequence of events that leads to the bug.

1) on PAUSED, gnl_source_prepare is called, a its src pad is blocked with GST_PAD_PROBE_BLOCK_DOWNSTREAM and an initial seek created but not sent yet.

2) when the first buffer is asked for to the decoder in alloc_output_buffer, as it didn't do its negotiation yet it calls gst_video_decoder_negotiate.

3) negotiate calls set_caps, there are stickied events on the pad (stream-start being the first)

4) the event is received in the source's probe callback, which triggers the creation of a thread to send the stored seek event, which executes nearly immediately.

5) the pad is now flushing, and the bufferpool negotiation which was to happen next in gst_video_decoder_negotiate_default fails.

6) the process goes on in gst-libav, eventually leading to the field "linesize" in a context implemented by libav to be set to certain values.

7) the bufferpool is finally negotiated, an alignment is set but for some reason that context is not updated.

8) the allocation of new frames in libav starts failing with the messages I mentioned.

I've found it quite challenging to understand how that context, which is private to libav, is supposed to be updated. I'm certain Edward will know better than I.

OK so that's the definition of the problem, now to the problem with my patch.

I can't wait for a specific event in that probe callback as it's blocking, and if I ignore the stream-start well I don't get anything next.

Now if I decide to set the probe callback to not blocking, ie let the events flow until I get caps, my problem is now that I'm gonna end up getting a segment from the source with a stop at -1, I guess because as it has not been seeked the driving element must be deciding to send a "default" segment. If I drop this segment I still have a problem, because I get data flow without segment.

I guess I could workaround all that but as I said I think it would be a pile of hacks, not sure we want to do that.

May'be there's a better solution but I fail to see it, I also don't really want to break something in the process.

I've spent a lot of time on this issue, and I'm a little disappointed not to have found the correct fix in libav, but well I suppose it has to happen sometimes :)

So I also am gonna end up by :

Edward ?

The gnonlin bug should be discussed in a different bug to prevent turning this one here in an unreadable mess that has nothing to do with the original bug anymore :)

Problem here is that renegotiation does not properly work with libav. That's independent of what gnonlin does.

See http://bugzilla.libav.org/show_bug.cgi?id=556

Created attachment 252942 [details] [review]
Only negotiate alignment once

This patch ensures we only negotiate alignment once. I think it is simple enough to be acceptable as a short-term solution, and I'll be happy to see it reverted once we get the proper fix merged upstream.

Sorry for the noise, this patch is incorrect, it should be if (!ffmpegdec->alignment_is_set , but in that case I still get a (unique) warning that stride changed :(

> gst-launch-1.0 playbin uri=http://rdmedia.bbc.co.uk/dash/ondemand/bbb/avc3/1/client_manifest-common_init.mpd

This reproduces it too, outside GES. But there are also other problems around that it seems, see bug #707449.

Created attachment 254082 [details] [review]
Patch to correct the high level issue, correct patch will be to fix libav.

Review of attachment 254082 [details] [review]:

::: ext/libav/gstavviddec.c
@@ +259,3 @@
   ffmpegdec->debug_mv = DEFAULT_DEBUG_MV;
   ffmpegdec->max_threads = DEFAULT_MAX_THREADS;
+  ffmpegdec->decided_allocation = FALSE;

This will make the element not reusable. At least in GstVideoDecoder::start this should be set to FALSE too, or maybe even in flush

@@ +1531,3 @@
   /* we can only enable the alignment if downstream supports the
    * videometa api */
+  if (!ffmpegdec->decided_allocation && have_alignment && have_videometa) {

Note that this will cause gst-libav to not work zerocopy anymore. Maybe it would be better to just keep the old allocation configuration? It is guaranteed that it will still work and maybe it is more optimal.

This issue seems to have broken Aurena use case too. After the synchronisation seel, I always get:
libav :0:: get_buffer() failed (stride changed)

commit 1277540b8c060983d518109237586ea12446d479
Author: Sebastian Dröge <slomo@circular-chaos.org>
Date:   Thu Sep 12 12:12:23 2013 +0200

    avviddec: Use the correct context for negotiation
    
    During get_buffer() it can happen that the main context is not
    updated yet and only the context passed here has the correct
    width, height, etc.

commit faec0142e34c3d514a62132f141622676499fa14
Author: Sebastian Dröge <slomo@circular-chaos.org>
Date:   Thu Sep 12 12:11:29 2013 +0200

    avviddec: Remember initially used stride and don't allow stride changes
    
    libav does not allow stride changes currently, fall back to
    non-direct rendering here:
    https://bugzilla.gnome.org/show_bug.cgi?id=704769
    https://bugzilla.libav.org/show_bug.cgi?id=556