[Bug 784599] kmssink: support videooverlay interface

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=784599

Víctor Manuel Jáquez Leal <vjaquez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #355313|none                        |needs-work
             status|                            |

--- Comment #16 from Víctor Manuel Jáquez Leal <vjaquez at igalia.com> ---
Review of attachment 355313:
 --> (https://bugzilla.gnome.org/review?bug=784599&attachment=355313)

::: sys/kms/gstkmssink.c
@@ +728,3 @@
+  GST_OBJECT_UNLOCK (self);
+
+  GST_DEBUG_OBJECT (self, "allowed caps %" GST_PTR_FORMAT, caps);

this debug message can be removed. I would remove both (the next added one too)
because they can be known with other logs, but have one is OK.

@@ +1250,3 @@

+static void
+gst_kms_sink_set_render_rectangle (GstVideoOverlay * overlay,

please move these three function up, as the first functions of the file, so we
could identify quickly as the block related with the overlay interface.

@@ +1253,3 @@
+    gint x, gint y, gint width, gint height)
+{
+  GstKMSSink *kmssink = GST_KMS_SINK (overlay);

please use 'self' instead of 'kmssink' to keep the consistence among the code.

@@ +1255,3 @@
+  GstKMSSink *kmssink = GST_KMS_SINK (overlay);
+
+  if (width > 0 && height > 0) {

I would use here a quick return idiomatic:

if (width == 0 || height == 0)
  return

thus we can save some indentation ;)

@@ +1291,3 @@
+gst_kms_sink_expose (GstVideoOverlay * overlay)
+{
+  GstKMSSink *kmssink = GST_KMS_SINK (overlay);

self instead of kmssink

@@ +1336,3 @@
   if (!buffer)
     return GST_FLOW_ERROR;
+

I guess there's no reason to add this space.

@@ +1343,3 @@
   GST_TRACE_OBJECT (self, "displaying fb %d", fb_id);

+  GST_OBJECT_LOCK (self);

I'm a bit worried about this long lock. Is it truly required? Just wondering...

@@ +1384,3 @@
+  src.h =
+      (result.y + result.h) >
+      self->vdisplay ? self->vdisplay - result.y : src.h;

these conditional operators are hard to read when they are not one-liners. I
would change them to normal if ()

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.