1190040 - tab audio-indicator incorrectly shown for media without sound

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: 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.

Comment 1

[Chris Peterson [:cpeterson]]

Similarly, this imgur webm video has an audio indicator but no audio.

http://i.imgur.com/5DJNj0U.webm

Comment 3

[(no longer active)]

Attachment: Do not dispatch media-playback notifications for media elements that do not have an audio track

Comment 5

[Tom S [:evilpie]]

Does this actually help when you have a video with an audio track, but is muted?

Comment 7

[Caspy7]

Geoff, you dealt with some of this on your Noise Control addon. 
Do you have any additional insights or scenarios you can share?

Comment 8

[(no longer active)]

(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.

Comment 9

[Pulsebot]

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

Comment 10

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b39c0e12d03

Comment 11

[(no longer active)]

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!

Comment 12

[(no longer active)]

Attachment: 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!

Comment 13

[(no longer active)]

Attachment: 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!

Comment 14

[(no longer active)]

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!

Comment 15

[Paul Adenot (:padenot)]

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 ?

Comment 16

[(no longer active)]

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!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08301f579388
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bb505664ef

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08301f579388
https://hg.mozilla.org/mozilla-central/rev/f7bb505664ef

Comment 19

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Aurora 42.0a2 (buildID: 20150813124640).

Comment 20

[Alastor Wu [:alwu]]

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!

Comment 21

[(no longer active)]

(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...

Comment 22

[Alastor Wu [:alwu]]

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

Comment 23

[(no longer active)]

(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?

Comment 24

[Alastor Wu [:alwu]]

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.

Comment 25

[(no longer active)]

(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.

Comment 26

[Alastor Wu [:alwu]]

Got it, Thanks :)

Comment 27

[Alastor Wu [:alwu]]

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?

Comment 28

[Alastor Wu [:alwu]]

Hi, Paul,

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

Very appreciate!

Comment 29

[Paul Adenot (: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 ?

Comment 30

[(no longer active)]

(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.

Comment 31

[Alastor Wu [:alwu]]

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

Comment 32

[Alastor Wu [:alwu]]

[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.

Comment 33

[Alastor Wu [:alwu]]

In addition, I have already confirmed it with paul, we don't need to call WindowAudioCaptureChanged() for non-audible object.

Comment 34

[(no longer active)]

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.  :-)

Comment 35

[Alastor Wu [:alwu]]

Thanks, I'll implement it in the bug1228564 :)