GNOME Bugzilla – Bug 417141
Enable variable speed playback
Last modified: 2016-06-01 16:12:22 UTC
Please describe the problem: Both mplayer and xine offer to play a video with variable speed (read: for example 1.5 speed). At least mplayer also plays sound when doing this. I played a bit with the gstreamer python bindings and with the playbin, it was quite easy to modifiy one of the provided examples, to enable exactly this. Basicly I did a seek with the desired playback speed. It would be very nice, if this would be enabled in totem. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
*** Bug 506526 has been marked as a duplicate of this bug. ***
xine-lib allows any speed between 0.25 and 4.0 using: xine_set_param (stream, XINE_FINE_SPEED_NORMAL, value * XINE_FINE_SPEED_NORMAL); For GStreamer, it's just like seeking, but the GstEvent is created with: gst_event_new_seek (rate, GST_FORMAT_TIME, flags, GST_SEEK_TYPE_SET, position, GST_SEEK_TYPE_SET, -1); As for the interface itself, I'm thinking of: - using page up/down to increase lower the speed - rate would increase/decrease when pressed longer (about 2 seconds?) - rate is reset when button is released - only works when mode works (can it fail for GStreamer?) - only works when already playing Would need a little change to the xine-lib backend: - return (xine_get_status (bvw->priv->stream) == XINE_STATUS_PLAY && xine_get_param (bvw->priv->stream, XINE_PARAM_SPEED) == XINE_SPEED_NORMAL); + return (xine_get_status (bvw->priv->stream) == XINE_STATUS_PLAY && xine_get_param (bvw->priv->stream, XINE_PARAM_SPEED) != XINE_SPEED_PAUSED);
Bastien Nocera wrote: - rate is reset when button is released actually, for the usecase I would have used the faster playback, this would have been counter productive. The situation was, that I had several lecture videos available, that I wanted to review prior to my exams. As I new most of the content already I wanted to get through them faster. I could have jumped through the whole video, but I wanted to see, whether I missed something - for this the "double speed method" worked quite well.
Also note that the "pitch" plugin should probably have been inserted for GStreamer, to avoid an helium effect.
*** Bug 113723 has been marked as a duplicate of this bug. ***
*** Bug 566359 has been marked as a duplicate of this bug. ***
*** Bug 566360 has been marked as a duplicate of this bug. ***
*** Bug 606576 has been marked as a duplicate of this bug. ***
I support this request too.
Created attachment 163549 [details] [review] test patch for enabling variable playback speed
I am new to gnome/totem development, this is a first attempt at a patch for variable speed playback. The patch implements the following: - using page up/down to increase lower the speed - Currently, the playback rate is increased with a steady step with consecutive keystrokes - not when key is pressed longer - rate is reset when seeking with right, left - only works when already playing comments and suggestions for changes / improvement are welcome
Does this bug report cover my problem, shown here: https://bugs.launchpad.net/ubuntu/+source/totem/+bug/593209 ???
Created attachment 164862 [details] [review] patch for enabling variable seek step (skip-duration) this patch implements 2 bars under Preferences->General->Playback, enabling the user to adjust forward and backward skip-durations (seekstep). This attempts to solve https://bugs.launchpad.net/ubuntu/+source/totem/+bug/593209 This patch is orthogonal to my first patch in this bug entry (for variable speed playback)
Comment on attachment 164862 [details] [review] patch for enabling variable seek step (skip-duration) I don't want to see options like that in Totem. FWIW, use Shift+Left Arrow if you want to skip back by 5 seconds.
Bastien, why wouldn't you want to have variable speed playback available in Totem? I can't see any good reason not to allow this. I would like you to explain why you'd reject a perfectly good contribution by a community member. You should be happy that someone wants to help with this project and work with everyone else.
(In reply to comment #15) > Bastien, why wouldn't you want to have variable speed playback available in > Totem? I can't see any good reason not to allow this. > > I would like you to explain why you'd reject a perfectly good contribution by a > community member. You should be happy that someone wants to help with this > project and work with everyone else. I think you need to learn to read better... I rejected a patch to make the arrow keys skip time configurable.
Sorry, you are right. I saw the other patches come through and really am looking forward to seeing the variable speed playback. I have been looking for this one for a long time. Again, I apologize.
Following up on this. The variable-speed playback patch doesn't seem to have received any review. I checked, and the patch still seems to apply albeit somewhat noisily. Any chance of seeing this patch go into Totem? If not, what does the patch need to do to go in?
(In reply to comment #18) > Following up on this. The variable-speed playback patch doesn't seem to have > received any review. I checked, and the patch still seems to apply albeit > somewhat noisily. Any chance of seeing this patch go into Totem? If not, what > does the patch need to do to go in? There's one rejected patch, and one test patch. I'll review the test patch now, but I can already tell you it won't be merged as-is.
Review of attachment 163549 [details] [review]: This patch also doesn't mention what should be done in terms of UI (small research shows that the iPod app on iOS only allows faster playback speed for audiobooks). The patch doesn't seem to handle pitch changes to make faster playback not sound like people inhaling helium. ::: src/backend/bacon-video-widget-gst-0.10.c @@ +3884,3 @@ +gboolean +bacon_video_widget_change_rate (BaconVideoWidget *bvw, gdouble rate) The video widget has knowledge of the rate, so it needs a get/set and an "increase" method. That way we can implement menu items (with a maximum and a minimum limit), as well as keyboard shortcuts (which would use an offset). ::: src/totem-object.c @@ +146,2 @@ static int totem_table_signals[LAST_SIGNAL] = { 0 }; +static gdouble playback_rate = 1.0; Keeping the playback rate inside Totem is wrong. As soon as there is a new item to play back, a seek that's not done through the keyboard, or an error, the rate will be reset.
(In reply to comment #20) > This patch also doesn't mention what should be done in terms of UI I'd suggest a new submenu in the View and context menus, called either "Playback Rate" or "Time Stretch", with menu entries for playing faster, playing slower, and resetting the playback rate to normal speed. (Those entries could go in the top-level menu like the zoom options, but they seem sufficiently infrequent that it doesn't seem worth having top-level menu entries for them.) For keyboard controls, which seem the most useful here, I'd suggest using [ and ] for slower and faster by increments of 10%, as well as supporting dedicated fast-forward and rewind keys as equivalent to the buttons described above. Also, I'd suggest doing 10% increments on an absolute scale, meaning 110% 120% 130% 140% rather than 110% 121% 133% 146% as mplayer does. Optionally, totem could add fast-forward ">>" and rewind "<<" controls near the play ">" button; fast-forward would advance to 2x, 3x, 4x, and rewind would go to -1x, -2x. Dedicated multimedia keys for fast-forward and rewind should trigger these as well. For rewind and fast-forward beyond a certain speed (2x or so), you probably want to disable audio. I see these as optional, though, and possibly a different feature request entirely; they have a different use case than time stretch or slow motion.
(In reply to comment #21) > I'd suggest a new submenu in the View and context menus, called either > "Playback Rate" or "Time Stretch", with menu entries for playing faster, > playing slower, and resetting the playback rate to normal speed. (Those > entries could go in the top-level menu like the zoom options, but they seem > sufficiently infrequent that it doesn't seem worth having top-level menu > entries for them.) Optionally, if this has its own submenu, it could have a set of menu entries similar to a zoom menu, for various common increments: 0.1x, 0.5x, normal speed, 1.2x, 1.5x, 2x. I still suspect this will most commonly get used via keyboard shortcuts via the menu, but nonetheless the menu ought to exist for discoverability, and once it exists it might as well support the common cases.
Also, the playback rate should appear in the UI near the time indicator and playback status ("Playing", "Paused"), as a multiplier: 1.1x, 1.2x, 0.1x, and so on. I don't know whether to hide it when "1x" to unclutter the interface, or show it when "1x" to make the presence of playback rate control more discoverable.
Created attachment 209391 [details] [review] patch for variable speed playback v2 v2 of variable speed playback: - rate increases /decreases with PgUp, PgDown respectively - The playback rate is changed with a steady step with consecutive keystrokes (not when key is pressed longer) - rate is reset to normal with Backslash - rate is reset when seeking with right, left, or with opening new file - 3 menuitems added under View menu for increase / decrease / reset - audio_pitch element inserted in pipeline to avoid pitch distortion - Implementation includes bvw_{set_rate, get_rate, change_rate, reset_rate}
Looks very promising. One minor issue: resetting the playback rate when seeking will prove annoying for the common use case of watching an entire video with a slight speedup applied.
yes, I have a sticky/permanent playback rate version of the patch as well (it's trivial to change) but I sent this one. Perhaps other use cases would prefer it to reset on a seek? If both behaviours are really useful, maybe a key/menu-item that toggles between sticky and not-sticky playback rate would be the best compromise.
(In reply to comment #26) > yes, I have a sticky/permanent playback rate version of the patch as well (it's > trivial to change) but I sent this one. Perhaps other use cases would prefer it > to reset on a seek? If both behaviours are really useful, maybe a key/menu-item > that toggles between sticky and not-sticky playback rate would be the best > compromise. I don't think a toggle makes sense. I'd suggest that two different use cases exist, and you can easily distinguish them by how someone invoked playback rate changes: - Fast forwarding to find a desired location. If the user started out by hitting fast-forward, then hitting play, seeking, or pretty much anything else should cancel the fast-forward and return to normal speed. - Using time compression to watch part or all a video faster (or slower). If the user started out by adjusting time stretch (with keys or menu items), then that time stretch should remain until explicitly changed/reset by the user. For example, the user might have set a slow speed to watch something carefully in slow-motion, and want to seek backward or skip ahead to find the right starting point, without going back to normal speed. And for the reverse case, a user might have set a 1.4x playback speed to watch something more quickly, and doesn't want that reset when skipping ahead.
Review of attachment 209391 [details] [review]: The changes to the .h files are missing (declarations of public functions). I would also prefer if 2 separate patches were made, one for the video widget, one for totem itself. ::: src/backend/bacon-video-widget-gst-0.10.c @@ +7161,3 @@ +gfloat +bvw_get_rate (BaconVideoWidget *bvw) bacon_video_widget, not bvw ::: src/totem-object.c @@ +3733,3 @@ case GDK_KP_5: bacon_video_widget_dvd_event (totem->bvw, BVW_DVD_ROOT_MENU_SELECT); There's a missing "break;" after the GDK_KP_5 case.
As Josh mentioned, there's 2 different use cases for using accelerated video. Let's focus on getting the 2nd one right, as it's the one that people have been requesting, and using "fast forward" would require more UI changes. Vasilis, can you make the changes requested?
yes, I 'll send 2 updated patches soon. minor question: Since the timestretch approach is to be implemented at the moment, are the names "playback_rate increase/decrease/reset" for the 3 new menu items appropriate, or should they be renamed to "timecompress/timestretch/timereset" instead? Also I forgot to mention I am developing/testing against gnome2_30 branch, let me know if that will do or if should send a forward ported patch to master/some other branch
(In reply to comment #30) > yes, I 'll send 2 updated patches soon. > > minor question: Since the timestretch approach is to be implemented at the > moment, are the names "playback_rate increase/decrease/reset" for the 3 new > menu items appropriate, or should they be renamed to > "timecompress/timestretch/timereset" instead? Note certain about the names, but the first alternative sounds better to me. I'll make sure to update your patches if we change our minds :) > Also I forgot to mention I am developing/testing against gnome2_30 branch, let > me know if that will do or if should send a forward ported patch to master/some > other branch master would be best, 2.30 is about 2 years old...
Created attachment 209793 [details] [review] bvw patch for variable speed playback v3
Created attachment 209794 [details] [review] totem patch for variable speed playback v3 v2->v3: - playback rate is persistent with seeking - split in 2 patches for bvw, totem - minor renaming (new public bvw* functions renamed to bacon_video_widget*) - issue: Playback rate will still be reset to normal, if the bvw_set_playback_direction actualy proceeds to change the direction i.e. when it is called with forward != (bvw->priv->rate > 0). Afaik this happens only with the Comma key. So, if the user has changed the playback rate but tries to step in the reverse direction, the playback rate will be reset. Perhaps another approach would be to have bvw_set_playback_direction negate the current rate instead of hardcode to FORWARD_RATE, REVERSE_RATE. Btw, is the Comma key working correctly in master? I 've only tested with gnome-2.30 or totem-3.0.1, but the pipeline seems to freeze often when sending the new seek event from bvw_set_playback_direction. This happens independently of this patch series.
(In reply to comment #33) > - issue: Playback rate will still be reset to normal, if the > bvw_set_playback_direction actualy proceeds to change the direction i.e. when > it is called with forward != (bvw->priv->rate > 0). Afaik this happens only > with the Comma key. So, if the user has changed the playback rate but tries > to > step in the reverse direction, the playback rate will be reset. > > Perhaps another approach would be to have bvw_set_playback_direction negate > the current rate instead of hardcode to FORWARD_RATE, REVERSE_RATE. That sounds quite sensible. If playing at 1.3x speedup, or 0.1x slow motion, it seems reasonable to reverse at the same rate. - Josh Triplett
Works nicely, but it would be best if the pitch element was available in gst-plugins-good instead of -bad. See bug 672147.
Review of attachment 209793 [details] [review]: Looks good. ::: src/backend/bacon-video-widget-gst-0.10.c @@ +6347,3 @@ +bacon_video_widget_reset_rate(BaconVideoWidget *bvw) +{ + return bacon_video_widget_set_rate(bvw, 1.0); Missing a space between function name and "(". ::: src/backend/bacon-video-widget.h @@ +256,3 @@ +gboolean bacon_video_widget_reset_rate (BaconVideoWidget *bvw); + +gfloat bacon_video_widget_get_rate (BaconVideoWidget *bvw); Remove the unneeded line feeds between the declarations.
(In reply to comment #35) > Works nicely, but it would be best if the pitch element was available in > gst-plugins-good instead of -bad. See bug 672147. Does this represent a blocker for merging this patch? Or, for now, would it suffice to just not do pitch correction if the pitch element doesn't exist? Or, alternatively, to treat it the same as a missing codec and poke packagekit to install it?
Comment on attachment 209793 [details] [review] bvw patch for variable speed playback v3 Patch for the backend is committed to master
*** Bug 549028 has been marked as a duplicate of this bug. ***
What's the current status of this bug? The last update occurred about 18 months ago. It sounds like the backend patch has been merged; what's the status of the frontend patch? Is the most recent version acceptable, or does some additional change need to occur?
Josh, sorry, the problem with this was always how to implement the UI. I've finally had time last cycle to re-do the totem UI (as you might have seen), and we finally have a place to put this functionality in. It will go in the "..." menu in the popup bar. We'll add 0.5, 0.8, 1, 1.2, 1.5, 2.x speeds. Also, bug 728020 contains code to use the new filter APIs in playbin, so that we can avoid offering this feature when using passthrough output.
Review of attachment 209794 [details] [review]: As mentioned in the bug, this won't be committed as-is.
(In reply to comment #41) > Josh, sorry, the problem with this was always how to implement the UI. I've > finally had time last cycle to re-do the totem UI (as you might have seen), and > we finally have a place to put this functionality in. > > It will go in the "..." menu in the popup bar. We'll add 0.5, 0.8, 1, 1.2, 1.5, > 2.x speeds. Could you please provide a keyboard shortcut as well, to incrementally increase/decrease the speedup (I'd suggest by increments of .1)? A quick check suggests that nothing is bound to '[' or ']', which are the keys mplayer uses for that functionality.
It's not implemented in current version of totem. I checked the new design of totem on https://wiki.gnome.org/Design/Apps/Videos but still lacks these basic functionalities of variable playback speed or subtitle/audio sync. I want to add this functionality. Being a newbie do help me implementing these functionalities to totem/videos.
(In reply to Bastien Nocera from comment #41) > Josh, sorry, the problem with this was always how to implement the UI. I've > finally had time last cycle to re-do the totem UI (as you might have seen), > and we finally have a place to put this functionality in. > > It will go in the "..." menu in the popup bar. We'll add 0.5, 0.8, 1, 1.2, > 1.5, 2.x speeds. Just following up; are there any blockers left that prevent implementing this?
It's missing UI design.
https://github.com/gnome-design-team/gnome-mockups/blob/aa99f912123fdf079d1aeaa9243935bb7fc3f9fa/videos/wireframes/png/playback-speed.png
Jakub, those designs don't allow for more fine-grained controls. I typically use VLC's [ and ] keyboard shortcuts to adjust the speed (in increments of 10%). I most often use 110% to speed through movies almost imperceptibly faster.
(In reply to Hashem Nasarat from comment #48) > Jakub, those designs don't allow for more fine-grained controls. I typically > use VLC's [ and ] keyboard shortcuts to adjust the speed (in increments of > 10%). > > I most often use 110% to speed through movies almost imperceptibly faster. And adding a few more speeds won't necessarily solve the general problem of keeping the UI in sync with keyboard shortcuts that can adjust numerically. This seems like the equivalent of zoom in a PDF viewer: while the menu should offer options like "50%", "Actual Size (100%)", "125%", "150%", "Page Width", and so on, the user also needs the ability to zoom arbitrarily with Ctrl-plus and Ctrl-minus, and have that reflected in the UI. Similarly, while it makes sense to drive this through the UI for common cases (just as YouTube offers a few preset speed options), totem should have keyboard shortcuts (ideally '[' and ']' to match mplayer and VLC) to adjust by 10% (0.1) down or up, to any arbitrary value. I think this requires only minimal changes to the current UI: add menu entries in the "speed" menu for "Speed up 10%" and "Slow down 10%", with the corresponding keyboard shortcuts ']' and '[' indicated. That tells people what keyboard shortcuts to use, and lets them adjust speed via the UI or the keyboard. The entry in the '...' menu that looks like "Speed: Normal" in the mockups can show the current speed. The menu *doesn't* need to provide UI to set the speed arbitrarily, though adding a bit more granularity near Normal seems potentially useful.
The speeds are just examples... I'm fine with adding only a few speeds, after we've checking which ones will be the most used. In any case, there's no point in bikeshedding over the speeds when nobody has written the UI code yet, so if somebody wants to take that up[1]. [1]: My current development machine has no sound support, so don't expect me to work on this before the GNOME 3.20 release.
Created attachment 323884 [details] [review] main: Make playback rate available to plugins
Created attachment 323885 [details] [review] variable-rate: Add menu items to change playback rate
Created attachment 323886 [details] [review] backend: Fix error reporting when setting rate fails And change the limits. We want to allow 0.5 to 2.0 rates, inclusive.
Comment on attachment 323885 [details] [review] variable-rate: Add menu items to change playback rate This is a WIP patch. I didn't add any keyboard shortcuts, because I don't know what those would be yet. Also, it seems not to work. But that's a first pass...
Thanks for working on this! Unless there's a strong reason to do otherwise, I'd suggest following the convention from mpv/mplayer: '[' and ']' to decrease and increase playback rate y 10%, respectively. (Unlike mpv, I'd suggest a flat 0.1x increase each time, rather than increasing the current rate by 10%.) You also need a "reset to 1x" key; mpv uses backspace for that, but I don't know if that conflicts with anything else in totem.
Review of attachment 323884 [details] [review]: ::: src/totem-object.c @@ +2076,3 @@ +totem_object_get_rate (TotemObject *totem) +{ + return bacon_video_widget_get_rate (totem->bvw); Could do with a g_return_val_if_fail (TOTEM_IS_OBJECT (totem), 0.0); @@ +2084,3 @@ + * @rate: the new absolute playback rate + * + * Sets the playback rate, with <code class="literal">1.0</code> being the normal playback rate. The new gtk-doc style is to use Markdown, so use `1.0` (backticks) instead of the DocBook. @@ +2091,3 @@ +totem_object_set_rate (TotemObject *totem, float rate) +{ + return bacon_video_widget_set_rate (totem->bvw, rate); Could do with a g_return_val_if_fail (TOTEM_IS_OBJECT (totem), FALSE); g_return_val_if_fail (rate >= 0.0, FALSE);
Review of attachment 323885 [details] [review]: ::: src/plugins/variable-rate/totem-variable-rate-plugin.c @@ +1,3 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ +/* + * Copyright (C) 2007 Philip Withnall <philip@tecnocode.co.uk> Oh really? @@ +61,3 @@ + const char *id; +} rates[NUM_RATES] = { + { 0.25, NC_("playback rate", "0.25"), "025" }, Could you use ‘_’ in the ID instead of ‘.’? Might make things a little clearer. @@ +70,3 @@ +TOTEM_PLUGIN_REGISTER(TOTEM_TYPE_VARIABLE_RATE_PLUGIN, TotemVariableRatePlugin, totem_variable_rate_plugin) + +#if 0 Why is this all commented out? @@ +158,3 @@ + for (i = 0; i < NUM_RATES; i++) + if (g_strcmp0 (rate_id, rates[i].id) == 0) + break; I’d add a g_assert (i < NUM_RATES); to catch dropping off the end. @@ +166,3 @@ + } + + g_message ("Managed to set rate to %f", rates[i].rate); g_debug(). @@ +199,3 @@ + target = 0; + else if (i < 0) + target = NUM_RATES; That’s the first element off the end of the array. @@ +216,3 @@ + TotemVariableRatePluginPrivate *priv = pi->priv; + +#if 0 Why is this commented out?
Review of attachment 323886 [details] [review]: LGTM.
(In reply to Philip Withnall from comment #56) > Review of attachment 323884 [details] [review] [review]: > > ::: src/totem-object.c > @@ +2076,3 @@ > +totem_object_get_rate (TotemObject *totem) > +{ > + return bacon_video_widget_get_rate (totem->bvw); > > Could do with a g_return_val_if_fail (TOTEM_IS_OBJECT (totem), 0.0); We don't have any such guards for other bits of the plugin API. > @@ +2084,3 @@ > + * @rate: the new absolute playback rate > + * > + * Sets the playback rate, with <code class="literal">1.0</code> being the > normal playback rate. > > The new gtk-doc style is to use Markdown, so use `1.0` (backticks) instead > of the DocBook. Noted, changed. > @@ +2091,3 @@ > +totem_object_set_rate (TotemObject *totem, float rate) > +{ > + return bacon_video_widget_set_rate (totem->bvw, rate); > > Could do with a > g_return_val_if_fail (TOTEM_IS_OBJECT (totem), FALSE); > g_return_val_if_fail (rate >= 0.0, FALSE); Same as above. If we were to add those guards, we'd do it for all the accessors.
(In reply to Bastien Nocera from comment #59) > (In reply to Philip Withnall from comment #56) > > Review of attachment 323884 [details] [review] [review] [review]: > > > > ::: src/totem-object.c > > @@ +2076,3 @@ > > +totem_object_get_rate (TotemObject *totem) > > +{ > > + return bacon_video_widget_get_rate (totem->bvw); > > > > Could do with a g_return_val_if_fail (TOTEM_IS_OBJECT (totem), 0.0); > > We don't have any such guards for other bits of the plugin API. I would be in favour of adding them (as a separate commit). Can’t do any harm.
(In reply to Philip Withnall from comment #57) > Review of attachment 323885 [details] [review] [review]: > > ::: src/plugins/variable-rate/totem-variable-rate-plugin.c > @@ +1,3 @@ > +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ > +/* > + * Copyright (C) 2007 Philip Withnall <philip@tecnocode.co.uk> > > Oh really? Right ;) > @@ +61,3 @@ > + const char *id; > +} rates[NUM_RATES] = { > + { 0.25, NC_("playback rate", "0.25"), "025" }, > > Could you use ‘_’ in the ID instead of ‘.’? Might make things a little > clearer. Yep. > @@ +70,3 @@ > +TOTEM_PLUGIN_REGISTER(TOTEM_TYPE_VARIABLE_RATE_PLUGIN, > TotemVariableRatePlugin, totem_variable_rate_plugin) > + > +#if 0 > > Why is this all commented out? Because it's unused yet. > @@ +158,3 @@ > + for (i = 0; i < NUM_RATES; i++) > + if (g_strcmp0 (rate_id, rates[i].id) == 0) > + break; > > I’d add a > g_assert (i < NUM_RATES); > to catch dropping off the end. Yep. > @@ +166,3 @@ > + } > + > + g_message ("Managed to set rate to %f", rates[i].rate); > > g_debug(). Yes, there are still debug messages in there, will leave them there until the patch is functional. > @@ +199,3 @@ > + target = 0; > + else if (i < 0) > + target = NUM_RATES; > > That’s the first element off the end of the array. Oopsie. > @@ +216,3 @@ > + TotemVariableRatePluginPrivate *priv = pi->priv; > + > +#if 0 > > Why is this commented out? Because that's not implemented yet, see comment 54. (In reply to Josh Triplett from comment #55) > Thanks for working on this! > > Unless there's a strong reason to do otherwise, I'd suggest following the > convention from mpv/mplayer: '[' and ']' to decrease and increase playback > rate y 10%, respectively. (Unlike mpv, I'd suggest a flat 0.1x increase > each time, rather than increasing the current rate by 10%.) You also need a > "reset to 1x" key; mpv uses backspace for that, but I don't know if that > conflicts with anything else in totem. I don't intend on making the rates free-form, rather, each step would be one of the steps that exists in the menu.
Created attachment 324483 [details] [review] backend: Fix error reporting when setting rate fails And change the limits. We want to allow 0.5 to 2.0 rates, inclusive.
Created attachment 324484 [details] [review] variable-rate: Add menu items to change playback rate
Comment on attachment 323884 [details] [review] main: Make playback rate available to plugins Attachment 323884 [details] pushed as c648819 - main: Make playback rate available to plugins With the changes mentioned in the review.
(In reply to Bastien Nocera from comment #61) > (In reply to Josh Triplett from comment #55) > > Thanks for working on this! > > > > Unless there's a strong reason to do otherwise, I'd suggest following the > > convention from mpv/mplayer: '[' and ']' to decrease and increase playback > > rate y 10%, respectively. (Unlike mpv, I'd suggest a flat 0.1x increase > > each time, rather than increasing the current rate by 10%.) You also need a > > "reset to 1x" key; mpv uses backspace for that, but I don't know if that > > conflicts with anything else in totem. > > I don't intend on making the rates free-form, rather, each step would be one > of the steps that exists in the menu. That seems fine, as long as the menu has sufficiently fine-grained increments. (Also, *thank you* for using the existing keys.) Based on the patch in comment 63, it looks like the menu increments are currently 0.5, 0.75, Normal, 1.5, and 2.0. This isn't quite fine-grained enough, especially close to 1.0. At a minimum, YouTube offers 1.25 in addition to 1.5 and 2.0; I'd also request a couple more "close to normal" speeds. I'd like to avoid bikeshedding as much as possible, so I'm not too concerned about the exact numbers, just approximate ranges. Assuming you don't want to offer every 0.1x increment between 1.0 and 2.0, then I'd suggest roughly the following four increments, with justifications for why I currently end up using each in mpv: 1.1 (Barely noticeable, but saves about 5.5 minutes per hour of video, or a little under a minute for a ten-minute video; that adds up.) 1.25 or 1.3 or 1.33 (Noticeably different, in that it tends to eliminate pauses in speech, but still quite watchable for something with low content density. I actually find this speed preferable for 1.25 saves 12 minutes per hour, or 2 minutes per ten-minute video; 1.33 saves 15 minutes per hour, or 2.5 minutes per ten-minute video. Personally, I prefer 1.3x or 1.33x, based on having used it on a wide range of content, but any of these would be approximately fine.) 1.5 or 1.6 (obtrusive but watchable; this is fast enough that it requires full attention, and too fast for understanding if the audio includes someone already speaking at a rapid clip, but still quite watchable for many videos.) 1.75 or 1.8 (About my upper bound for understandable speech; this is the highest level I'd play an entire video on.) Any higher level (e.g. 2.0) would be the equivalent of skimming/scrubbing, needing to slow down for more interesting content. The value of offering every 0.1x increment is that doing so makes it easier to "tune" the rate based on the video, slowing down a bit if it's going too fast for understanding, or speeding up a bit if possible. Bigger increments make that process less smooth. But if that's a problem from a usability perspective, I can live with a few well-placed increments, along the lines of the four above.
(In reply to Josh Triplett from comment #65) > 1.25 or 1.3 or 1.33 (Noticeably different, in that it tends to eliminate > pauses in speech, but still quite watchable for something with low content > density. I actually find this speed preferable for 1.25 saves 12 minutes Missed a few words in here: 'I actually find this speed preferable over "normal" for quite a bit of content.'
Review of attachment 324483 [details] [review]: LGTM.
Created attachment 328893 [details] [review] variable-rate: Add menu items to change playback rate
(In reply to Bastien Nocera from comment #68) > Created attachment 328893 [details] [review] [review] > variable-rate: Add menu items to change playback rate No submenus in this one, compared to attachment 324484 [details] [review], but GtkMenuButton is broken in a different way there (see bug 767108).
Created attachment 328899 [details] [review] variable-rate: Add menu items to change playback rate With the default values from: https://bugzilla.gnome.org/show_bug.cgi?id=417141#c65 If you think that the values aren't the right ones, write a new plugin. If you think it looks bad with the separator there, see https://bugzilla.gnome.org/show_bug.cgi?id=767108
Attachment 324483 [details] pushed as 36c0ddd - backend: Fix error reporting when setting rate fails Attachment 328899 [details] pushed as 38683c6 - variable-rate: Add menu items to change playback rate
Thank you very much! This is awesome; I really look forward to using it, and to making totem my default media player.