840745 - default html5 audio/video player controls should facilitate changing playback rate

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, enhancement)

Description

[Mike Linksvayer]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20130212 Firefox/21.0
Build ID: 20130212031120

Steps to reproduce:

Viewed video and audio embedded using the <video> and <audio> tags.


Actual results:

I had to view/listen at normal speed, or download and play in desktop player that allows me to playback faster.


Expected results:

I would like default controls (right click menu might be even better) to allow me to change playback speed, probably 0.25x/0.5x/normal/1.5x/2x.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

I agree, now that playbackRate is pretty stable, a control to change playbackRate would be a good thing to add to our built in video controls.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

We can do something for this with the video context menu.

For example, IE9 has a "Play Speed" submenu in their video context menus (I'm not sure what they do for audio): http://screencast.com/t/L5Hbhptk

This is their audio context menu: http://screencast.com/t/X9HGdJx9W

It's not clear to me why they chose to provide different playbackRate defaults for audio and video. We should use their simpler audio rates.

The context menu is updated through nsContextMenu.js in /browser/base/content. You'll also have to update the context menu test located at /browser/base/content/test/test_contextmenu.html

Comment 3

[Sunny [:darkowlzz]]

Hi,
I would like to work on this bug.
Will post a patch shortly.

Comment 4

[Sunny [:darkowlzz]]

Hi,

I went through the code and here are what I think should be done. 

Add |playbackRate| items for various playback rates in this function http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#388

Make the corresponding changes for new |menuitem|(s) as in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-context.inc#77 . |menuitem| has an attribute |oncommand|. Use |oncommand| to pass the rate to |setPlaybackRate| which resides http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#3551

Please let me know whether I am right or there is something else to be done.

Comment 5

[Mike Linksvayer]

Hi darkowlzz, I had started taking a look at this and stopped as soon as I saw your interest, but here's what I had learned anyway (I'm a complete newbie), which overlaps with the notes you posted above. Will also upload changes I had made so far, FWIW.

I think changing the playback rate is accomplished in javascript by adding cases to https://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1473 rather than in C++.

Adding to browser-context.inc requires adding to a DTD, which seemed to be https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd

Changes I attempted in the files above are probably suboptimal, but here are some things that I didn't even try to:

* figure out how tests are run (got as far as running all of mochitest-plain; impressive, but not even sure it ran the right tests)
* figure out how to add a submenu, if that is the right UI (looks like that's what IE does)
* figure out why changing the rate to 0.5 seems to not work (for me)

Hope this helps, but you may well be better off waiting for answers from someone who knows what they're doing. :)

Comment 6

[Mike Linksvayer]

Attachment: partial work mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=840745#c5

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to darkowlzz from comment #4)
This is mostly on the right track. 

Within the |oncommand|, you'll want to call |gContextMenu.mediaCommand('playbackRate', newRate);| where |newRate| is the playback rate that the user selected.

The mediaCommand function, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1473, will need to be updated to take a second parameter. You can name this second parameter something generic, like |data|. The function will need to be updated to include a case for 'playbackRate', and the code there will set playbackRate by calling |media.playbackRate = data;|.

You shouldn't need to make any changes to nsHTMLMediaElement.cpp.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 714876 [details] [diff] [review]
partial work mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=840745#c5

Review of attachment 714876 [details] [diff] [review]:
-----------------------------------------------------------------

This is in the right direction. Nice job for your first patch! :)

Comment 9

[Sunny [:darkowlzz]]

