After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747574 - videodecoder: reverse playback in non-packetized decoders
videodecoder: reverse playback in non-packetized decoders
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 742922
 
 
Reported: 2015-04-09 17:23 UTC by Víctor Manuel Jáquez Leal
Modified: 2016-06-10 07:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videodecoder: handle buffer's flags at offset (3.61 KB, patch)
2015-04-09 17:23 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
videodecoder: push buffer to adapter if not NULL (1.10 KB, patch)
2015-04-09 17:23 UTC, Víctor Manuel Jáquez Leal
rejected Details | Review
videodecoder: playback rate is in input_segment (1.14 KB, patch)
2015-04-09 17:23 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
videodecoder: squash two message logs into one (1.36 KB, patch)
2015-04-09 17:23 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapidecode Debug messages (1.80 KB, text/plain)
2015-09-04 07:42 UTC, Engin FIRAT
  Details
videodecoder: enable reverse playback with non-packetized (1.76 KB, patch)
2016-05-24 08:39 UTC, Hyunjun Ko
none Details | Review
videodecoder: set SYNC to key frame in reverse playback with non-packetized (1.76 KB, patch)
2016-05-30 16:26 UTC, Hyunjun Ko
none Details | Review
videodecoder: set SYNC to key frame in reverse playback with non-packetized (1.71 KB, patch)
2016-05-30 16:28 UTC, Hyunjun Ko
rejected Details | Review

Description Víctor Manuel Jáquez Leal 2015-04-09 17:23:26 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.
Comment 1 Víctor Manuel Jáquez Leal 2015-04-09 17:23:32 UTC
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.
Comment 2 Víctor Manuel Jáquez Leal 2015-04-09 17:23:42 UTC
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.
Comment 3 Víctor Manuel Jáquez Leal 2015-04-09 17:23:48 UTC
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.
Comment 4 Víctor Manuel Jáquez Leal 2015-04-09 17:23:53 UTC
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.
Comment 5 Víctor Manuel Jáquez Leal 2015-04-10 08:00:50 UTC
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.
Comment 6 Jan Schmidt 2015-04-10 08:07:59 UTC
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.
Comment 7 Víctor Manuel Jáquez Leal 2015-04-10 08:38:07 UTC
(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.
Comment 8 Engin FIRAT 2015-09-03 20:27:37 UTC
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.
Comment 9 Engin FIRAT 2015-09-04 07:42:30 UTC
Created attachment 310637 [details]
vaapidecode Debug messages
Comment 10 Hyunjun Ko 2016-05-24 08:39:13 UTC
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.
Comment 11 Hyunjun Ko 2016-05-30 16:26:07 UTC
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.
Comment 12 Hyunjun Ko 2016-05-30 16:28:42 UTC
Created attachment 328755 [details] [review]
videodecoder: set SYNC to key frame in reverse playback with non-packetized
Comment 13 Andy Devar 2016-06-03 20:32:10 UTC
I wonder whether bug 766514 might be related to this one (747574)?

See:
https://bugzilla.gnome.org/show_bug.cgi?id=766514
Comment 14 Sebastian Dröge (slomo) 2016-06-06 07:37:23 UTC
It's not related to bug 766514
Comment 15 Sebastian Dröge (slomo) 2016-06-09 16:07:43 UTC
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 16 Sebastian Dröge (slomo) 2016-06-09 16:15:08 UTC
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
Comment 17 Víctor Manuel Jáquez Leal 2016-06-10 07:32:20 UTC
(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.