tab audio-indicator incorrectly shown for media without sound

VERIFIED FIXED in Firefox 42

Status

()

Core
Audio/Video: Playback
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Dolske, Assigned: Away for a while)

Tracking

42 Branch
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 unaffected, firefox42 verified)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8642003 [details]
Screenshot

Media without an audio track (such as the "gifv" http://i.imgur.com/tyqePAf.gifv) is displaying a tab audio indicator. Yet the default video controls (right-click, Show Controls) indicate that there is no audio track.

The tab indicator should not be shown when there's no actual audio available (ie, medis.mozhasAudio == false). See screenshot, where the volume indicator is disabled with an (/) icon.

Updated

2 years ago
Component: Tabbed Browser → Audio/Video: Playback
Product: Firefox → Core
Similarly, this imgur webm video has an audio indicator but no audio.

http://i.imgur.com/5DJNj0U.webm
status-firefox41: --- → unaffected
Version: Trunk → 42 Branch
Blocks: 1188860
Duplicate of this bug: 1190023
(Assignee)

Updated

2 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 3

2 years ago
Created attachment 8642038 [details] [diff] [review]
Do not dispatch media-playback notifications for media elements that do not have an audio track
Attachment #8642038 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Blocks: 1190023
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1189885
Does this actually help when you have a video with an audio track, but is muted?
Duplicate of this bug: 1190096
Status: NEW → ASSIGNED

Comment 7

2 years ago
Geoff, you dealt with some of this on your Noise Control addon. 
Do you have any additional insights or scenarios you can share?
Flags: needinfo?(geoff)
(Assignee)

Comment 8

2 years ago
(In reply to Tom Schuster [:evilpie] from comment #5)
> Does this actually help when you have a video with an audio track, but is
> muted?

Yes.  See the test I have added in bug 1190023.

(In reply to Caspy7 from comment #7)
> Geoff, you dealt with some of this on your Noise Control addon. 
> Do you have any additional insights or scenarios you can share?

I have a good handle on what is happening here.  Thanks, this patch fixes the issue.
Flags: needinfo?(geoff)
(Assignee)

Updated

2 years ago
Attachment #8642038 - Flags: review?(amarchesini) → review?(cpearce)
Attachment #8642038 - Flags: review?(cpearce) → review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c81a51b3fc

Comment 10

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b39c0e12d03
(Assignee)

Comment 11

2 years ago
This broke dom/media/tests/mochitest/test_getUserMedia_audioCapture.html.  The reason seems to be that not notifying the agent means that the agent won't be registered in the winData and when we try to call WindowAudioCaptureChange() from AudioChannelService::RefreshAgentsCapture(), we can't find the agent, therefore the test times out.  It seems like we need to teach the Audio Channel service how to avoid notifying media-playback!
(Assignee)

Comment 12

2 years ago
Created attachment 8643678 [details] [diff] [review]
Part 2: Teach the audio channel service how to not notify audio-playback, and do that when a media element has no audio track

Paul, can you please help review this since Andrea is on PTO?  Thanks!
Attachment #8643678 - Flags: review?(padenot)
(Assignee)

Comment 13

2 years ago
Created attachment 8643687 [details] [diff] [review]
Part 2: Teach the audio channel service how to not notify audio-playback, and do that when a media element has no audio track

Oops, sorry the previous patch was cut in half!
Attachment #8643678 - Attachment is obsolete: true
Attachment #8643678 - Flags: review?(padenot)
Attachment #8643687 - Flags: review?(padenot)
(Assignee)

Comment 14

2 years ago
Comment on attachment 8643687 [details] [diff] [review]
Part 2: Teach the audio channel service how to not notify audio-playback, and do that when a media element has no audio track

Chris, maybe you'd feel comfortable reviewing this?  I'd like to land this ASAP...

Thanks!
Attachment #8643687 - Flags: review?(cpearce)
Comment on attachment 8643687 [details] [diff] [review]
Part 2: Teach the audio channel service how to not notify audio-playback, and do that when a media element has no audio track

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

Sorry for the delay.

::: dom/audiochannel/AudioChannelService.h
@@ +49,5 @@
>     * Any audio channel agent that starts playing should register itself to
>     * this service, sharing the AudioChannel.
>     */
> +  void RegisterAudioChannelAgent(AudioChannelAgent* aAgent,
> +                                 bool aNotifyPlayback,

I'd prefer to not use booleans in signatures (boolean trap), but maybe it's harder to replace it by an enum the IDL ? Maybe we could name the boolean at call site instead ?
Attachment #8643687 - Flags: review?(padenot) → review+
(Assignee)

Comment 16

2 years ago
Comment on attachment 8643687 [details] [diff] [review]
Part 2: Teach the audio channel service how to not notify audio-playback, and do that when a media element has no audio track

Will use an enum in the IDL.  Thanks for the review!
Attachment #8643687 - Flags: review?(cpearce)

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/08301f579388
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bb505664ef
https://hg.mozilla.org/mozilla-central/rev/08301f579388
https://hg.mozilla.org/mozilla-central/rev/f7bb505664ef
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Aurora 42.0a2 (buildID: 20150813124640).
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
Hi, Ehsan,

In my opinion, we can only use the patch 1 to solve this issue. Why we need the patch 2?

When no-audio-track MediaElement registeted the AudioChannelAgent, it causes the incorrect audio competing interruption.

ex. In b2g, when we open the camera app, the video stream shouldn't interrupt other audio.
    but now it would interrupt other audio, because it registered an AudioChannelAgent.

Thanks!
Flags: needinfo?(ehsan)
(Assignee)

Comment 21

2 years ago
(In reply to Alastor Wu [:alwu] from comment #20)
> Hi, Ehsan,
> 
> In my opinion, we can only use the patch 1 to solve this issue. Why we need
> the patch 2?

Because we need to still notify the audio channel service if the audio element doesn't have an audio track, otherwise it will get confused about the state of the media element.  The point of part 2 is to not dispatch the media-playback notification in case there is no audio track, which only controls the visibility of the tab audio indicator.

> When no-audio-track MediaElement registeted the AudioChannelAgent, it causes
> the incorrect audio competing interruption.
> 
> ex. In b2g, when we open the camera app, the video stream shouldn't
> interrupt other audio.
>     but now it would interrupt other audio, because it registered an
> AudioChannelAgent.

Then sounds like you need to handle AUDIO_AGENT_DONT_NOTIFY for audio competing interruption somehow as well.  I don't know the details of that code so I can't suggest anything more concrete...
Flags: needinfo?(ehsan)
Hi, Ehsan,

Thanks for your explanation.
As we don't register MediaElement when it is muted or no volume [1], I don't understand the reason why we need to register the no-audio-track MediaElement.
Could you tell me more about "get confused about the state of the media element"?

Very appreciate!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#4755
Flags: needinfo?(ehsan)
(Assignee)

Comment 23

2 years ago
(In reply to Alastor Wu [:alwu] from comment #22)
> Hi, Ehsan,
> 
> Thanks for your explanation.
> As we don't register MediaElement when it is muted or no volume [1]

That is not the same condition as HasAudio(): <https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.h#430>.  We definitely register the media element with the audio channel service when it has no audio track, because it may gain one later, which is what part 2 tries to handle.  When we start to have an audio track, we are already registered with the audio channel service, so all we have to do is dispatch the media-playback notification.

You can test this by reverting part 2 locally and running the tests.  IIRC one of the dom/base/tests/*audio* tests should fail.

Does that make more sense now?
Flags: needinfo?(ehsan)
Hi, Ehsan,

If the MediaElement doesn't have any audio track in the beginning, and then gains the audio track later.
We can register AudioChannelAgent at that moment, don't need to register it in advance.

I still don't understand the benefit of registering AudioChannelAgent for no-audio-track MediaElement.

IMMO, we should only register AudioChannelAgent when the MediaElement is prepared to playback audio.

---

Ni baku to this discussion.
Flags: needinfo?(ehsan)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 25

2 years ago
(In reply to Alastor Wu [:alwu] from comment #24)
> Hi, Ehsan,
> 
> If the MediaElement doesn't have any audio track in the beginning, and then
> gains the audio track later.
> We can register AudioChannelAgent at that moment, don't need to register it
> in advance.
> 
> I still don't understand the benefit of registering AudioChannelAgent for
> no-audio-track MediaElement.
> 
> IMMO, we should only register AudioChannelAgent when the MediaElement is
> prepared to playback audio.

If I remember correctly, when I tried that approach, I saw a test failure.  I can't find the test right now but IIRC it was happening because we were not calling WindowAudioCaptureChanged() when we were supposed to.  I _think_ the test in question might have been test_getUserMedia_audioCapture.html which was added in bug 1156472 where the call to WindowAudioCaptureChanged() was also added.
Flags: needinfo?(ehsan)
Got it, Thanks :)
Hi, Ehsan,

I am thinking about whether the MediaElement really needs to call WindowAudioCaptureChanged() when the source media stream doesn't have an audio track. I'll ask paul for that, if the answer is yes, we should keep registering AudioChannelAgent for non-audio-track MediaElement.

But if not, maybe the problem is that MediaElement::HasAudio() doesn't update the audio info correctly for source media stream, and we can figure the problem and fix it.

How do you think?
Flags: needinfo?(ehsan)
Hi, Paul,

Do we really need to call MediaElement::WindowAudioCaptureChanged() even if the source media stream doesn't have an audio track?

Very appreciate!
Flags: needinfo?(amarchesini) → needinfo?(padenot)
(In reply to Alastor Wu [:alwu] from comment #28)
> Hi, Paul,
> 
> Do we really need to call MediaElement::WindowAudioCaptureChanged() even if
> the source media stream doesn't have an audio track?
> 
> Very appreciate!

I think for now we are calling it for all MediaElement, but it could be changed maybe ?
Flags: needinfo?(padenot)
(Assignee)

Comment 30

2 years ago
(In reply to Alastor Wu [:alwu] from comment #27)
> But if not, maybe the problem is that MediaElement::HasAudio() doesn't
> update the audio info correctly for source media stream, and we can figure
> the problem and fix it.

I don't really understand the media capture side of things here, and I also don't understand the problem you're trying to solve.  All I can really say is that if you want to special case the interrupt behavior on b2g, you should be able to use the suggestion in comment 21.

Note that we can definitely change how and when media elements are registered with the audio channel service, so as long as you come up with a plan that keeps the Firefox side and the media capture side working the same as they do right now, we can discuss changing how this works.  But it's really difficult for me to suggest anything concretely since I don't understand all of the different angles here.
Flags: needinfo?(ehsan)
See Also: → bug 1227078
Hi, Ehsan,

Here I want to propose some ideas after running the test_getUserMedia_audioCapture.html and studying about the WindowAudioCaptureChanged().

tl;dr : the test is passed; we shouldn't register AudioChannelAgent for non-audible object.

---

(1) About AudioChannelService
In my opinion, the AudioChannelService is used to manage all audible sound. 
- in desktop, we use it to control the volume of the tab. 
- in b2g, we use it to control the audio competing.

(2) Why I don't want to register a non-audible object
First, I still think we should only register AudioChannelAgent for audible object. It's not a special case for b2g, it should be a basic rule for AudioChannelService. It's meaningless and confused that AudioChannelService needs to manage a non-audible object.

If we did that is only because want to call the WindowAudioCaptureChanged(), this method seems totally like a workaround. Do we really need to capture a "silence" stream? The answer is probably not. 

In addition, we use the AudioChannelAgent number to decide whether the specific channel is active [1] and whether dispatch the event. (before this patch) [2] 

(3) The testing result of "test_getUserMedia_audioCapture.html"
This test can pass successfully in the latest master, so I don't think it's a problem.

However, after study the usage of WindowAudioCaptureChanged(), we still have some cases need to consider.

For example, if we playback a new MediaElement after calling GetAudioCaptured(), this MediaElement wouldn't be captured. I would describe this situation more detailed in following comment.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#843
[2] https://dxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#291
Flags: needinfo?(ehsan)
[About WindowAudioCaptureChanged]

The purpose of the bug1156472 is to capture the audio for the screen sharing, it is implemented by following steps,

1. using the nsPIDOMWindow::GetAudioCaptured() to capture all audible objects in the window
2. by RefreshAgentsCapture(), we would call the WindowAudioCaptureChanged() of each audible object
3. create a DOMMediaStream to contain its media content 

That is, if the object is "audible" and "its owner window have been enable capturing audio", we should call WindowAudioCaptureChanged() to generate the captured DOMMediaStream.

---

[The situation we didn’t handle in bug1156472]

In present design, the WindowAudioCaptureChanged() would only be called via the GetAudioCaptured(). 
We don't have a mechanism to automatically capture other new MediaElements which are created after calling GetAudioCaptured(). 

We need to have a check when registering the AudioChannelAgent. If its owner window has already been captured, we should capture its media content as well.
In addition, I have already confirmed it with paul, we don't need to call WindowAudioCaptureChanged() for non-audible object.
(Assignee)

Comment 34

2 years ago
What you're describing in comment 31 and 32 makes sense to me.  I'm fine with changing things like this provided that your changes pass all of our tests on the try server.  :-)
Flags: needinfo?(ehsan)
Thanks, I'll implement it in the bug1228564 :)

Updated

2 years ago
Depends on: 1240369
You need to log in before you can comment on or make changes to this bug.