Closed Bug 1182444 Opened 5 years ago Closed 5 years ago

Volume option is displayed as disabled although is enabled

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 + verified
firefox41 --- verified
firefox42 --- verified
firefox-esr38 --- unaffected

People

(Reporter: adalucinet, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(1 file)

Reproducible on latest 40.0b3 (Build ID: 20150709163524), latest 41.0a2 and 42.0a1 (both from 2015-07-09)
Affected platforms: Ubuntu 14.04 32-bit, Windows 10 32-bit, Mac OS X 10.8.5

Prerequisites: Camera and microphone available.

Steps to reproduce:
1. Launch Firefox and navigate to http://mozilla.github.io/webrtc-landing/gum_test.html
2. Click on Audio&Video button and share devices when prompted.
3. Right click on video area and select Show controls.

Expected results: All the available options from the container's controls bar are properly displayed.
Actual results: Volume is displayed as disabled.

Additional notes:
1. Screenshot showing this issue: https://goo.gl/pnhiUx
2. Not reproducible with 39.0 RC build 6 (Build ID: 20150630154324): https://goo.gl/hyJ9Ig
Also reproducible with 40.0b1 (Build ID: 20150702173756); will investigate further for the regression range.
[Tracking Requested - why for this release]:
Sounds like a bug we should not ship to release.
This is not a bug because the video container seen on this page is simply displaying the video frames that are coming in. The video does not have audio being played back, which is why the controls are showing that audio is not available (crossed out/disabled).

The final output of the WebRTC page will have the streams merged, but the "preview" doesn't merge them.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
Hmm, well...

As comment 0 notes, this works in Firefox 39. When I mute with the video element's controls, the audio mutes. So on 39, audio output can definitely going to the video element. 

[I'm a little confused, looking at the source, because it seems sometimes a separate audio element is being created... But I don't get one in the page, looks like that's maybe only intended to be used when ONLY audio is being used?]

The test page is broken for me in my running 7/7 Nightly (nothing seen/heard on the page, no errors in console), but works in a new profile. The controls have the mute button disabled, but the content menu has Mute command enabled and it successfully mutes the video. So audio is being streamed through the <video> element and can be muted, but we're not showing the UI.

Something in WebRTC / DOM media is incorrectly setting the mozHasAudio property on the video element now -- it's false, when I clearly hear audio. So that's why the controls show as disabled.

[Also, we should probably be checking .mozHasAudio in the context menu, and disabling Mute when it's not available.]
Status: RESOLVED → REOPENED
Component: Video/Audio Controls → WebRTC: Audio/Video
Flags: needinfo?(rjesup)
Product: Toolkit → Core
Resolution: INVALID → ---
Filed bug 1182606 for the context menu.
(In reply to Justin Dolske [:Dolske] from comment #4)
> Hmm, well...
> 
> As comment 0 notes, this works in Firefox 39. When I mute with the video
> element's controls, the audio mutes. So on 39, audio output can definitely
> going to the video element. 
> 
> [I'm a little confused, looking at the source, because it seems sometimes a
> separate audio element is being created... But I don't get one in the page,
> looks like that's maybe only intended to be used when ONLY audio is being
> used?]

Correct.  And when you select audio+video, the <video> element should be getting both from gUM.

And in Nightly (7/7, 7/8) windows, in gum_test.html, if I right-click and Mute, it mutes, and UnMute, it unmutes (and I can hear audio, at least in a headset).  Note that audio will sound funny in some cases since the AEC will be fighting you....

However, I agree that Show Controls is showing audio as disabled, and it doesn't mute/unmute if you click it (though the right-button context menu does).

> The test page is broken for me in my running 7/7 Nightly (nothing seen/heard
> on the page, no errors in console), but works in a new profile. The controls
> have the mute button disabled, but the content menu has Mute command enabled
> and it successfully mutes the video. So audio is being streamed through the
> <video> element and can be muted, but we're not showing the UI.

Right.  (Though I'm interested how your profile broke it... perhaps you set a disable on gum in about:config?  (navigator.something pref)

> Something in WebRTC / DOM media is incorrectly setting the mozHasAudio
> property on the video element now -- it's false, when I clearly hear audio.
> So that's why the controls show as disabled.

Perhaps it has to do with how this is set with a mozSrcObject stream?  This appears to come from MediaInfo::HasAudio() which is AudioInfo::IsValid(), which is mChannels > 0 && mRate > 0.  But I haven't gdb'd it to be sure.

> [Also, we should probably be checking .mozHasAudio in the context menu, and
> disabling Mute when it's not available.]

yes.
Component: WebRTC: Audio/Video → Video/Audio
Flags: needinfo?(rjesup)
Flags: needinfo?(jyavenard)
Flags: needinfo?(cpearce)
This is a regression right? A range would be helpful to pinpoint what changed here.
Flags: needinfo?(cpearce)
This is likely a result of bug 1153049; when I removed what I thought were redundant member (and for normal media playback they certainly were)
Flags: needinfo?(jyavenard)
[Tracking Requested - why for this release]:
Important usability regression

Set flags based on presumption that bug 1153049 is the cause and is on 40 and later.

jya - can you take this?  We'll need uplift to beta
Blocks: 1153049
Flags: needinfo?(jyavenard)
Keywords: regression
Regression range:

(m-c): 
Last good revision: 2c9708e6b54d
First bad revision: 2ee2da378d12
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2c9708e6b54d&tochange=2ee2da378d12

(m-i): 
Last good revision: e17f5910da75
First bad revision: 65492fe83362
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e17f5910da75&tochange=65492fe83362
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
Regression so let's track it.
The HTML MediaElement's audio info configuration isn't set via a call to MetadataLoaded as it normally would with standard media playback when streams are in use.
Ideally, we would want to set the MediaInfo object like we do for video with a call to HTMLMediaElement::UpdateMediaSize.
However, this would be a much greater change and the aim of this one is to make it as simple and quick as possible so it can be uplifted to beta.
Attachment #8634625 - Flags: review?(rjesup)
Attachment #8634625 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/c7e2eb0b375c
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Looks like a straightforward change. This is a regression in 40 and 40 and 41 are marked as affected. What do you think about uplifting to Beta and Aurora?
Flags: needinfo?(jyavenard)
Comment on attachment 8634625 [details] [diff] [review]
Show audio as enabled when an audio track is present in stream.

Approval Request Comment
[Feature/regressing bug #]: bug 1153049
[User impact if declined]: Audio will appear as muted but is active. Audio can't be muted
[Describe test coverage new/current, TreeHerder]: In m-c, local tests
[Risks and why]: Low. Though not what I would call an ideal fix, it was designed to have the least impact and lowest chance of regression.
[String/UUID change made/needed]: None
Flags: needinfo?(jyavenard)
Attachment #8634625 - Flags: approval-mozilla-beta?
Attachment #8634625 - Flags: approval-mozilla-aurora?
(In reply to Jean-Yves Avenard [:jya] from comment #16)
> [Describe test coverage new/current, TreeHerder]: In m-c, local tests

Given that the patch involves setting dummy values for for a function to return true, I'd prefer to see this fix verified before uplifting.

Florin - Does you team have time to verify this fix today?
Flags: needinfo?(florin.mezei)
Verified fixed on Windows 7 64bit, Mac OSX 10.9.5 and Ubuntu 14.04 32bit using latest Nightly 42.0a1 (buildID: 20150720030213).
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
Comment on attachment 8634625 [details] [diff] [review]
Show audio as enabled when an audio track is present in stream.

The fix has been verified. Thanks to QE for the quick turnaround. Let's get this into beta6. Beta+ Aurora+
Attachment #8634625 - Flags: approval-mozilla-beta?
Attachment #8634625 - Flags: approval-mozilla-beta+
Attachment #8634625 - Flags: approval-mozilla-aurora?
Attachment #8634625 - Flags: approval-mozilla-aurora+
Setting this for quick verification in Firefox 40 Beta.
Flags: qe-verify+
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #17)
> Given that the patch involves setting dummy values for for a function to
> return true, I'd prefer to see this fix verified before uplifting.

This is pretty much what the original code was doing, except that it stored a boolean to say "active"

But yeah, it's pretty ugly regardless.
Verified fixed with 40.0b6 (Build ID: 20150720220238) and latest 41.0a2 (from 2015-07-21), across platforms [1].

[1] Ubuntu 14.04 32-bit, Mac OSX 10.10.4, Windows 7 64-bit and Windows 10 32-bit
You need to log in before you can comment on or make changes to this bug.