GNOME Bugzilla – Bug 765798
vaapisink: add support for GST_TAG_IMAGE_ORIENTATION
Last modified: 2016-07-05 19:17:59 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.
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?
(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.
The element structs are private already, since headers are not installed, so you can remove/rename/reshuffle/add things to your liking.
Ah right, as you both said, this header is used by only vaapisink:)
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?
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
(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.
Created attachment 330699 [details] [review] vaapisink: add support for GST_TAG_IMAGE_ORIENTATION Change to orientation-automatic prop. Doesn't support for flip method
Created attachment 330700 [details] [review] tests:elements: Add testsuite for vaapisink Create tests/elements directory and put this testsuite.
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.
Review of attachment 330700 [details] [review]: looks good to me
Created attachment 330883 [details] [review] vaapisink: add support for GST_TAG_IMAGE_ORIENTATION
Created attachment 330884 [details] [review] tests:elements: Add testsuite for vaapisink
Comment on attachment 330883 [details] [review] vaapisink: add support for GST_TAG_IMAGE_ORIENTATION it looks good to me
Comment on attachment 330884 [details] [review] tests:elements: Add testsuite for vaapisink I'll commit a small update to this after push it
Attachment 330883 [details] pushed as e8fabf6 - vaapisink: add support for GST_TAG_IMAGE_ORIENTATION