Created attachment 362244 [details] [review]
videoencoder: add qos property

This new property control if the encoder base class should gather QoS
stats and if subclasses should use them by dropping late frames.

Review of attachment 362244 [details] [review]:

Looks good to me apart from that :)

::: gst-libs/gst/video/gstvideoencoder.c
@@ +174,2 @@
   /* QoS properties */
+  gint qos_enabled;             /* ATOMIC */

Hm, why not just protect with the object lock as well? It's taken right next to where you check the variable already, just need to protect  the getters / setters

I implemented it as in gstbasesink. I don't mind changing if it's considered as a better pattern.

(In reply to Guillaume Desmottes from comment #3)
> I implemented it as in gstbasesink. I don't mind changing if it's considered
> as a better pattern.

I'd say for consistency's sake it is a better one here at least :)

It requires the subclass to release the object lock before checking if qos is enabled, maybe that is the reason why it is done that way in base sink though ?

Review of attachment 362244 [details] [review]:

The atomic approach has the advantage of not requiring the subclass to release the object lock when checking, accepted

Attachment 362244 [details] pushed as 7950a46 - videoencoder: add qos property

"gst_video_encoder_is_qos_enabled" should be called "gst_video_encoder_get_qos_enabled" for proper naming of a gobject-property-accessor

I named it as 'gst_base_sink_is_qos_enabled'. Do we want to always enforce the get/set name pattern?