Created attachment 195676 [details] [review]
mpeg2dec: post QoS messages when dropping a frame due to QoS

*** Bug 617020 has been marked as a duplicate of this bug. ***

Cool, thanks for addingn this, but maybe we could move that whole block into a separate function, something along the lines of e.g. gst_jpeg_dec_do_qos() ?

In fact, since it's all very similar between elements, maybe this could be moved to some lib in -base, if there's one that would be convenient for this kind of code.

Created attachment 195689 [details] [review]
theoradec: move the QoS logic to libgstvideo

Created attachment 195690 [details] [review]
libgstvideo: add a new API to handle QoS events and dropping logic

Something like this ?
Moved in libgstvideo with test case using theoradec.
I'll update my patches to other elements to use that when the API's OK'd.

Review of attachment 195690 [details] [review]:

::: gst-libs/gst/video/video.c
@@ +2367,3 @@
+  qt->diff = diff;
+  qt->timestamp = timestamp;
+  qt->earliest_time = qt->timestamp + qt->diff;

timestamp can be GST_CLOCK_TIME_NONE

The formula should be:

   if (diff > 0)
      et = ts + 2 * diff + frame_duration;
   else
      et = ts + diff

::: gst-libs/gst/video/video.h
@@ +444,3 @@
+  guint64 dropped;
+  GMutex *lock; /* protects the above */
+  GstElement *element;

Padding

@@ +445,3 @@
+  GMutex *lock; /* protects the above */
+  GstElement *element;
+} GstQoSTracker;

GstVideoQOSTracker because it's video specific

@@ +585,3 @@
+void           gst_video_qos_reset           (GstQoSTracker * qt);
+void           gst_video_qos_update          (GstQoSTracker * qt, GstEvent* event);
+gboolean       gst_video_qos_process_frame   (GstQoSTracker * qt, GstSegment *segment,

Make the segment const

@@ +587,3 @@
+gboolean       gst_video_qos_process_frame   (GstQoSTracker * qt, GstSegment *segment,
+                                             GstClockTime timestamp, GstClockTime duration);
+void           gst_video_qos_destroy         (GstQoSTracker * qt);

_clear() because it doesn't free the struct. Also name all functions like the struct, e.g. gst_video_qos_tracker_init()

Created attachment 195872 [details] [review]
libgstvideo: add a new API to handle QoS events and dropping logic

Created attachment 195873 [details] [review]
theoradec: move the QoS logic to libgstvideo

Created attachment 195875 [details] [review]
libgstvideo: add a new API to handle QoS events and dropping logic

Use GstClockTime, not GstClockTimeDiff, for frame duration.

Created attachment 195883 [details] [review]
libgstvideo: add a new API to handle QoS events and dropping logic

Created attachment 196015 [details] [review]
libgstvideo: add a new API to handle QoS events and dropping logic

Do not ref the element, that would give us easy ref cycles.
Mention it in the doc blurb.

Created attachment 196020 [details] [review]
deinterlace: switch to the libgstvideo QoS tracking code

Created attachment 196021 [details] [review]
videomixer2: switch to using the libgstvideo QoS tracking code

Created attachment 196022 [details] [review]
mpeg2dec: switch to using the libgstvideo QoS tracking code

Created attachment 196023 [details] [review]
ffmpegdec: switch to the new libgstvideo QoS tracking code

As the interface seems to be OK now, I've ported the elements that were posting QoS messages. Except GstBaseTransform, which does the same but isn't video. Not sure what to do here. Maybe move that QoS stuff to GstVideoFilter, and switch API there, but that would mean audio transforms can't use it unless I leave a copy of the code in GstBaseTransform, so...

Created attachment 196041 [details] [review]
libgstvideo: add a new API to handle QoS events and dropping logic

I'd left a stray GST_ERROR level debug log.

Created attachment 197508 [details] [review]
ffmpegdec: switch to the new libgstvideo QoS tracking code

Created attachment 198339 [details] [review]
jpegdec: switch to the libgstvideo qos code

Created attachment 199418 [details] [review]
timing method

Based on discussions, I'm putting this change here for comments, and if OK I'll integrate it in the patch, and change other elements to pass the appropriate method.

Created attachment 199420 [details] [review]
libgstvideo: add a new API to handle QoS events and dropping logic

Created attachment 199421 [details] [review]
theoradec: move the QoS logic to libgstvideo

Created attachment 199422 [details] [review]
jpegdec: switch to the libgstvideo qos code

Created attachment 199423 [details] [review]
deinterlace: switch to the libgstvideo QoS tracking code

Created attachment 199424 [details] [review]
videomixer2: switch to using the libgstvideo QoS tracking code

Created attachment 199425 [details] [review]
mpeg2dec: switch to using the libgstvideo QoS tracking code

Created attachment 199426 [details] [review]
ffmpegdec: switch to the new libgstvideo QoS tracking code

Created attachment 199447 [details] [review]
mpeg2dec: switch to using the libgstvideo QoS tracking code

whoaaa, a debug rank switch slipped though - fixed.

Created attachment 199459 [details] [review]
jasperdec: port to new QoS API

Untested, I don't have a test file for it.

Created attachment 199460 [details] [review]
basevideocodec: port to new QoS API

Created attachment 199461 [details] [review]
goom: port to new QoS API

Created attachment 199462 [details] [review]
goom2k1: port to new QoS API

Created attachment 199463 [details] [review]
monoscope: port to the new QoS API

Created attachment 199464 [details] [review]
shapewipe: port to new QoS API

Created attachment 199465 [details] [review]
videomixer: port to new QoS API

Created attachment 199466 [details] [review]
libgstvideo: add a new API to handle QoS events and dropping logic

Created attachment 199467 [details] [review]
theoradec: move the QoS logic to libgstvideo

Ok, so:

 - I think *ideally* this should be handled by some relevant
    base class, whether that is GstVideoDecoder, GstVideoFilter
    or whatnot

 - however, I still think it's worth putting this as utility API into
   libgstvideo for now. It doesn't cost us much and it removes
   a considerable amount of code duplication all over the place.
   And it will likely be a while longer before e.g. GstBaseVideoDecoder
   is ready for prime time.

 - I'm a bit undecided about the GstVideoQoSTrackerMethod method
   argument to gst_video_qos_tracker_update(). I know you added it
   because I brought up the different formulas, but ultimately I'm not
   sure whether there's actually a good reason those formulas differ,
   or whether we should just not go with one of the two options (we
   could later still configure a method via the structure by doing
   gst_video_qos_tracker_set_method() or so if we found a good
   reason for the different strategies to exist).

Well, I'll change the patches to whatever's decided in the end, I don't have a particular preference.

As far as I know, there isn't a good reason for the difference in algorithm, and an element gets to use this or that formula depending on where its QoS code was nicked from.

And I will still, for the record, mention that argument about not only decoders doing QoS, so bases classes probably needed to piggy back on a shared implementation anyway.

+1 for having this in the video library and then using it from elements. besides BaseVideoEnc/Dec its also VideoFilter that would benefit from this and many elements right now don't even handle QOS.

Sooo... I'd forgot all about these old patches. What shall we do about them ? If I port them to latest 0.10, are they OK to push, or is there some leftover uncertainty about whether it's best hidden and taken care of by base classes (remembering that all users of QoS do not have a base class) ? I don't really want to go to the trouble of rebasing everything if they're not going up :p

OK, so deemed not very useful on #gstreamer IRC, now that many/all video decoders have been ported to decoder base classes, so closing.