[Bug 709711] New: video decoders - send upstream force_key_unit events when unable to decode

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=709711
  GStreamer | gst-plugins-base | 1.2.0

           Summary: video decoders - send upstream force_key_unit events
                    when unable to decode
    Classification: Platform
           Product: GStreamer
           Version: 1.2.0
        OS/Version: All
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: Normal
         Component: gst-plugins-base
        AssignedTo: gstreamer-bugs at lists.freedesktop.org
        ReportedBy: owilliams.cafex at gmail.com
         QAContact: gstreamer-bugs at lists.freedesktop.org
     GNOME version: ---


Apologies for longish bug report, most of it is a description of a patch that I
could write/submit.  I'm new here and don't want to start writing a patch that
won't be accepted.

+++ What I want:
I want vp8dec and avdec_h264 to send upstream force_key_unit events when they
are unable to decode a frame.  The force_key_unit event will instruct an
upstream element that downstream needs a key frame in order for the decoder to
recover (e.g. from packet loss/corrupt frames).  

The mechanism to detect corrupt frames is codec specific, e.g. VP8  would use 
vpx_codec_control(&dec->decoder, VP8D_GET_FRAME_CORRUPTED, &corrupted).

When decoding problems are detected, each decoder should create a GstEvent
using gst_video_event_new_upstream_force_key_unit and push it on the sink pad.

+++ Proposed patch:
So a basic patch would be to add detection code into handle_frame method of the
two decoders and push the force_key_unit event on the sink pad.

However I don't want spammy force_key_unit events, I want to be able to
configure that a force_key_unit event can happen only once a second.  It makes
sense to put this logic in the base decoder.

I can provide the following patch:
1) add min-force-key-unit-interval property to gstvideodecoder.c.  This
read/write property will get inherited by all videodecoders including vp8dec
and avdec_h264. 
2) add gst_video_decoder_corrupt_frame method to gstvideodecoder.c - this
should be called by a subclass whenever it cannot decode a frame.  It does not
replace the need to call gst_video_decoder_finish_frame or
gst_video_decoder_drop_frame.
3) the base class will keep track of the last time a force_key_unit was sent
and will only send new force_key_unit if: 

if (corrupt_frame->pts >= (last_sent_time + decoder->min_key_unit_interval)) {
   //send upstream force_key_unit
}

4) vp8dec and avdec_h264 are changed to call new
gst_video_decoder_corrupt_frame method each time a frame cannot be decoded.

I'm looking for guidance as to whether this patch will likely be acceptable
before I start writing it.

One possible objection people may have is that by adding the
min-force-key-unit-interval property to the base class, it may look as if other
video decoders support sending force_key_unit events, however this won't be the
case until the necessary detection logic has been added to the decoder.  

Another area of contention is what the default value for
min-force-key-unit-interval should be.  The safest would probably -1, which
means never send force-key-unit events, this would ensure existing pipelines
don't start behaving differently.  

Whilst I'm currently testing on 1.0.7 the patch would likely be for 1.2

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.