762191 - Volume control in video files is broken

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Sid]

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]

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

[(mostly gone) XtC4UaLL [:xtc4uall]]

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 (Not reading bugmail)]]

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)]

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)]

I think we should make .mozHasAudio be true until we find that the assumption was false.

Comment 6

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

Attachment: 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 (Not reading bugmail)]]

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 (Not reading bugmail)]]

Attachment: 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)]

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 (Not reading bugmail)]]

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)]

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)]

And, once some finally finished, mochitests/content/a11y/accessible/actions/test_media.html

Comment 13

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

(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)]

Have you been able to push this to tryserver yet?

Comment 15

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

(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)]

I'm pretty sure the tracking flag got accidentally unset there.

Comment 17

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

Oops, it totally did. We should still track this.

Comment 18

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

Attachment: 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)]

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 (Not reading bugmail)]]

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)]

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)]

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 (Not reading bugmail)]]

Nice to have.

Comment 24

[Justin Dolske [:Dolske]]

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)]

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 (Not reading bugmail)]]

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 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f62640f30159

Comment 28

[Ed Morley [:emorley]]

Sorry, this merged yesterday, but I completely forgot to do the bug marking:
https://hg.mozilla.org/mozilla-central/rev/f62640f30159