GNOME Bugzilla – Bug 731791
videometa: add GstVideoAffineTransformationMeta
Last modified: 2015-11-10 13:20:49 UTC
Hello, Here is a proposal for a new buffer metadata responsible for carrying a 4x4 transformation matrix. This implementation let user specify a custom transformation matrix as well as a custom get_matrix function, see get_buffer_add_video_affine_transformation_meta() function. The custom get_matrix function can be helpful when the transformation matrix have to be retrieved from the buffer to which the meta is attached to. I'm thinking about the android media buffers here where a transformation matrix has to be retrieved from the android surface texture after updateTexImage() has been called and be used to sample the texture. In this case the get_matrix function will retrieved the transformation matrix from the surface texture stored in the buffer and then multiply it with the matrix stored in the meta. The user can also apply transformations using the gst_video_affine_transformation_apply_matrix() function. The resulting matrix is retrieved by the gst_video_affine_transformation_get_matrix() function. Comments welcome.
Created attachment 278612 [details] [review] videometa: add GstVideoAffineTransformationMeta
Review of attachment 278612 [details] [review]: How is it negotiated? Are users of this API expected to handle all possible transformations? What's the expected behaviour for transformations that convert a rectangular video to a non-rectangular one (or non multiple of 90° rotations)? ::: gst-libs/gst/video/gstvideoaffinetransformationmeta.h @@ +46,3 @@ + * gst_video_affine_transformation_meta_get_matrix. + * The user can provide its own get_matrix function, which can be used to + * retrieve a transformation matrix from the buffer. Define if this is matrix is stored by rows or by columns, and maybe use a [4][4] array instead? @@ +57,3 @@ + + gfloat matrix[16]; + GstVideoAffineTransformationGetMatrix get_matrix; Why the get_matrix function pointer? What is this supposed to do other than returning the matrix?
(In reply to comment #2) > Review of attachment 278612 [details] [review]: > > How is it negotiated? Are users of this API expected to handle all possible > transformations? What's the expected behaviour for transformations that convert > a rectangular video to a non-rectangular one (or non multiple of 90° > rotations)? I haven't though about it yet. > > ::: gst-libs/gst/video/gstvideoaffinetransformationmeta.h > @@ +46,3 @@ > + * gst_video_affine_transformation_meta_get_matrix. > + * The user can provide its own get_matrix function, which can be used to > + * retrieve a transformation matrix from the buffer. > > Define if this is matrix is stored by rows or by columns, and maybe use a > [4][4] array instead? The matrix is stored by rows. > > @@ +57,3 @@ > + > + gfloat matrix[16]; > + GstVideoAffineTransformationGetMatrix get_matrix; > > Why the get_matrix function pointer? What is this supposed to do other than > returning the matrix? As I said in the bug description, it is used to provide a way for the user to fetch the matrix from the buffer / buffer memory. I'm thinking about the android buffers where the matrix has to be retrieved from the surface texture, after the texture is updated with the updateTexImage. But i'm sure if this design is right since it will imply that the upload function is called before the [...]_get_matrix function is. The default implementation just returns the stored matrix.
(In reply to comment #2) > Review of attachment 278612 [details] [review]: > > How is it negotiated? Are users of this API expected to handle all possible > transformations? What's the expected behaviour for transformations that convert > a rectangular video to a non-rectangular one (or non multiple of 90° > rotations)? Negotiated through the allocation query. I don't think we would need to depend on bug #727886 for this. Downstream says it supports it or not, upstream decides to abort or use it/something else/nothing based on that. Yes the possibility is there for all possible transformations to occur (translate, scale, rotate, etc). If the downstream element would like the bounds of the rotated image inside the bounds of the output image, then it can set that up using the matrix. If a transformed pixel is outside the bounds of the output image, then it should discard that pixel. Likewise, if an output pixel does not correspond to some pixel in the transformed input image, then that pixel should be set to some background colour, typically black. (In reply to comment #3) > > @@ +57,3 @@ > > + > > + gfloat matrix[16]; > > + GstVideoAffineTransformationGetMatrix get_matrix; > > > > Why the get_matrix function pointer? What is this supposed to do other than > > returning the matrix? > > As I said in the bug description, it is used to provide a way for the user to > fetch the matrix from the buffer / buffer memory. I'm thinking about the > android buffers where the matrix has to be retrieved from the surface texture, > after the texture is updated with the updateTexImage. But i'm sure if this > design is right since it will imply that the upload function is called before > the [...]_get_matrix function is. > > The default implementation just returns the stored matrix. What if we assume that any GstMeta that we call a function on could modify (add?) any other GstMeta attached to the buffer unless explicitly stated otherwise. With that in mind, the upload meta could modify the transform meta and thus would need to be called after the upload meta. If we allow the addition of meta's by GstMeta user functions then it must be retrieved from the buffer after all meta-modifying operations have completed. How does that sound?
Or what about some kind of GstMeta that specifies the ordering that downstream expects.
Created attachment 315115 [details] [review] video: add GstVideoAffineTransformationMeta This version drops the vfunc from the meta and simply contains a float array for the 4x4 matrix.
Review of attachment 315115 [details] [review]: ::: gst-libs/gst/video/gstvideoaffinetransformationmeta.c @@ +28,3 @@ + static volatile GType type = 0; + static const gchar *tags[] = + { GST_META_TAG_VIDEO_STR, GST_META_TAG_MEMORY_STR, NULL }; Why memory? This should only affect "video" and maybe "video-orientation" or something like that @@ +99,3 @@ + * @buffer: a #GstBuffer + * @matrix: a 4x4 transformation matrix + * @get_matrix: a custom get_matrix function to be used by the This functions has no matrix and get_matrix parameters. Why doesn't it have a matrix parameter? :) @@ +144,3 @@ + GST_ERROR ("%i %i %i", i, j, k); + GST_ERROR ("%f", meta->matrix[i + (k * 4)]); + GST_ERROR ("%f", matrix[k + (j * 4)]); Forgot to remove debug output here ::: gst-libs/gst/video/gstvideoaffinetransformationmeta.h @@ +60,3 @@ + +void gst_video_affine_transformation_meta_apply_matrix (GstVideoAffineTransformationMeta * meta, + gfloat * matrix); Could also make that a "gfloat matrix[16]" which is basically the same but provides more information about the intent.
Created attachment 315167 [details] [review] video: add GstVideoAffineTransformationMeta Addressed review comments. (In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 315115 [details] [review] [review]: > @@ +99,3 @@ > + * @buffer: a #GstBuffer > + * @matrix: a 4x4 transformation matrix > + * @get_matrix: a custom get_matrix function to be used by the > > This functions has no matrix and get_matrix parameters. Why doesn't it have > a matrix parameter? :) It's slightly simpler without the matrix parameter and the use case I have does not have the matrix when the meta needs to be added to the buffer. If you want we can always add it later in a _full() variant. The same functionality can always be achieved with meta = add_meta(); apply_matrix (meta, matrix).
Created attachment 315174 [details] [review] video: add GstVideoAffineTransformationMeta Updated patch
Review of attachment 315174 [details] [review]: Go ahead, just some minor things to clean up before pushing :) Please also add it to the docs and the win32 .def files (run make check-exports for the latter). ::: gst-libs/gst/video/gstvideoaffinetransformationmeta.c @@ +29,3 @@ + static volatile GType type = 0; + static const gchar *tags[] = + { GST_META_TAG_VIDEO_STR, GST_META_TAG_VIDEO_ORIENTATION_STR, NULL }; And video-dimensions I guess. If you scale, anything that uses absolute pixel coordinates also needs to be updated @@ +103,3 @@ + * the given parameters. + * + * Returns: the #GstVideoAffineTransformationMeta on @buffer. Since: 1.8 Also for everything else
commit 0b98ed32ce9db48089787f32c13c1a3db3ac36f3 Author: Matthew Waters <matthew@centricular.com> Date: Mon Nov 2 23:12:19 2015 +1100 videometa: add GstVideoAffineTransformationMeta Adds a simple 4x4 affine transformations meta for passing arbitrary transformations on buffers. Based on patch by Matthieu Bouron https://bugzilla.gnome.org/show_bug.cgi?id=731791