Closed
Bug 1182444
Opened 9 years ago
Closed 9 years ago
Volume option is displayed as disabled although is enabled
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
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)
1.95 KB,
patch
|
jesup
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•9 years ago
|
||
Also reproducible with 40.0b1 (Build ID: 20150702173756); will investigate further for the regression range.
Comment 2•9 years ago
|
||
[Tracking Requested - why for this release]: Sounds like a bug we should not ship to release.
Comment 3•9 years ago
|
||
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: 9 years ago
Resolution: --- → INVALID
Updated•9 years ago
|
status-firefox40:
affected → ---
status-firefox41:
affected → ---
status-firefox42:
affected → ---
tracking-firefox40:
? → ---
Comment 4•9 years ago
|
||
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 → ---
Comment 5•9 years ago
|
||
Filed bug 1182606 for the context menu.
Comment 6•9 years ago
|
||
(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)
Comment 7•9 years ago
|
||
This is a regression right? A range would be helpful to pinpoint what changed here.
Flags: needinfo?(cpearce)
Keywords: regressionwindow-wanted
Assignee | ||
Comment 8•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
[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
status-firefox39:
--- → unaffected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox-esr38:
--- → unaffected
tracking-firefox40:
--- → ?
Flags: needinfo?(jyavenard)
Keywords: regression
Reporter | ||
Comment 10•9 years ago
|
||
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
Keywords: regressionwindow-wanted
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
Regression so let's track it.
Assignee | ||
Comment 12•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8634625 -
Flags: review?(rjesup) → review+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c7e2eb0b375c
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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?
Comment 17•9 years ago
|
||
(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)
Comment 18•9 years ago
|
||
Verified fixed on Windows 7 64bit, Mac OSX 10.9.5 and Ubuntu 14.04 32bit using latest Nightly 42.0a1 (buildID: 20150720030213).
Comment 19•9 years ago
|
||
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+
Assignee | ||
Comment 23•9 years ago
|
||
(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.
Reporter | ||
Comment 24•9 years ago
|
||
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.
Description
•