Closed Bug 762191 Opened 12 years ago Closed 11 years ago

Volume control in video files is broken


(Toolkit :: Video/Audio Controls, defect)

16 Branch
Not set



Tracking Status
firefox13 --- unaffected
firefox14 --- unaffected
firefox15 --- unaffected
firefox16 + fixed
firefox-esr10 --- unaffected


(Reporter: sidrabbit, Assigned: cpearce)




(Keywords: regression)


(1 file, 2 obsolete files)

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 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.
Component: Untriaged → Video/Audio
Product: Firefox → Core
In the previous Nightly build it still works, so the regression window would be:
Keywords: regression
Window for Inbound:
Last good nightly: 2012-06-04
First bad nightly: 2012-06-05


Btw, in Video Fullscreen it works.
Ever confirmed: true
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.
Blocks: 749520
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: untriaged →
Target Milestone: --- → mozilla16
Assignee: nobody → paul
From the patch on bug 657791 (revision 730209ed620b):

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

This code is wrong. || 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.
I think we should make .mozHasAudio be true until we find that the assumption was false.
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: mozilla16 → ---
Version: Trunk → 16 Branch
Attached patch Patch for bug (obsolete) — Splinter Review
Tested with these two video files: (audio) (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.
Attachment #632130 - Flags: review?(cpearce)
Blocks: 480376
Version: 16 Branch → 15 Branch
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.
Attachment #632130 - Flags: review?(cpearce) → review-
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.
Attachment #632912 - Flags: review?(jaws)
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.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?
Attachment #632912 - Flags: review?(jaws) → review+

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 :(
Assignee: paul → cpearce
Target Milestone: --- → mozilla16
Backed out in since you were timing out in test_videocontrols.html, so you can get it right when you reland with that fixed :)
And, once some finally finished, mochitests/content/a11y/accessible/actions/test_media.html
(In reply to Phil Ringnalda (:philor) from comment #11)
> Backed out in
> 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...
Have you been able to push this to tryserver yet?
(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:
I'm pretty sure the tracking flag got accidentally unset there.
Oops, it totally did. We should still track this.
Attached patch PatchSplinter Review
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.

Attachment #632130 - Attachment is obsolete: true
Attachment #632912 - Attachment is obsolete: true
Attachment #635182 - Flags: review?(jaws)
Comment on attachment 635182 [details] [diff] [review]

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

::: toolkit/content/tests/widgets/
@@ +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?
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 on attachment 635182 [details] [diff] [review]

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

Can we also keep a test case that has a video with no audio track?
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.
Nice to have.
Version: 15 Branch → 16 Branch
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 on attachment 635182 [details] [diff] [review]

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.
Attachment #635182 - Flags: review?(jaws) → review+
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.
Sorry, this merged yesterday, but I completely forgot to do the bug marking:
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 772428
You need to log in before you can comment on or make changes to this bug.