GNOME Bugzilla – Bug 768687
Rework video orientation
Last modified: 2016-08-25 07:21:35 UTC
Both [gl]videoflip (and now also rpicamsrc) implement the same property based interface to set the orientation/flip. The intention of this rework is to unify the values and add the property to the interface so that classes implementing it only have to override it.
Created attachment 331250 [details] [review] gst-plugins-base 1/3
Created attachment 331251 [details] [review] gst-plugins-base 2/3
Created attachment 331252 [details] [review] gst-plugins-base 3/3
Created attachment 331253 [details] [review] gst-plugins-good 1/2
Created attachment 331254 [details] [review] gst-plugins-good 2/2
Created attachment 331255 [details] [review] gst-plugins-bad 1/2
Created attachment 331257 [details] [review] gst-plugins-bad 2/2
Review of attachment 331253 [details] [review]: ::: gst/videofilter/gstvideoflip.c @@ -60,1 +60,1 @@ This would be an ABI break though. Identity is like none, while auto is orientation tag driven.
Review of attachment 331252 [details] [review]: This also breaks ABI: any implementor of the interface will fail now because the property is not implemented. It should become a new interface to prevent that
Review of attachment 331253 [details] [review]: I was puzzled by this and that's why I set to AUTO, but I can change it back static void gst_video_flip_init (GstVideoFlip * videoflip) { /* AUTO is not valid for active method, this is just to ensure we setup the * method in gst_video_flip_set_method() */ videoflip->active_method = GST_VIDEO_ORIENTATION_AUTO; }
(In reply to Xabier Rodríguez Calvar from comment #10) > Review of attachment 331253 [details] [review] [review]: > > I was puzzled by this and that's why I set to AUTO, but I can change it back > > static void > gst_video_flip_init (GstVideoFlip * videoflip) > { > /* AUTO is not valid for active method, this is just to ensure we setup the > * method in gst_video_flip_set_method() */ > videoflip->active_method = GST_VIDEO_ORIENTATION_AUTO; > } We have the method and a copy (the active one) of that method. This is to be able to remember we are in auto mode, and so the get_property() return what the user have set. The active method cannot be AUTO since it would mean we have no idea what to do. During negotiation, if there was no tag, we change that into identity. If there is a tag, we keep the tag. We also need the copy to remember in the first. So auto here is used as a tempory "invalid" value, to signal that we are not negotiated.
Created attachment 331336 [details] [review] gst-plugins-base 2/5
Created attachment 331337 [details] [review] gst-plugins-base 3/5
Created attachment 331338 [details] [review] gst-plugins-base 4/5
Created attachment 331339 [details] [review] gst-plugins-base 5/5
Created attachment 331340 [details] [review] gst-plugins-good 1/2
Created attachment 331341 [details] [review] gst-plugins-good 2/2
Created attachment 331342 [details] [review] gst-plugins-bad 1/2
Created attachment 331343 [details] [review] gst-plugins-bad 2/2
Review of attachment 331336 [details] [review]: ::: gst-libs/gst/video/video.h @@ +71,3 @@ + * @GST_VIDEO_ORIENTATION_CUSTOM: Current status depends on plugin internal setup + * + * The different video orientation methods. Since: 1.10
Review of attachment 331337 [details] [review]: ::: gst-libs/gst/video/videodirection.c @@ +43,3 @@ +{ + g_object_interface_install_property (iface, + g_param_spec_enum ("direction", "Direction", Maybe "video-direction", less likelyness of name conflict. I don't like "direction" here in general but what can we do :)
Comment on attachment 331339 [details] [review] gst-plugins-base 5/5 And squash all the base patches together into one please :)
Comment on attachment 331340 [details] [review] gst-plugins-good 1/2 This breaks ABI. The names of the enum values are changing (none -> identity, upper-left-horizonal -> ul-lr), etc.
Comment on attachment 331341 [details] [review] gst-plugins-good 2/2 squash together with the other -good change. For the enum, I think we need to keep both and translate internally.
Comment on attachment 331342 [details] [review] gst-plugins-bad 1/2 same as for the -good change
(In reply to Sebastian Dröge (slomo) from comment #23) > Comment on attachment 331340 [details] [review] [review] > gst-plugins-good 1/2 > > This breaks ABI. The names of the enum values are changing (none -> > identity, upper-left-horizonal -> ul-lr), etc. (In reply to Sebastian Dröge (slomo) from comment #24) > Comment on attachment 331341 [details] [review] [review] > gst-plugins-good 2/2 > > For the enum, I think we need to keep both and translate internally. How can I do this? Won't it duplicate the names? The only solution I can come up for this is that we keep the original enums for the old properties...
Created attachment 332171 [details] [review] gst-plugins-base 2/2
Created attachment 332172 [details] [review] gst-plugins-good 1/1
Created attachment 332173 [details] [review] gst-plugins-bad 1/1
Btw, Sebastian, we didn't talk about this during GUADEC, but what about this? O:)
Does it keep the old enum? Apart from that it all looked good to me last time. Are you proposing this for 1.10? It would also be nice to have playbin/playsink implement that, and automatically insert videoflip if the sink does not support it. Just like colorbalance :)
(In reply to Sebastian Dröge (slomo) from comment #31) > Does it keep the old enum? I am keeping the old enums in order not to break ABI but I changed them internally cause values are the same. > Apart from that it all looked good to me last > time. Are you proposing this for 1.10? Yep, why not? :) > It would also be nice to have playbin/playsink implement that, and > automatically insert videoflip if the sink does not support it. Just like > colorbalance :) Though I can't now, it would be nice yes.
commit 0341f04ce161e9a09345ffe84036bdb023c4e94f Author: Xabier Rodriguez Calvar <calvaris@igalia.com> Date: Tue Jul 26 19:14:40 2016 +0200 videodirection: interface for rotation and flip A GstVideoOrientationMethod enumeration is also provided for the admitted property values. https://bugzilla.gnome.org/show_bug.cgi?id=768687
Comment on attachment 332172 [details] [review] gst-plugins-good 1/1 commit 569820598f3ca29a9f62685da6e745329608c2dd Author: Xabier Rodriguez Calvar <calvaris@igalia.com> Date: Tue Jul 26 19:39:58 2016 +0200 videoflip: added GstVideoDirection interface It implements now this interface with its video-direction property. Values are changed to GstVideoOrientationMethod but they have the same value than the originals. https://bugzilla.gnome.org/show_bug.cgi?id=768687
Comment on attachment 332173 [details] [review] gst-plugins-bad 1/1 commit 8ada38e8f4b92842f9a3cc7243c262b3d9b7d7f8 Author: Xabier Rodriguez Calvar <calvaris@igalia.com> Date: Tue Jul 26 19:55:13 2016 +0200 glvideoflip: implement GstVideoDirection interface It implements now this interface with its video-direction property. Values are changed to GstVideoOrientationMethod but they have the same value than the originals. https://bugzilla.gnome.org/show_bug.cgi?id=768687