GNOME Bugzilla – Bug 765795
glimagesink: support video frame rotation
Last modified: 2016-05-25 08:30:20 UTC
Hi, Do we have plan to support video frame rotation in glimagesink? This feature is not hard to realize. For example, add a "rotate" property and re-calculate the texture coordinate when this property changed.
That would be a good idea, and it should also come with automatic rotation/flipping based on the GST_TAG_IMAGE_ORIENTATION. Similar to what videoflip does.
(In reply to Sebastian Dröge (slomo) from comment #1) > That would be a good idea, and it should also come with automatic > rotation/flipping based on the GST_TAG_IMAGE_ORIENTATION. Similar to what > videoflip does. So，we can implement this two property in glimagesink and also automatic change texture coordinate by GST_TAG_IMAGE_ORIENTATION. This feature will be useful for us and I will try to enable this.
Why two properties? Take a look at what videoflip does
(In reply to Sebastian Dröge (slomo) from comment #3) > Why two properties? Take a look at what videoflip does I got your point. I am going to enable this feature just like videoflip does
It might also make sense to do this as another element inside glimagesink, just like glvideobalance, etc.
(In reply to Sebastian Dröge (slomo) from comment #5) > It might also make sense to do this as another element inside glimagesink, > just like glvideobalance, etc. You mean something like "glvideoflip" and put it into glimagesinkbin. The main concern is performance, if we just put it in glimagesink as a property, we only need to calculate texture coordinate when the property changes, but if we treat it as a plugin,we need to do fbo rendering on every buffer, this will cost some time and will cause performance downgrade. So I think property is better.
Maybe both :) Performance-wise you're right, it also seems simple enough. A separate element has its advantages too though. Or maybe flipping/rotation can be part of GLUpload/GLDownload too?
(In reply to Sebastian Dröge (slomo) from comment #7) > Maybe both :) Performance-wise you're right, it also seems simple enough. A > separate element has its advantages too though. Or maybe flipping/rotation > can be part of GLUpload/GLDownload too? Yes, It is simple enough. Please wait for my patch :)
Hi Matthew, I notice that you have already implemented a element named "glvideoflip", what's your opinion about this new feature?
My opinion is that removing unnecessary stuff from glimagesinkelement and having separate distinct elements that only do one thing is a good architectural goal. That comes at the cost of performance though which isn't ideal. For this, if adding glvideoflip to glimagesinkbin is good enough performance-wise, then that's what should happen. Otherwise, we should attempt to reuse the GstVideoAffineTransformationMeta for this to avoid adding extra unnecessary code to glimagesink. See https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gl/gstglimagesink.c#n2028
Another option is to do both and make glvideoflip ! glimagesink negotiate the affine transformation meta and glvideoflip goes passthrough.
(In reply to Matthew Waters (ystreet00) from comment #10) > My opinion is that removing unnecessary stuff from glimagesinkelement and > having separate distinct elements that only do one thing is a good > architectural goal. That comes at the cost of performance though which > isn't ideal. > > For this, if adding glvideoflip to glimagesinkbin is good enough > performance-wise, then that's what should happen. Otherwise, we should > attempt to reuse the GstVideoAffineTransformationMeta for this to avoid > adding extra unnecessary code to glimagesink. See On some embed system, adding glvideoflip to glimagesinkbin will definitely cost performance downgrade. For example on my imx6Q. You mean we also could use transform matrix in vertex shader to get rotation effect? Use a property to control it? > https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gl/ > gstglimagesink.c#n2028
(In reply to Haihua Hu from comment #12) > On some embed system, adding glvideoflip to glimagesinkbin will definitely > cost performance downgrade. For example on my imx6Q. You mean we also could > use transform matrix in vertex shader to get rotation effect? Use a property > to control it? yes, use what's already there, i.e. the GstVideoAffineTransformationMeta.
Created attachment 327550 [details] [review] Support video rotation in glimagesink This patch apply a transform matrix to vertex coordinate to realize rotation
Hi All, Any comments for this patch?
Review of attachment 327550 [details] [review]: . ::: ext/gl/gstglimagesink.c @@ +2290,3 @@ + (gl_sink->stored_buffer[0]); + if (af_meta) + gl_sink->transform_matrix = af_meta->matrix; What if there's a transformation meta and the rotate property is set to !identity? We need to combine the two transformations. ::: gst-libs/gst/gl/gstglshaderstrings.c @@ +45,3 @@ + " gl_Position = u_transformation * a_position;\n" + " v_texcoord = a_texcoord;\n" + "}\n"; Why do you need another shader? You can do the same with the existing gst_gl_shader_string_vertex_mat4_texture_transform shader with a slightly different matrix.
(In reply to Matthew Waters (ystreet00) from comment #16) > Review of attachment 327550 [details] [review] [review]: > > . > > ::: ext/gl/gstglimagesink.c > @@ +2290,3 @@ > + (gl_sink->stored_buffer[0]); > + if (af_meta) > + gl_sink->transform_matrix = af_meta->matrix; > > What if there's a transformation meta and the rotate property is set to > !identity? We need to combine the two transformations. Yes, the result should be the multiple of these two transformations. > > ::: gst-libs/gst/gl/gstglshaderstrings.c > @@ +45,3 @@ > + " gl_Position = u_transformation * a_position;\n" > + " v_texcoord = a_texcoord;\n" > + "}\n"; > > Why do you need another shader? You can do the same with the existing > gst_gl_shader_string_vertex_mat4_texture_transform shader with a slightly > different matrix. I have some issue by using the exist shader. I apply the rotation transform to the texture coordinate,then the video frame seems to be repeat of one single line, is there something wrong with my code? My transform is such as clockwise-90: 0.0f, -1.0f, 0.0, 0.0f, 1.0f, 0.0f, 0.0, 0.0f, 0.0f, 0.0f, 1.0, 0.0f, 0.0f, 0.0f, 0.0, 1.0f,
The current transformation is based on 0 to 1 not -1 to 1 as is normal with transformations so you have to modify the matrix for that.
(In reply to Matthew Waters (ystreet00) from comment #18) > The current transformation is based on 0 to 1 not -1 to 1 as is normal with > transformations so you have to modify the matrix for that. Hi Matthew， I met a problem, when I just apply a translation matrix to the texture coordinated which is intent to set the origin to point(0.5 0.5), then nothing happen, the video frame do not move. blow is the matrix v_texcoord = (u_transformation * vec4(a_texcoord, 0, 1)).xy; static const gfloat identity_matrix[] = { 1.0f, 0.0f, 0.0f, -0.5f, 0.0f, 1.0f, 0.0f, -0.5f, 0.0f, 0.0f, 1.0f, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f, }; but when I just v_texcoord = a_texcoord-0.5, then the origin move to the middle. What is the difference? where is wrong?
(In reply to Matthew Waters (ystreet00) from comment #18) > The current transformation is based on 0 to 1 not -1 to 1 as is normal with > transformations so you have to modify the matrix for that. By the way, why do we apply u_transformation to texture coordinate but not vertex coordinate? What effects do you want to realize? Since rotation transform on vertex coordinate is simpler than texture coordinate. Is it ok to change to apply u_transformation to vertex?
Created attachment 327972 [details] [review] modify the patch This patch is based on the lastest master branch of gst-plugins-bad
Review of attachment 327972 [details] [review]: . ::: ext/gl/gstglimagesink.c @@ +191,3 @@ + {GST_GL_ROTATE_METHOD_AUTO, + "Select rotate method based on image-orientation tag", "automatic"}, + {0, NULL, NULL}, This is missing the flipped methods that are available in videoflip and glvideoflip, i.e. horizontal-flip, vertical-flip, upper-left-diagonal, upper-right-diagonal. ::: gst-libs/gst/gl/gstglutils.h @@ +119,3 @@ +void gst_gl_get_affine_transformation_meta_as_ndc (GstVideoAffineTransformationMeta * meta, + const gfloat * rotate_matrix, gfloat * matrix); I don't like the changing of this function. You should separate out the retrieval and the matrix multiply into another function like gst_gl_matrix_multiply()
Created attachment 328111 [details] [review] Add more video rotate method
(In reply to Matthew Waters (ystreet00) from comment #22) > Review of attachment 327972 [details] [review] [review]: > > . > > ::: ext/gl/gstglimagesink.c > @@ +191,3 @@ > + {GST_GL_ROTATE_METHOD_AUTO, > + "Select rotate method based on image-orientation tag", "automatic"}, > + {0, NULL, NULL}, > > This is missing the flipped methods that are available in videoflip and > glvideoflip, i.e. horizontal-flip, vertical-flip, upper-left-diagonal, > upper-right-diagonal. Have add these rotate method. > > ::: gst-libs/gst/gl/gstglutils.h > @@ +119,3 @@ > > +void gst_gl_get_affine_transformation_meta_as_ndc > (GstVideoAffineTransformationMeta * meta, > + const gfloat * rotate_matrix, gfloat * matrix); > > I don't like the changing of this function. > > You should separate out the retrieval and the matrix multiply into another > function like gst_gl_matrix_multiply() Just modify the origin _mulyiply_matrix4() function.
Any comments for this new patch？
commit a5cb7469835e73285aa1b82f8c4defafb5760152 Author: Haihua Hu <jared.hu@nxp.com> Date: Mon May 16 20:02:28 2016 +0800 glimagesink: support video rotation using transform matrix Add "rotate-method" to glimagesink and apply transform matrix to vertex coordinate to control rotation. https://bugzilla.gnome.org/show_bug.cgi?id=765795