After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 765798 - vaapisink: add support for GST_TAG_IMAGE_ORIENTATION
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-29 08:33 UTC by Víctor Manuel Jáquez Leal
Modified: 2016-07-05 19:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION (8.86 KB, patch)
2016-05-16 03:22 UTC, Hyunjun Ko
none Details | Review
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION (6.56 KB, patch)
2016-07-01 07:04 UTC, Hyunjun Ko
none Details | Review
tests:elements: Add testsuite for vaapisink (6.01 KB, patch)
2016-07-01 07:04 UTC, Hyunjun Ko
none Details | Review
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION (5.74 KB, patch)
2016-07-05 00:43 UTC, Hyunjun Ko
committed Details | Review
tests:elements: Add testsuite for vaapisink (5.91 KB, patch)
2016-07-05 00:44 UTC, Hyunjun Ko
committed Details | Review

Description Víctor Manuel Jáquez Leal 2016-04-29 08:33:24 UTC
As in the element videoflip, we could add an event handler in vaapisink where if the tag GST_TAG_IMAGE_ORIENTATION is received, automatically do a rotation of the video.

This behavior could be disabled through a property.
Comment 1 Hyunjun Ko 2016-05-16 03:22:50 UTC
Created attachment 327956 [details] [review]
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION

To handle image-orientation tag event.
What I'm wondering is that abi-break because of adding new member 'orient_enable'

There is no private structure in vaapisink.
Should I add new private structure to vaapisink?
Comment 2 Víctor Manuel Jáquez Leal 2016-05-16 10:54:35 UTC
(In reply to Hyunjun Ko from comment #1)
> Created attachment 327956 [details] [review] [review]
> vaapisink: add support for GST_TAG_IMAGE_ORIENTATION
> 
> To handle image-orientation tag event.
> What I'm wondering is that abi-break because of adding new member
> 'orient_enable'
> 
> There is no private structure in vaapisink.
> Should I add new private structure to vaapisink?

It should be nice to add a private structure to the element, but, as far as I understand, there is no big deal breaking the ABI in elements since they are not libraries.
Comment 3 Tim-Philipp Müller 2016-05-16 11:08:48 UTC
The element structs are private already, since headers are not installed, so you can remove/rename/reshuffle/add things to your liking.
Comment 4 Hyunjun Ko 2016-05-17 01:49:05 UTC
Ah right, as you both said, this header is used by only vaapisink:)
Comment 5 Víctor Manuel Jáquez Leal 2016-06-16 15:48:20 UTC
Review of attachment 327956 [details] [review]:

::: 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?
Comment 6 Nicolas Dufresne (ndufresne) 2016-06-16 15:58:03 UTC
Imho, the best property would be to mimic videoflip, this way you give full control to the user, including being able to flip without this tag.

  method              : method
                        flags: accès en lecture, accès en écriture, contrôlable
                        Enum "GstVideoFlipMethod" Default: 0, "none"
                           (0): none             - Identity (no rotation)
                           (1): clockwise        - Rotate clockwise 90 degrees
                           (2): rotate-180       - Rotate 180 degrees
                           (3): counterclockwise - Rotate counter-clockwise 90 degrees
                           (4): horizontal-flip  - Flip horizontally
                           (5): vertical-flip    - Flip vertically
                           (6): upper-left-diagonal - Flip across upper left/lower right diagonal
                           (7): upper-right-diagonal - Flip across upper right/lower left diagonal
                           (8): automatic        - Select flip method based on image-orientation tag
Comment 7 Víctor Manuel Jáquez Leal 2016-06-16 16:03:04 UTC
(In reply to Nicolas Dufresne (stormer) from comment #6)
> Imho, the best property would be to mimic videoflip, this way you give full
> control to the user, including being able to flip without this tag.

Yup, that's what I think too.
Comment 8 Hyunjun Ko 2016-07-01 07:04:08 UTC
Created attachment 330699 [details] [review]
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION

Change to orientation-automatic prop.
Doesn't support for flip method
Comment 9 Hyunjun Ko 2016-07-01 07:04:43 UTC
Created attachment 330700 [details] [review]
tests:elements: Add testsuite for vaapisink

Create tests/elements directory and put this testsuite.
Comment 10 Víctor Manuel Jáquez Leal 2016-07-01 11:29:53 UTC
Review of attachment 330699 [details] [review]:

::: gst/vaapi/gstvaapisink.c
@@ +1794,3 @@
+      "Select to set orientation based on image-orientation tag automatically",
+      TRUE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+

Sorry, I didn't  explain myself correctly.

We should be very careful when we add new interfaces to the user. I don't see the need to add another knob for this feature.

Right now vaapisink has a property for rotation:

rotation            : The display rotation mode
                        flags: readable, writable
                        Enum "GstVaapiRotation" Default: 0, "0"
                           (0): 0                - Unrotated mode
                           (90): 90               - Rotated by 90°, clockwise
                           (180): 180              - Rotated by 180°, clockwise
                           (270): 270              - Rotated by 270°, clockwise
 
The idea, then, is to mimic videoflip's property "method". This is, to add a new item in the rotation enum, called "automatic", which will enable this feature.
Comment 11 Víctor Manuel Jáquez Leal 2016-07-01 11:29:55 UTC
Review of attachment 330699 [details] [review]:

::: gst/vaapi/gstvaapisink.c
@@ +1794,3 @@
+      "Select to set orientation based on image-orientation tag automatically",
+      TRUE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+

Sorry, I didn't  explain myself correctly.

We should be very careful when we add new interfaces to the user. I don't see the need to add another knob for this feature.

Right now vaapisink has a property for rotation:

rotation            : The display rotation mode
                        flags: readable, writable
                        Enum "GstVaapiRotation" Default: 0, "0"
                           (0): 0                - Unrotated mode
                           (90): 90               - Rotated by 90°, clockwise
                           (180): 180              - Rotated by 180°, clockwise
                           (270): 270              - Rotated by 270°, clockwise
 
The idea, then, is to mimic videoflip's property "method". This is, to add a new item in the rotation enum, called "automatic", which will enable this feature.
Comment 12 Víctor Manuel Jáquez Leal 2016-07-01 11:31:11 UTC
Review of attachment 330700 [details] [review]:

looks good to me
Comment 13 Hyunjun Ko 2016-07-05 00:43:53 UTC
Created attachment 330883 [details] [review]
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION
Comment 14 Hyunjun Ko 2016-07-05 00:44:26 UTC
Created attachment 330884 [details] [review]
tests:elements: Add testsuite for vaapisink
Comment 15 Víctor Manuel Jáquez Leal 2016-07-05 18:56:42 UTC
Comment on attachment 330883 [details] [review]
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION

it looks good to me
Comment 16 Víctor Manuel Jáquez Leal 2016-07-05 18:57:40 UTC
Comment on attachment 330884 [details] [review]
tests:elements: Add testsuite for vaapisink

I'll commit a small update to this after push it
Comment 17 Víctor Manuel Jáquez Leal 2016-07-05 19:13:56 UTC
Attachment 330883 [details] pushed as e8fabf6 - vaapisink: add support for GST_TAG_IMAGE_ORIENTATION