(In reply to Mike Linksvayer from comment #5)

Hi Mike,

I didn't notice |CM_mediaCommand| and had no idea about browser.dtd. Thanks a lot for pointing them out and for sharing your experiences/suggestions. :)

Comment 10

[Sunny [:darkowlzz]]

Attachment: Added playbackRate Menu with various playback rate options

* Created Playback Rates context-menu with submenu.
* 0.25x, 0.5x, 1.5x and 2.0x speed works, but when switching back to normal speed, it doesn't comes to perfect normal speed.

I have not assigned |assesskey|(s) yet. Would be great if you suggest some standard keys.

Normal Speed isn't working properly. It comes back to almost normal speed, but not the original normal speed. Any idea why?

Will add tests in the next patch. Can you tell you what are tests I should look for? Mike has worked on some tests in his patch. Are those the only ones?

Comment 11

[Mike Linksvayer]

(In reply to darkowlzz from comment #10)
> Created attachment 714889 [details] [diff] [review]

Tried, works great except for returning to normal (didn't work with my patch either, but I failed to notice).

0.25x probably shouldn't be enabled for <audio> as it seems to be muted automatically. https://developer.mozilla.org/en-US/docs/DOM/HTMLMediaElement says "Gecko mute the sound outside the range 0.25 and 5.0"; I guess that range doesn't include its bounds.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 714889 [details] [diff] [review]
Added playbackRate Menu with various playback rate options

Review of attachment 714889 [details] [diff] [review]:
-----------------------------------------------------------------

Paul, I tested with http://video.webmfiles.org/big-buck-bunny_trailer.webm and the web console and did the following:
> var v = document.body.firstElementChild;
> v.playbackRate = 0.5;
// Notice that the video now plays slower
> v.playbackRate = 1;
// Notice no change in the playback rate.

Do you know why setting playbackRate to 0.5 then back to 1 doesn't start playing the video at 1x speed? Is this a known bug? It seems to fix itself if I manually seek.

Comment 13

[Paul Adenot (:padenot)]

Jared, this is bug 825329. The fix is trivial, but we hit what is most probably a Windows PGO miscompilation, so I have to investigate a bit to make sure.

The spec requires the playbackRate to return to |defaultPlaybackRate| (which is 1.0 by default) on seek, so that is the fix you see.

I'll try to land that soon, sorry.

Comment 14

[Sunny [:darkowlzz]]

So what can we do now?
Wait or implement |accesskey|(s) and tests?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to darkowlzz from comment #14)
> So what can we do now?
> Wait or implement |accesskey|(s) and tests?

You can continue with adding tests. I'm not sure accesskeys are necessary here.

Comment 16

[Sunny [:darkowlzz]]

Attachment: Added playbackRate Menu with various playback rate options

Added tests in test_contextmenu.html.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 715772 [details] [diff] [review]
Added playbackRate Menu with various playback rate options

Review of attachment 715772 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-context.inc
@@ +69,5 @@
> +      <menu id="context-media-playbackrate" label="&mediaPlaybackRate.label;">
> +        <menupopup>
> +          <menuitem id="context-media-playbackrate025x"
> +                    label="&mediaPlaybackRate025x.label;"
> +                    oncommand="gContextMenu.mediaCommand('playbackRate', 0.25);"/>

These menuitems should have type="radio" and share a common name. See https://developer.mozilla.org/en-US/docs/XUL/Attribute/menuitem.name for more information.

@@ +75,5 @@
> +                    label="&mediaPlaybackRate050x.label;"
> +                    oncommand="gContextMenu.mediaCommand('playbackRate', 0.5);"/>
> +          <menuitem id="context-media-playbackrate100x"
> +                    label="&mediaPlaybackRate100x.label;"
> +                    oncommand="gContextMenu.mediaCommand('playbackRate', 1.0);"/>

There should be some code that initializes this menuitem to checked=true when the video controls are initialized (as long as the playbackRate is still equal to 1).

If a playback rate doesn't match one of the preset options, we should have no menuitems checked.

::: browser/base/content/nsContextMenu.js
@@ +1488,5 @@
>        case "unmute":
>          media.muted = false;
>          break;
> +      case "playbackRate":
> +        media.playbackRate = data;

This should also set checked=true on the relevant menuitem.

::: browser/base/content/test/test_contextmenu.html
@@ +374,5 @@
> +                              ["context-media-playbackrate025x", true,
> +                               "context-media-playbackrate050x", true,
> +                               "context-media-playbackrate100x", true,
> +                               "context-media-playbackrate150x", true,
> +                               "context-media-playbackrate200x", true], null,

I would prefer if these IDs were of the form "context-media-playbackrate-025x".

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +461,5 @@
> +<!ENTITY mediaPlaybackRate025x.label "0.25x speed">
> +<!ENTITY mediaPlaybackRate050x.label "0.5x speed">
> +<!ENTITY mediaPlaybackRate100x.label "normal speed">
> +<!ENTITY mediaPlaybackRate150x.label "1.5x speed">
> +<!ENTITY mediaPlaybackRate200x.label "2.0x speed">

Let's go with these for the strings so it is less technical and easier for users to approach:

mediaPlaybackRate.label     "Play Speed"
mediaPlaybackRate050x.label "Slow motion (0.5x)"
mediaPlaybackRate100x.label "Normal speed"
mediaPlaybackRate150x.label "High speed (1.5x)"
mediaPlaybackRate200x.label "Ludicrous speed (2x)"

I don't think 0.25x is useful enough in practice for our default video controls.

The mention of Ludicrous speed would be a nice Easter Egg and adds some whimsy to our video controls :)

Comment 18

[Sunny [:darkowlzz]]

Attachment: Added playbackRate Menu with various playback rate options

Made the suggested changes.

Comment 19

[Frank Yan (:fryn)]

Comment on attachment 718540 [details] [diff] [review]
Added playbackRate Menu with various playback rate options

Review of attachment 718540 [details] [diff] [review]:
-----------------------------------------------------------------

I preferred simply the previous labels. I think 0.5x is not overly technical, and I don't think Slow Motion, Ludicrous Speed, etc. are more easily understood. Just as we display the actual duration of the video/audio file, not some fuzzy word, e.g. brief, short, long, ludicrously long, we should simply display the number for the playback rate.
To avoid bike-shedding, I'll have UX make a final call on the following:
- format of the strings
- which playback rates to include
- how we present these to the user

(In reply to Jared Wein [:jaws] from comment #17)
> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +461,5 @@
> > +<!ENTITY mediaPlaybackRate025x.label "0.25x speed">
> > +<!ENTITY mediaPlaybackRate050x.label "0.5x speed">
> > +<!ENTITY mediaPlaybackRate100x.label "normal speed">
> > +<!ENTITY mediaPlaybackRate150x.label "1.5x speed">
> > +<!ENTITY mediaPlaybackRate200x.label "2.0x speed">
> 
> Let's go with these for the strings so it is less technical and easier for
> users to approach:
> 
> mediaPlaybackRate.label     "Play Speed"
> mediaPlaybackRate050x.label "Slow motion (0.5x)"
> mediaPlaybackRate100x.label "Normal speed"
> mediaPlaybackRate150x.label "High speed (1.5x)"
> mediaPlaybackRate200x.label "Ludicrous speed (2x)"
> 
> I don't think 0.25x is useful enough in practice for our default video
> controls.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 718540 [details] [diff] [review]
Added playbackRate Menu with various playback rate options

Review of attachment 718540 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +407,5 @@
>        this.setItemAttr("context-media-play",  "disabled", hasError);
>        this.setItemAttr("context-media-pause", "disabled", hasError);
>        this.setItemAttr("context-media-mute",   "disabled", hasError);
>        this.setItemAttr("context-media-unmute", "disabled", hasError);
> +      this.setItemAttr("context-media-playbackrate", "disabled", hasError);

This should also set the 'checked' attribute to true based on the current playbackRate.

To test this out, go to https://videos-cdn.mozilla.net/serv/Marketplace-videos/FirefoxMarketplace-airbnb-BR-RC-SD1%20640.webm and open the Web Console.

While the video is playing, run the following command:
> document.body.firstElementChild.playbackRate = 2

Then right-click on the video and look at the Play Speed menu. The 2x menuitem should be checked, but it currently shows the 1x being checked.

Comment 21

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 718540 [details] [diff] [review]
Added playbackRate Menu with various playback rate options

This looks fine to me. I'd recommend using the "multiplication" Unicode character instead of lowercase x if you want it to be correct. (× in HTML, × in Unicode) — if it's hard to do this, it's not *that* important.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

For clarification on the ui-review, http://www.pastebin.mozilla.org/2185019

Comment 23

[Sunny [:darkowlzz]]

Attachment: Added playbackRate Menu with various playback rate options

1. Fixed menuitem |checked| when playbackRate is changed using js.
2. Implemented × instead of x in menuitem label.

Tested. Seems to work properly.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 721116 [details] [diff] [review]
Added playbackRate Menu with various playback rate options

Review of attachment 721116 [details] [diff] [review]:
-----------------------------------------------------------------

This is very close, but there are a couple minor issues to fix first.

Previously I said not to worry about accesskeys, but we should add them now. You can use these accesskeys:
* "l" (lowercase L) as the accesskey for "context-media-playbackrate"
* "S" as the accesskey for "context-media-playbackrate-050x"
* "N" as the accesskey for "context-media-playbackrate-100x"
* "H" as the accesskey for "context-media-playbackrate-150x"
* "L" as the accesskey for "context-media-playbackrate-200x"

::: browser/base/content/nsContextMenu.js
@@ +408,5 @@
>        this.setItemAttr("context-media-pause", "disabled", hasError);
>        this.setItemAttr("context-media-mute",   "disabled", hasError);
>        this.setItemAttr("context-media-unmute", "disabled", hasError);
> +      this.setItemAttr("context-media-playbackrate", "disabled", hasError);
> +      this.setItemAttr("context-media-playbackrate-050x", "checked", this.target.playbackRate == 0.5);

This section of the code is used to disable menuitems, so setting the checked attribute here seems a little out of place.

Please move the setting of the checked attributes to the line above where "hasError" is defined.

You should also call this.setItemAttr("context-media-playbackrate-050x", "disabled", hasError); for each of the individual playbackRate menuitems.

Comment 25

[Sunny [:darkowlzz]]

Attachment: Added playbackRate Menu with various playback rate options

Made the suggested changes.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 722273 [details] [diff] [review]
Added playbackRate Menu with various playback rate options

Review of attachment 722273 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect! I'll check this in for you later today to mozilla-inbound. In a day or two mozilla-inbound will get merged to mozilla-central. The following day the changes will appear in the Nightly build of Firefox.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b54bc328c1d

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Already checked-in :) (see comment #27)

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b54bc328c1d

Comment 30

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:22.0) Gecko/20130403 Firefox/22.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130403 Firefox/22.0
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20130402 Firefox/22.0
BuildID: 20130402004013

Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130403 Firefox/23.0
Mozilla/5.0 (Windows NT 6.2; rv:23.0) Gecko/20130403 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) Gecko/20130404 Firefox/23.0
Build ID: 20130403090950

I verified that the the setting playback speed option is available in the context menu and video/audio plays with the selected speed.
Still, I won't mark it as verified until the dependencies (being unable to always change back to normal speed - bug 825329 and Playback speed changing to 1x when jumping to other time in the video/audio - bug 858025) are fixed.

Comment 31

[Alex Keybl [:akeybl]]

This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email [e-mail] asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

Comment 32

[(not reading bugmail) Nick Desaulniers [:\n]]

Backlink: http://www.reddit.com/r/funny/comments/2py8mk/firefox_has_a_ludicrous_easter_egg/