[Bug 765798] vaapisink: add support for GST_TAG_IMAGE_ORIENTATION

[GStreamer (GNOME Bugzilla)]

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

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

::: gst/vaapi/gstvaapisink.c
@@ +1654,3 @@
+        } else if (!g_strcmp0 ("rotate-270", orientation)) {
+          sink->rotation_req = (sink->rotation + GST_VAAPI_ROTATION_270) %
360;
+        } else if (!g_strcmp0 ("flip-rotate-0", orientation)) {

flip-rotate-X as expressed in the documentation: 'flip' means an horizontal
mirroring.

Right now, flipping is not merged in libva-intel-driver (see
https://bugs.freedesktop.org/show_bug.cgi?id=90654 ), so this patch should not
implement it. It would be great if you can test the patches in the
libva-intel-driver bug.

@@ +1787,3 @@
+   */
+  g_properties[PROP_ORIENTATION_ENABLE] =
+      g_param_spec_boolean ("orientation-enable",

enable-orientation would be better.


But, I prefer to add an AUTOMATIC option in the orientation enum property.

@@ +1905,3 @@
   sink->rotation = DEFAULT_ROTATION;
   sink->rotation_req = DEFAULT_ROTATION;
+  sink->orient_enable = FALSE;

Thinking about it, perhaps it should be TRUE by default. But if we keep if as
false, it is not necessary to express it explicitly here, since 0/FALSE is the
default value.

::: tests/Makefile.am
@@ +155,3 @@
+test_vaapisink_CFLAGS    = $(TEST_CFLAGS)
+test_vaapisink_LDFLAGS    = $(GST_VAAPI_LIBS)
+test_vaapisink_LDADD    = libutils.la $(TEST_LIBS)

Is it truly need to link libutils?

::: tests/test-vaapisink.c
@@ +3,3 @@
+#include <gst/gst.h>
+#include <gst/base/gstbasesrc.h>
+

It is cool to have this test, but the directory tests is only for the internal
API, known as libgstvaapi.
I would create a subdirectory tests/elements with these kind of gstreamer-based
elements.

@@ +70,3 @@
+  /* Print usage map */
+  g_print ("USAGE: Choose one of the following options, then press enter:\n"
+      " 'S' to send image-orientation tag event\n 'Q' to quit\n");

S or R????

@@ +78,3 @@
+  data.video_sink = gst_bin_get_by_name (GST_BIN (data.pipeline),
"vaapisink");
+  data.src_sink = gst_bin_get_by_name (GST_BIN (data.pipeline), "src");
+  data.src_pad = GST_BASE_SRC_PAD (data.src_sink);

Shouldn't we use gst_element_get_static_pad() instead of using gst internal
API?
I don't know... is it possible?

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