Last Comment Bug 762191 - Volume control in video files is broken
: Volume control in video files is broken
Status: RESOLVED FIXED
: regression
Product: Toolkit
Classification: Components
Component: Video/Audio Controls (show other bugs)
: 16 Branch
: All All
: -- normal (vote)
: mozilla16
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
http://video.webmfiles.org/big-buck-b...
Depends on: 772428
Blocks: 480376 749520
  Show dependency treegraph
 
Reported: 2012-06-06 12:26 PDT by Sid
Modified: 2012-07-10 07:07 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
unaffected
+
fixed
unaffected


Attachments
Patch for bug (1016 bytes, patch)
2012-06-11 21:42 PDT, Jared Wein [:jaws] (please needinfo? me)
cpearce: review-
Details | Diff | Review
Patch: Only setup video controls' volume control event listeners once we know that we have an audio stream. (8.62 KB, patch)
2012-06-13 15:30 PDT, Chris Pearce (:cpearce)
jaws: review+
Details | Diff | Review
Patch (370.02 KB, patch)
2012-06-20 21:19 PDT, Chris Pearce (:cpearce)
jaws: review+
Details | Diff | Review

Description Sid 2012-06-06 12:26:44 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:16.0) Gecko/16.0 Firefox/16.0a1
Build ID: 20120606030528

Steps to reproduce:

Open http://video.webmfiles.org/big-buck-bunny_trailer.webm and try to change volume level.


Actual results:

There's no way to do it. The volume control that usually appears on mouse hover is broken.


Expected results:

Should be able to control volume.
Comment 1 Sid 2012-06-06 13:19:28 PDT
In the previous Nightly build it still works, so the regression window would be:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=3edf11eed119&enddate=6338a8988917
Comment 2 XtC4UaLL [:xtc4uall] 2012-06-06 16:41:31 PDT
Window for Inbound:
Last good nightly: 2012-06-04
First bad nightly: 2012-06-05

Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=778ba119ded6&tochange=f56e2197d9cd

Btw, in Video Fullscreen it works.
Comment 3 Chris Pearce (:cpearce) 2012-06-11 17:06:21 PDT
The only likely culprit in that range is:

730209ed620b	Paul Adenot — Bug 749520 - Use new mozHasAudio API to let users know that the video being watched does not have an audio track r=jaws

