GNOME Bugzilla – Bug 747574
videodecoder: reverse playback in non-packetized decoders
Last modified: 2016-06-10 07:40:40 UTC
While trying to fix bug 742922, I found that the gstvideodecoder doesn't handle the frame's SYNC_POINT in non packetized decoder, such as vaapidecode, among other one-liner issues.
Created attachment 301227 [details] [review] videodecoder: handle buffer's flags at offset For reverse playback it is important to handle correctly the frame sync points, which is set when the input buffer doesn't have the DELTA_UNIT flag. This is handled correctly when decoder is packetized, but when it is not the frame's sync point is not copied, and the reverse playback never decodes frame batches. The current patch adds the buffer's flags to the Timestamp list, where the timestamp and duration of the input buffers are hold.
Created attachment 301228 [details] [review] videodecoder: push buffer to adapter if not NULL In gst_video_decoder_add_to_frame(), the function gst_adapter_take_buffer() is used for the input adapter, but it can return NULL. This patch checks if the returned buffer is not NULL in order to push it into the output adapter.
Created attachment 301229 [details] [review] videodecoder: playback rate is in input_segment The playback rate is hold in the input_segment member variable, not in the output_segment, and the parse_gather list was never filled because of that. This patch changes the comparison with input_segment.
Created attachment 301230 [details] [review] videodecoder: squash two message logs into one There were two consecutive log messages in gst_video_decoder_decode_frame(). Given the information they provide, it is more efficient to squash them into a single one.
Since it is related, I'm' copying the comment in https://bugzilla.gnome.org/show_bug.cgi?id=742922#c21 With the patches in bug 747574, I could make the reverse playback to work, but its logic is not friendly with decoders that have a limited number of output buffers (in our case surfaces): The reverse playback in GstVideoDecoder, if I understand it correctly, gathers a bunch of frames, traverses the list backwards, decoding each frame; finally pushes the whole list to the next element. The problem is that the list of frames to decoder is bigger that our number of available surfaces. Hence, there's a moment where the decoder run out of surfaces leading to a blocking condition, but the reversed frame list is not decoded completely. Race condition. We could change the logic of reverse playback in GstVideoDecoder, pushing every frame after its decoding, but I don't know if that is correct.
No, you've misunderstood. Videodecoder gathers an entire GOP, decodes it *forwards* (as it has to, it can't decode in reverse in general). When it reaches the end of the GOP, it pushes each decoded frame out in reverse order. When there are B-frames, we could do better, since with some decoder implementations B-frames could be decoded 'just in time' using existing reference frames and decoding only the B-frames in reverse and outputting them immediately - but it's still possible we'd run out of surfaces. In short, we need a mechanism to cope with surface exhaustion deadlock, one way or another.
(In reply to Jan Schmidt from comment #6) > No, you've misunderstood. Videodecoder gathers an entire GOP, decodes it > *forwards* (as it has to, it can't decode in reverse in general). When it > reaches the end of the GOP, it pushes each decoded frame out in reverse > order. Oh! Thanks for the clarification! > > When there are B-frames, we could do better, since with some decoder > implementations B-frames could be decoded 'just in time' using existing > reference frames and decoding only the B-frames in reverse and outputting > them immediately - but it's still possible we'd run out of surfaces. > > In short, we need a mechanism to cope with surface exhaustion deadlock, one > way or another. Indeed.
I have applied the patches 227 to 230 to gstreamer-plugin-base version 1.4.3. Moreover I have realized that, the patches 299086, 299087 and 299088 in <a href="">Bug 742922</a> are pushed in gstreamer-vaapi version 0.6.0. But the reverse playback is still not working. What should I do? One can find the output of element (GST_DEBUG=vaapidecode:7) when a -1.0 seek_event is sent to pipeline. Regards.
Created attachment 310637 [details] vaapidecode Debug messages
Created attachment 328419 [details] [review] videodecoder: enable reverse playback with non-packetized See commit msg. Passing only key frame looks tricky, but I think this or something like this is necessary to enable reverse playback for some specific decoders.
Created attachment 328754 [details] [review] videodecoder: set SYNC to key frame in reverse playback with non-packetized see commit message. I believe this is necessary to make reverse playback working with non-packetized decoder.
Created attachment 328755 [details] [review] videodecoder: set SYNC to key frame in reverse playback with non-packetized
I wonder whether bug 766514 might be related to this one (747574)? See: https://bugzilla.gnome.org/show_bug.cgi?id=766514
It's not related to bug 766514
Comment on attachment 301228 [details] [review] videodecoder: push buffer to adapter if not NULL Why does this happen? Wouldn't this mean that the wrong number of bytes was passed to add_to_frame(), i.e. more bytes than are actually available? Seems like a bug in the caller?
Comment on attachment 328755 [details] [review] videodecoder: set SYNC to key frame in reverse playback with non-packetized This is basically the same as https://bugzilla.gnome.org/attachment.cgi?id=301227 (which solves it a bit nicer) and https://bugzilla.gnome.org/attachment.cgi?id=301229
(In reply to Sebastian Dröge (slomo) from comment #15) > Comment on attachment 301228 [details] [review] [review] > videodecoder: push buffer to adapter if not NULL > > Why does this happen? Wouldn't this mean that the wrong number of bytes was > passed to add_to_frame(), i.e. more bytes than are actually available? Seems > like a bug in the caller? Agreed. I don't remember when this was happening. Let's close this issue then, and open another bug if the issue reappears.