GNOME Bugzilla – Bug 589399
Needs a way to rotate or flip video playback
Last modified: 2013-06-11 10:06:41 UTC
Something that would be needed for totem is a rotation plugin, where you can select a 90, 180 or 270 degree rotation of the video, i.e. not the whole screen as with RandR. Not a conversion utility, just transforming the playback stream. Gstreamer has a videoflip element for this, I guess it chooses software or hardware method from what's available. This is commonly needed when viewing clips from phones or compact cameras, which are often flipped. It would be cool also if preferences could be stored in one place for all videos, so that next time you reopen the video it is already rotated as chosen previously.
This looks like a good idea. We'd probably use the look-aside cache to save the rotation, just like we save the position when paused.
Created attachment 159013 [details] [review] video rotation patch I wrote a patch to add video rotation support for Totem using the VideoFlip element. I works, but there is one small issue: If you rotate the video in the paused state, then the current frame is not redrawn properly. I'm not a Gstreamer expert. Suggestions how I could improve my patch are very welcome.
The first thing I'll note, is that this needs to be a plugin, not added directly to the core of Totem. This might require adding API to the BaconVideoWidget to allow plugins to add and remove elements in the pipeline.
Writing it as a plugin was my first plan too, but the BaconVideoWidget would then become Gstreamer specific. Will Gstreamer be the only backend? Is the xine backend still used somewhere?
(In reply to comment #4) > Writing it as a plugin was my first plan too, but the BaconVideoWidget would > then become Gstreamer specific. It already is, really, since we removed the xine-lib backend. There is still a lot of implementation details that the widget hides, and should continue to. > Will Gstreamer be the only backend? Yes, for the foreseeable future. > Is the xine > backend still used somewhere? No, if it is, it would only be in older versions.
Comment on attachment 159013 [details] [review] video rotation patch This needs to be added as a plugin.
Note that we could either use the videoflip element in GStreamer, or we could animate this properly in the video widget by simply rotating the clutter actor containing the video (though it might have side-effects with pixel aspect ratio, etc.). Still needs to be a plugin though.
Created attachment 191250 [details] [review] rotation plugin Finally I found the time to write a plugin to rotate videos. It directly transforms the clutter actor that contains the video. Totem's clutter actors are now named so that plugins can query them. The plugin depends on the most recent Vala release (Gir parsing issue) and BGO #653934 (incorrect Gir annotation in Clutter-Gtk).
Review of attachment 191250 [details] [review]: Here's a brief review, assuming that Bastien acks the overall approach. I haven't had a chance to compile or run it yet. ::: src/backend/bacon-video-widget-gst-0.10.c @@ +6076,3 @@ /* The video */ bvw->priv->frame = totem_aspect_frame_new (); + clutter_actor_set_name (bvw->priv->frame, "frame"); Could you split these changes out into a separate commit please? They're not directly related to adding the rotation plugin, even though it needs them. ::: src/plugins/rotation/Makefile.am @@ +7,3 @@ + +# here we are explicitly specifying gtk+-3.0 to use the vapi because vala still +# cannot parse the gir Would be useful to add a bug reference to this comment. @@ +14,3 @@ +librotation_la_SOURCES = totem-rotation-plugin.vala +librotation_la_LDFLAGS = $(plugin_ldflags) +librotation_la_CFLAGS = $(plugin_cflags) -include $(CONFIG_HEADER) What's the “-include $(CONFIG_HEADER)” needed for? ::: src/plugins/rotation/rotation.plugin.in @@ +3,3 @@ +IAge=1 +_Name=Rotation Plugin +_Description=Rotates videos How about “Allows videos to be rotated if they're in the wrong orientation.”? ::: src/plugins/rotation/totem-rotation-plugin.vala @@ +41,3 @@ + _180 = 2, + _90L = 3, + _COUNT = 4 Why are these all prefixed by underscores? I think it would make more sense to have _COUNT defined separately, since you wouldn't put it in a case statement over an instance of Rotation. @@ +46,3 @@ + public unowned GLib.Object object { get; construct; } + private unowned Totem.Object t; + private unowned Clutter.Actor video = null; Although at the moment there's no difference between the two, I think it would technically be better to define these as “weak” rather than “unowned”. @@ +55,3 @@ + public void activate () + { + t = (Totem.Object) object; I think it would be clearer to explicitly include “this.” in statements referring to class variables. Otherwise it's hard to work out their scope. @@ +80,3 @@ + // search for the actor which contains the video + for (int i = 0; i < stage.get_n_children (); i++) + { Opening braces on the previous line. @@ +82,3 @@ + { + Clutter.Actor actor = stage.get_nth_child (i); + if(actor.name == "frame") { Need a space before the opening parenthesis. @@ +97,3 @@ + + // get notified if the video gets resized + video.allocation_changed.connect (cb_allocation_changed); You should disconnect from this in deactivate(). @@ +111,3 @@ + video.set_rotation (Clutter.RotateAxis.Z_AXIS, 0, 0, 0, 0); + video.set_scale_full (1, 1, 0, 0); + } Something funny going on with the indentation here. @@ +136,3 @@ + width = box.x2 - box.x1; + height = box.y2 - box.y1; + update_video_geometry (); Again, I think it's clearer to explicitly include the “this.” in method calls. @@ +152,3 @@ + + /* Gir doesn't allow marking functions as non-abstract */ + public void update_state () {} It might be worth calling this.update_video_geometry() from update_state(), just for completeness' sake. There's some funniness going on with the indentation here as well. @@ +158,3 @@ +public void peas_register_types (GLib.TypeModule module) { + var objmodule = module as Peas.ObjectModule; + objmodule.register_extension_type (typeof(Peas.Activatable), typeof(RotationPlugin)); Missing spaces before the opening parentheses in the typeof invocations.
(In reply to comment #9) > Review of attachment 191250 [details] [review]: > > ::: src/plugins/rotation/Makefile.am > @@ +7,3 @@ > + > +# here we are explicitly specifying gtk+-3.0 to use the vapi because vala > still > +# cannot parse the gir > > Would be useful to add a bug reference to this comment. I could not find a matching bug. All other Totem plugins written in Vala contain the same comment. ;-) > > @@ +14,3 @@ > +librotation_la_SOURCES = totem-rotation-plugin.vala > +librotation_la_LDFLAGS = $(plugin_ldflags) > +librotation_la_CFLAGS = $(plugin_cflags) -include $(CONFIG_HEADER) > > What's the “-include $(CONFIG_HEADER)” needed for? > It defines GETTEXT_PACKAGE so that the two UI strings can be translated. > > ::: src/plugins/rotation/totem-rotation-plugin.vala > @@ +41,3 @@ > + _180 = 2, > + _90L = 3, > + _COUNT = 4 > > Why are these all prefixed by underscores? > Vala does not allow me to create enums that start with a digit. > > I think it would be clearer to explicitly include “this.” in statements > referring to class variables. Otherwise it's hard to work out their scope. > I added all the "this" to make you happy... it's such a simple class. > I fixed everything else as requested.
Created attachment 191258 [details] [review] name clutter actors
Created attachment 191259 [details] [review] rotation plugin (updated)
Review of attachment 191259 [details] [review]: Thanks for making the changes. Just a few more missing “this.”s that I spotted (might as well get them correct for consistency's sake) — but don't bother coming up with a new patch for the changes; just make them when it gets committed. Same for the Makefile.am change. ::: src/plugins/rotation/Makefile.am @@ +10,3 @@ +VALAFLAGS = \ + --girdir=$(top_srcdir)/src \ + --pkg Totem-1.0 --pkg Peas-1.0 --pkg GtkClutter-1.0 --pkg gtk+-3.0 I've just done a little experimentation, and (at least with the version of Vala I'm using — 0.13.0.36-69be9) valac is quite happy for the GTK+ dependency to be listed as “--pkg Gtk-3.0” which I believe should use the GIR rather than the VAPI file. Consequently, I think that making that change and removing the comment should be OK (hopefully). ::: src/plugins/rotation/totem-rotation-plugin.vala @@ +67,3 @@ + + var rotate_right = new Gtk.Action ("rotate-right", _("_Rotate Clockwise"), null, null); + rotate_right.activate.connect (cb_rotate_right); “this.” missing here (and three times below)! @@ +137,3 @@ + this.width = box.x2 - box.x1; + this.height = box.y2 - box.y1; + update_video_geometry (); “this.” missing here and in the methods below.
(In reply to comment #10) > (In reply to comment #9) > > Review of attachment 191250 [details] [review] [details]: > > > > ::: src/plugins/rotation/Makefile.am > > @@ +7,3 @@ > > + > > +# here we are explicitly specifying gtk+-3.0 to use the vapi because vala > > still > > +# cannot parse the gir > > > > Would be useful to add a bug reference to this comment. > > I could not find a matching bug. All other Totem plugins written in Vala > contain the same comment. ;-) See comment #13. > > > > @@ +14,3 @@ > > +librotation_la_SOURCES = totem-rotation-plugin.vala > > +librotation_la_LDFLAGS = $(plugin_ldflags) > > +librotation_la_CFLAGS = $(plugin_cflags) -include $(CONFIG_HEADER) > > > > What's the “-include $(CONFIG_HEADER)” needed for? > > > > It defines GETTEXT_PACKAGE so that the two UI strings can be translated. I should've known that. Thanks for clarifying. > > > > ::: src/plugins/rotation/totem-rotation-plugin.vala > > @@ +41,3 @@ > > + _180 = 2, > > + _90L = 3, > > + _COUNT = 4 > > > > Why are these all prefixed by underscores? > > > > Vala does not allow me to create enums that start with a digit. Fair enough. I don't think I can come up with anything better. :-) > > > > I think it would be clearer to explicitly include “this.” in statements > > referring to class variables. Otherwise it's hard to work out their scope. > > > > I added all the "this" to make you happy... it's such a simple class. :-) > > > > I fixed everything else as requested. Great, thanks. I guess the only other thing which has been mentioned in this bug is saving the rotation for each video which has a non-0 rotation using the GIO file attributes stuff which is also used to save the current position in a file (cf. totem_save_position() and totem_try_restore_position() in totem-uri.c). It would be quite nice if the rotation plugin could do this as well, but I guess it could be added later if you don't feel like doing it now.
You will have to wait for the additional patch as I will go on holidays for the next three weeks.
I had a lot of trouble getting this to even compile in a jhbuild environment, not sure how you managed to get ti to compile. In any case, the only problems I can see are: - the rotation isn't reset when switching to another file - the "Rotate" menu items should be just below "switch angles" - the menu items should be disabled when no video is loaded (eg. when the logo is shown)
and add the vala file to po/POTFILES.in
Created attachment 192944 [details] [review] rotation plugin (second update) I compiled my initial patch using the stable GNOME 3.0 packages from Debian experimental. Since the last update of clutter (1.6.16) Vala fails to parse the clutter gir file and I had to switch to the vapi file for clutter too. In addition I made the following changes: - require clutter-gtk >= 1.0.2 - reset rotation after video change - position buttons below "switch angles" - disable buttons if no video is loaded - load/store state as a GIO file attribute - add files to po/POTFILES.in
Looks great. Feel free to push when you've made store_state() and try_restore_state() async (so as not to block for remote locations, try streaming one of the videos here: http://dir.xiph.org/by_format/Ogg_Theora and see whether it works as expected).
Comment on attachment 192944 [details] [review] rotation plugin (second update) Committed as is to get into the released tarball. Leaving opened for you to fix the bug calls mentioned above.
Created attachment 193330 [details] [review] rotation-plugin: store/restore the state async Thanks for committing. The attached patch makes store_state and try_restore_state async.
Review of attachment 193330 [details] [review]: Looks good to me. Please commit.
Attachment 193330 [details] pushed as 99db20b - rotation-plugin: store/restore the state async
*** Bug 701950 has been marked as a duplicate of this bug. ***