We'd better fix this for FF16, or backout 730209ed620b if we don't get around to it.
Comment 4 Paul Adenot (:padenot) 2012-06-11 17:23:31 PDT
From the patch on bug 657791 (revision 730209ed620b):

 
> -                    if (!this.isAudioOnly) {
> +                    if (!this.isAudioOnly && this.video.mozHasAudio) {
>                       addListener(this.muteButton, "mouseover", this.onVolumeMouseInOut);

This code is wrong. |this.video.mozHasAudio| is set only if |readyState >= HAVE_METADATA|, but it is likely to be false when calling init() for the first time (that is when the video is loaded initially). |init()| is called again when going full screen and when going out of full screen, hence the observed result.

Patch incoming.
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-06-11 17:50:38 PDT
I think we should make .mozHasAudio be true until we find that the assumption was false.
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-06-11 21:42:43 PDT
Created attachment 632130 [details] [diff] [review]
Patch for bug

Tested with these two video files:
http://msuja.ws/2011-12-20_0309.theora.ogv (audio)
http://static.tumblr.com/mx0pgsj/Qjalii98e/dust-theme.ogv (no-audio)

I wasn't sure if the UUID needed to be updated or not.

We should default to assuming that there is audio available since it will be the majority case.
Comment 7 Chris Pearce (:cpearce) 2012-06-13 15:27:03 PDT
Comment on attachment 632130 [details] [diff] [review]
Patch for bug

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

I don't think we should report (or assume!) that a file has an audio stream before we know that it does. Indeed, I'd even go as far to suggest that we change GetMozHasAudio() to return NS_ERROR_DOM_INVALID_STATE_ERR (i.e. throw an exception in JS) when it's called when (readyState < HAVE_METADATA).

I have a counter proposal patch, I'll upload it.
Comment 8 Chris Pearce (:cpearce) 2012-06-13 15:30:33 PDT
Created attachment 632912 [details] [diff] [review]
Patch: Only setup video controls' volume control event listeners once we know that we have an audio stream.

So instead of assuming all files have audio and thus need a volume control, I propose we instead alter the video controls so that they only add the mouse event listeners that make the volume control pop up/usable once metadata is loaded, and thus once we're sure that HTMLMediaElement.mozHasAudio returns an accurate value.
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2012-06-14 14:18:15 PDT
Comment on attachment 632912 [details] [diff] [review]
Patch: Only setup video controls' volume control event listeners once we know that we have an audio stream.

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

r+ with the following changes made.

::: toolkit/content/widgets/videocontrols.xml
@@ +643,5 @@
>  
> +                // Helper function to add an event listener to the given element.
> +                addListener: function (elem, eventName, func) {
> +                    let self = this;
> +                    let boundFunc = func.bind(self);

Is the usage of |self| here necessary? Could |this| be passed to func.bind()?

@@ +653,5 @@
> +                    if (this.isAudioOnly) {
> +                        return;
> +                    }
> +                    if (!this.video.mozHasAudio) {
> +                        this.muteButton.setAttribute("noAudio", "true");

We should also set the |disabled| attribute on the button to help with a11y.

@@ +658,5 @@
> +                    } else {
> +                        this.addListener(this.muteButton, "mouseover", this.onVolumeMouseInOut);
> +                        this.addListener(this.muteButton, "mouseout", this.onVolumeMouseInOut);
> +                        this.addListener(this.volumeStack, "mouseover", this.onVolumeMouseInOut);
> +                        this.addListener(this.volumeStack, "mouseout", this.onVolumeMouseInOut);

We should only add the command event listener for the muteButton if the video has audio. Can you move this line: |this.addListener(this.muteButton, "command", this.toggleMute);| to here?
Comment 10 Chris Pearce (:cpearce) 2012-06-14 22:10:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcad61fcaf73

I accidentally labelled changeset as "bug 726191" (2,6 interposed), but that bug is access denied for me so I can't comment there to say so :(
Comment 11 Phil Ringnalda (:philor) 2012-06-14 22:57:51 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ffdfb75ce5 since you were timing out in test_videocontrols.html, so you can get it right when you reland with that fixed :)
Comment 12 Phil Ringnalda (:philor) 2012-06-14 23:12:31 PDT
And, once some finally finished, mochitests/content/a11y/accessible/actions/test_media.html
Comment 13 Chris Pearce (:cpearce) 2012-06-17 17:32:35 PDT
(In reply to Phil Ringnalda (:philor) from comment #11)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ffdfb75ce5 since
> you were timing out in test_videocontrols.html, so you can get it right when
> you reland with that fixed :)

The problem was test_videocontrols.html expects to be able to press the mute button, but the video in the test doesn't have an audio track, so after the review changes were made the the mute button is now disabled, and the test fails. I'll add an audio track to that media.

(In reply to Phil Ringnalda (:philor) from comment #12)
> And, once some finally finished,
> mochitests/content/a11y/accessible/actions/test_media.html

Oops, we also need to ensure the mute button has its command listener added when playing <audio> only in initVolumeControlListeners().

Now just have to wait for try to reopen so I can retest...
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-06-19 13:48:18 PDT
Have you been able to push this to tryserver yet?
Comment 15 Chris Pearce (:cpearce) 2012-06-19 21:03:11 PDT
(In reply to Jared Wein [:jaws] from comment #14)
> Have you been able to push this to tryserver yet?

Yes, there was a race condition in a11y's test_media.html. Turns out that test is pushing the mute button before the controls have added their listeners (in the loadedmetadata handler).

This exposes the larger issue; should we enable the controls to change the volume or mute state before metadata is loaded? After thinking about it, I think we should allow this. A user may decide they want to mute a video before it's loaded metadata, particularly if they're on a slow network.

So I think we should change approach. Instead I think we should allow the volume and mute buttons to work before metadata is loaded, and only disable them once readyState >= HAVE_METADATA && !mozHasAudio. Easiest way to do this is to have onVolumeMouseInOut check that condition and exit if it's true.

Testing this approach now:
https://tbpl.mozilla.org/?tree=Try&rev=9b7a206c195f
Comment 16 Jared Wein [:jaws] (please needinfo? me) 2012-06-19 22:29:14 PDT
I'm pretty sure the tracking flag got accidentally unset there.
Comment 17 Chris Pearce (:cpearce) 2012-06-20 15:44:12 PDT
Oops, it totally did. We should still track this.
Comment 18 Chris Pearce (:cpearce) 2012-06-20 21:19:20 PDT
Created attachment 635182 [details] [diff] [review]
Patch

Bail out in volume/mute control mouse listeners, rather than preventing the listeners from being added. This means the user can use the controls before metadata has loaded.

Also adds an audio track to the video uses in test_volumecontrols.html, as that test presses the mute button, which this patch disabled when there's no audio track.

Green: https://tbpl.mozilla.org/?tree=Try&rev=bd26901b2de9
Comment 19 Jared Wein [:jaws] (please needinfo? me) 2012-06-21 13:43:11 PDT
Comment on attachment 635182 [details] [diff] [review]
Patch

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

::: toolkit/content/tests/widgets/Makefile.in
@@ +41,5 @@
>  		videocontrols_direction-2e.html \
>  		videocontrols_direction_test.js \
>  		use_large_cache.js \
> +		$(topsrcdir)/content/media/test/audio.wav \
> +		$(topsrcdir)/content/media/test/seek_with_sound.ogg \

How common is this pattern? Would it be easier to just have these files located in the same directory?
Comment 20 Chris Pearce (:cpearce) 2012-06-21 14:34:27 PDT
It's used in a few other tests: /accessible/tests/mochitest/{actions,tree}/test_media.html and content/html/document/test/test_document-element-inserted.html

It saves space, and centralizes our test media.
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2012-06-21 15:12:47 PDT
Comment on attachment 635182 [details] [diff] [review]
Patch

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

Can we also keep a test case that has a video with no audio track?
Comment 22 Jared Wein [:jaws] (please needinfo? me) 2012-06-26 01:21:32 PDT
Chris, what do you think about keeping a test case with a video that lacks audio? I think the extra code coverage would be nice here, especially considering the high rate of regressions that we encounter with the media controls.
Comment 23 Chris Pearce (:cpearce) 2012-06-26 16:49:35 PDT
Nice to have.
Comment 24 Justin Dolske [:Dolske] 2012-06-27 09:18:50 PDT
Driveby questions:

* Can the user still mute a video before it has loaded? I think it's important to retain the ability to do so. (Ah, yes, comment 15 seems to say this is indeed the case).

* Is there any visual icon switching as a video-with-sound is loaded? Since the common case will be that videos have sound, the UI (if not the API) should assume so, and only change the icon when it knows there actually isn't sound.
Comment 25 Jared Wein [:jaws] (please needinfo? me) 2012-06-27 09:32:28 PDT
Comment on attachment 635182 [details] [diff] [review]
Patch

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

To answer the previous question, there will be no icon switching for the common case of videos having audio. However, the volume cannot be adjusted until HAVE_METDATA is true.

I filed bug 768937 to add a video controls test case that lacks audio.
Comment 26 Chris Pearce (:cpearce) 2012-06-27 16:25:52 PDT
With my patch here the volume should be adjustable before readyState has reached HAVE_METADATA, even if we don't know whether the media has an audio track or not.
Comment 28 Ed Morley [:emorley] 2012-06-29 03:38:51 PDT
Sorry, this merged yesterday, but I completely forgot to do the bug marking:
https://hg.mozilla.org/mozilla-central/rev/f62640f30159

Note You need to log in before you can comment on or make changes to this bug.