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 767232 - videodecoder: Drain data in more situations
videodecoder: Drain data in more situations
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: 767641
 
 
Reported: 2016-06-04 07:54 UTC by Edward Hervey
Modified: 2018-07-09 13:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videodecoder: Drain decoder on DISCONT buffers (1.20 KB, patch)
2016-06-04 07:54 UTC, Edward Hervey
committed Details | Review
videodecoder: Drain out keyframes in TRICK_MODE_KEY_UNITS (1.60 KB, patch)
2016-06-04 07:54 UTC, Edward Hervey
committed Details | Review

Description Edward Hervey 2016-06-04 07:54:01 UTC
While investigating trick mode with key units, there were two situations that
would introduce big delays in getting the decoded keyframe (which should be
decoded and pushed out straight away).
Comment 1 Edward Hervey 2016-06-04 07:54:06 UTC
Created attachment 329108 [details] [review]
videodecoder: Drain decoder on DISCONT buffers

This ensures the decoder is properly drained out when receiving a
DISCONT buffer. The optimal way of doing this would have been to
receive a GAP event before hand but it is not always possible.

Fixes big delays with some decoders (ex gst-libav) that will not
drain out data when only decoding keyframes.
Comment 2 Edward Hervey 2016-06-04 07:54:12 UTC
Created attachment 329109 [details] [review]
videodecoder: Drain out keyframes in TRICK_MODE_KEY_UNITS

When asked to just decoder keyframe, if we got a keyframe drain out
the decoder straight away.
This avoids having to wait for the next frame and reduces delay even
more.
Comment 3 Sebastian Dröge (slomo) 2016-06-06 08:02:42 UTC
Comment on attachment 329109 [details] [review]
videodecoder: Drain out keyframes in TRICK_MODE_KEY_UNITS

Typo "to just decoder keyframe" -> decode. Otherwise good :)
Comment 4 Edward Hervey 2016-06-07 07:52:22 UTC
commit 183e94b2d3ee830f196d45756563f4572c9954f7
Author: Edward Hervey <edward@centricular.com>
Date:   Sat Jun 4 09:51:17 2016 +0200

    videodecoder: Drain out keyframes in TRICK_MODE_KEY_UNITS
    
    When asked to just decode keyframe, if we got a keyframe drain out
    the decoder straight away.
    This avoids having to wait for the next frame and reduces delay even
    more.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767232

commit eb1ebf226ff8c29ea976169f81135219f1ab412b
Author: Edward Hervey <edward@centricular.com>
Date:   Sat Jun 4 09:49:00 2016 +0200

    videodecoder: Drain decoder on DISCONT buffers
    
    This ensures the decoder is properly drained out when receiving a
    DISCONT buffer. The optimal way of doing this would have been to
    receive a GAP event before hand but it is not always possible.
    
    Fixes big delays with some decoders (ex gst-libav) that will not
    drain out data when only decoding keyframes.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767232
Comment 5 Philippe Normand 2016-06-14 08:46:10 UTC
Since this commit I observe a bunch of GST_FIXMEs spitting out:

gstvideodecoder.c:1057:gst_video_decoder_drain_out:<omxh264dec-omxh264dec0> Sub-class should implement drain()

The discont flag is set by baseparse.

My use-case is the following, an appsrc pushing H264 byte-stream in playbin on RPi2.
Comment 6 Tim-Philipp Müller 2016-06-14 08:56:19 UTC
Sounds like you should open/clone a bug against gst-omx then, no? :)

Or are you saying the log level should be downgraded?
Comment 7 Philippe Normand 2016-06-14 09:10:21 UTC
I'm just wondering why I get this message for every buffer handled by the decoder.

Wouldn't draining the OMX decoder for every buffer kill the performance?

Well perhaps this is actually a baseparse issue, sorry for the noise here.
Comment 8 Tim-Philipp Müller 2016-06-14 09:19:25 UTC
I don't know, it depends on the kind of buffer - is this I-frame only trick play / reverse playback, or does it happen for every single frame in normal playback or .. ?

I don't think the DISCONT flag should be set on every frame/buffer in normal playback, maybe clone this bug into a new one so we can investigate?
Comment 9 Nicolas Dufresne (ndufresne) 2016-06-14 13:54:54 UTC
Wasn't it a little bold to add drain() virtual, and not implement it outside of avedec ? Specially that we now seem to use it everywhere. The other side effect, is that not all decoder let you drain without removing the decoder state, which means for GAPS it's pretty hard to do.
Comment 10 Sebastian Dröge (slomo) 2016-06-17 08:03:47 UTC
If that is a problem in practice, we could add something to the base class to let the subclass specify how expensive drains are. And if they are too expensive, they would only be used when really needed (EOS, caps change).
Comment 11 Nicolas Dufresne (ndufresne) 2018-07-09 13:34:13 UTC
(In reply to Edward Hervey from comment #1)
> Created attachment 329108 [details] [review] [review]
> videodecoder: Drain decoder on DISCONT buffers
> 
> This ensures the decoder is properly drained out when receiving a
> DISCONT buffer. The optimal way of doing this would have been to
> receive a GAP event before hand but it is not always possible.
> 
> Fixes big delays with some decoders (ex gst-libav) that will not
> drain out data when only decoding keyframes.

We observe that this change increase the amount of distortion on packet lost. Draining imply flushing the DPB (the references), so instead of having the following frame missing one reference, it's missing all references. This leads to gray background instead of couple of blocky spots.