Closed Bug 840745 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Video/Audio Controls, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
relnote-firefox --- 22+

People

(Reporter: ml, Assigned: darkowlzz)

References

(Depends on 1 open bug)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=js])

Attachments

(1 file, 5 obsolete files)

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.
Severity: normal → enhancement
Component: Untriaged → Video/Audio
OS: Linux → All
Product: Firefox → Core
Hardware: x86 → All
Version: 21 Branch → Trunk
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][mentor=jaws][lang=js]
Hi,
I would like to work on this bug.
Will post a patch shortly.
Assignee: nobody → indiasuny000
Status: NEW → ASSIGNED
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.
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. :)
(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 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! :)
(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. :)
* 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?
Attachment #714889 - Flags: review?(jAwS)
(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 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.
Attachment #714889 - Flags: review?(jAwS) → feedback?(paul)
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.
Attachment #714889 - Flags: feedback?(paul)
So what can we do now?
Wait or implement |accesskey|(s) and tests?
(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.
Added tests in test_contextmenu.html.
Attachment #714889 - Attachment is obsolete: true
Attachment #715772 - Flags: feedback?(jAwS)
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 :)
Attachment #715772 - Flags: feedback?(jAwS) → feedback+
Made the suggested changes.
Attachment #715772 - Attachment is obsolete: true
Attachment #718540 - Flags: feedback?(jAwS)
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.
Attachment #718540 - Flags: ui-review?(limi)
Attachment #718540 - Flags: feedback-
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.
Attachment #718540 - Flags: feedback?(jAwS) → feedback+
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. (&times; in HTML, × in Unicode) — if it's hard to do this, it's not *that* important.
Attachment #718540 - Flags: ui-review?(limi) → ui-review+
1. Fixed menuitem |checked| when playbackRate is changed using js.
2. Implemented × instead of x in menuitem label.

Tested. Seems to work properly.
Attachment #718540 - Attachment is obsolete: true
Attachment #721116 - Flags: review?(jAwS)
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.
Attachment #721116 - Flags: review?(jAwS) → feedback+
Made the suggested changes.
Attachment #721116 - Attachment is obsolete: true
Attachment #722273 - Flags: review?(jAwS)
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.
Attachment #722273 - Flags: review?(jAwS) → review+
Attachment #714876 - Attachment is obsolete: true
Keywords: checkin-needed
Already checked-in :) (see comment #27)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1b54bc328c1d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
relnote-firefox: --- → ?
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.
QA Contact: mihaela.velimiroviciu
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 release-mgmt@mozilla.com asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/
Depends on: 883186
You need to log in before you can comment on or make changes to this bug.