1240429 - Implement silent-data checking in MediaElement when the input is the media stream

Status: RESOLVED WONTFIX

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Description

[Alastor Wu [:alwu]]

This issue is similar with the bug1238906.
Implement the way that MediaElement can be notified if its input audio stream doesn't produce any sound.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

It would help to describe the story why this feature is needed.

Comment 2

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240429 - silent-checking for input source stream.

Review commit: https://reviewboard.mozilla.org/r/31413/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31413/

Comment 3

[Alastor Wu [:alwu]]

Comment on attachment 8709390 [details]
MozReview Request: Bug 1240429 - silent-checking for input source stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31413/diff/1-2/

Comment 4

[Alastor Wu [:alwu]]

Hi, JW,
Could you help me review this patch?
Now I use the MediaStreamListener to do the silence-checking so that MediaElement can be notified the audible state when its input is a media stream.
Very appreciate!

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8709390 [details]
MozReview Request: Bug 1240429 - silent-checking for input source stream.

https://reviewboard.mozilla.org/r/31413/#review28289

::: dom/audiochannel/AudioChannelUtils.h:15
(Diff revision 2)
> +static const uint32_t SILENT_DATA_THRESHOLD_USECS = 500000;

500ms?

::: dom/audiochannel/AudioChannelUtils.h:26
(Diff revision 2)
> +class AudibleStatusCalculator {

AudibleStatus is inconsistent with the function name AudibleState below.

::: dom/audiochannel/AudioChannelUtils.h:33
(Diff revision 2)
> +  bool GetAudibleState(bool aIsSampleAudible, int64_t aDuration)

GetBlah() is usually a const function. Maybe UpdateAudibleState is a better name.

::: dom/audiochannel/AudioChannelUtils.h:64
(Diff revision 2)
> +  uint32_t mSilentDataDuration;

Shouldn't you use int64_t for the duration?

::: dom/html/HTMLMediaElement.cpp:3087
(Diff revision 2)
> +      bool audible = mCalculator.GetAudibleState(!audio.IsNull(),

You should also check the volume and each sample in the buffer.

::: dom/media/MediaDecoderStateMachine.h:1201
(Diff revision 2)
> -  uint32_t mSilentDataDuration;
> +  nsAutoPtr<AudibleStatusCalculator> mCalculator;

Use UniquePtr.

::: dom/media/MediaDecoderStateMachine.cpp:316
(Diff revision 2)
> +  mCalculator = new AudibleStatusCalculator();

Use member initialization list.

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8709390 [details]
MozReview Request: Bug 1240429 - silent-checking for input source stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31413/diff/2-3/

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8709390 [details]
MozReview Request: Bug 1240429 - silent-checking for input source stream.

https://reviewboard.mozilla.org/r/31413/#review28453

::: dom/media/AudioSegment.h:385
(Diff revision 3)
> +  bool IsAudible() const {

You also need to check each PCM value in the buffer.

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8709390 [details]
MozReview Request: Bug 1240429 - silent-checking for input source stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31413/diff/3-4/

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8709390 [details]
MozReview Request: Bug 1240429 - silent-checking for input source stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31413/diff/4-5/

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Note: I think before this lands, we need to make sure that this user-visible change in behavior is ok with other "stakeholders" - at minimum baku and probably someone from UX who approved the adding of the audio indicator, as this is a significant change in behavior, and changes accessibility of the ability to mute a tab - pausing a video (or rather in this case perhaps a webaudio generator, or the far side in a webrtc call muting), then going to mute the tab may not be possible if I understand how this works.

Comment 11

[Andrea Marchesini [:baku]]

> webaudio generator, or the far side in a webrtc call muting), then going to
> mute the tab may not be possible if I understand how this works.

Muting a tab is always possible via keyboard shortcut but yes, I really don't want to have a flickering audio icon if the video/webrtc/webaudio produce sound, then no-sound, then sound again and so on. But I guess this is something we should manage from a UI point of view and keep the icon for at least X secs after the real muting.

This seems a follow up. Ehsan, feedback?

Comment 12

[Alastor Wu [:alwu]]

The UX suggestion is in [1]. 
The sound icon would disappear in two cases,
(1) Totally silence (pause/mute/ZERO volume) or video without audio track
(2) The silence continuing more than 10 secs

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1235612#c18

Comment 13

[Alastor Wu [:alwu]]

In this patch, it has already implemented the 10-secs rule for audible state checking.

Comment 14

[(no longer active)]

(In reply to Andrea Marchesini (:baku) from comment #11)
> > webaudio generator, or the far side in a webrtc call muting), then going to
> > mute the tab may not be possible if I understand how this works.
> 
> Muting a tab is always possible via keyboard shortcut but yes, I really
> don't want to have a flickering audio icon if the video/webrtc/webaudio
> produce sound, then no-sound, then sound again and so on. But I guess this
> is something we should manage from a UI point of view and keep the icon for
> at least X secs after the real muting.
> 
> This seems a follow up. Ehsan, feedback?

Not sure if I'm following this correctly.  What's the specific issue with the flickering?  And how does that affect muting?

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

I'm concerned about it disappearing if the other side goes quiet due to hitting Mute (which will send all 0's), or if you pause something - suddenly your user api disappears.  I would be ok with *changing* the icon's imagery if the audio is silent/muted at the moment

Ehsan: I think baku is worried about the 1/2-second window the patch uses causing the icon to appear/disappear randomly and in some cases frequently (like in a game it would appear everytime the game generated sound, and disappear 1/2 second later or so).  And if the icon is gone, you can't use it to mute (or un-mute).

Comment 16

[Alastor Wu [:alwu]]

Hi, all,
Please see my following descriptions.

---

[Before this bug]
In present firefox, the sound icon would appear/disappear in following situations.

Appear   : when the a/v start playing (with volume/non-muted/with audio track)
Disapper : when the a/v is paused/muted/zero-volume/without audio track/seeking [1]

However, we can't handle the case that *the video with the silent audio track* (bug1235612) and *the input media stream is producing silence*. In these cases, the sound icon would always appear even if there is no any audible sound. 

[In this patch]
We would detect the input from media stream, and follow the 10-sec rule made by UX [2] to correctly display the sound icon according to its audible state. 

The 10-sec rule is used to avoid the icon flickering. When the audio silence is less than 10 secs, the sound icon would still keep itself there. It doesn't affect the muting tab ability. Therefore, it won't flicking when the input only have intermittent short silent period.

You can see more related info in bug1235612.

[Only flickering case]
Now we only have the one flickering case that is when we do seek for the a/v, see [1].

---

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1239372
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1235612#c18

Comment 17

[Alastor Wu [:alwu]]

In addition, this patch doesn't effect the icon displaying. This patch is only to provide a way to notify the audible state to MediaElement.

The changeset about icon would be implemented in bug1235612.

Comment 18

[(no longer active)]

(In reply to Randell Jesup [:jesup] from comment #15)
> I'm concerned about it disappearing if the other side goes quiet due to
> hitting Mute (which will send all 0's), or if you pause something - suddenly
> your user api disappears.

That's the current behavior.

>  I would be ok with *changing* the icon's imagery
> if the audio is silent/muted at the moment
> 
> Ehsan: I think baku is worried about the 1/2-second window the patch uses
> causing the icon to appear/disappear randomly and in some cases frequently
> (like in a game it would appear everytime the game generated sound, and
> disappear 1/2 second later or so).  And if the icon is gone, you can't use
> it to mute (or un-mute).

Ah, I just saw bug 1235612 comment 18 and 20.  Somehow I missed that before.  That sounds like a pretty bad idea to me.  I'll comment there.

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Alastor Wu [:alwu] from comment #17)
> In addition, this patch doesn't effect the icon displaying. This patch is
> only to provide a way to notify the audible state to MediaElement.
> 
> The changeset about icon would be implemented in bug1235612.

The point is if we don't take the feature in bug 1235612, the patches in this bug will not be useful.

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

IIUC, this feature is copied from Chrome which however doesn't provide the ability to mute a tab by clicking on the speaker icon as we do.

Comment 21

[Alastor Wu [:alwu]]

In fact, we can mute the tab even if there is no any sound indicator showing in the tab. 
Just use the right-click and select "mute tab", so I think it isn't a problem.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

(In reply to JW Wang [:jwwang] from comment #20)
> IIUC, this feature is copied from Chrome which however doesn't provide the
> ability to mute a tab by clicking on the speaker icon as we do.

That's part of why this is an issue; in Chrome it's just an indicator; if it comes and goes it's not a big deal.  With ours it's also a control, so hiding a control semi-randomly can be confusing and annoying.  (and keyboard/right-click options don't make it not an issue, I'm afraid).  It is an arguable issue, though.

Comment 23

[Alastor Wu [:alwu]]

(In reply to Randell Jesup [:jesup] from comment #22)
> That's part of why this is an issue; in Chrome it's just an indicator; if it
> comes and goes it's not a big deal.  With ours it's also a control, so
> hiding a control semi-randomly can be confusing and annoying.  (and
> keyboard/right-click options don't make it not an issue, I'm afraid).  It is
> an arguable issue, though.

But why we need to control something without any audible sound? If the control is the mainly goal that we would provide this indicator in every tab, but we didn't. That is because we want this indicator can directly correlated to audible sound.

Therefore, what we need to do is to follow this goal and make sure it won't bring some bad user-experience (ex. flickering). 

To hide icon is just one method, we still have other methods to inform user isn't audible. ex. provide a new icon which can let user clearly know it's just an controller. However, that are the follow-up, the first thing we need to do is to make sure we have the ability to detect the long silence and make sure we can inform that to our users.

Comment 24

[JW Wang [:jwwang] [:jw_wang]]

Being predictable is important to the user experience. It hurts the user if the icon goes way at the moment when the user tries to click the icon to mute the tab.

Comment 25

[Alastor Wu [:alwu]]

The predict rule is very simple and easy that the indicator only appears when there is audible sound. The case you mentioned just like that users have intend to mute the tab without any sound. Have you ever tried to press "mutd-tab" button for the tab without sound? (like bugzilla) I think the answer is not.

The point I want to emphasize is, "if there is no any audible sound, why we need to mute it?" If the sound starts again and interrupts users, the indicator would display again, and then users can mute it.

Comment 26

[Alastor Wu [:alwu]]

Here is one case users might want to mute the tab even it's not producing audible sound (bug1240358).
But in this case, the sound indicator doesn't appear when user want to mute the tab.

Comment 27

[Alastor Wu [:alwu]]

FYI, Ehsan had some feedback in [1]. We're waiting the feedback from the Philipp now (firefox UX).

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1235612#c29

Comment 28

[Alastor Wu [:alwu]]

I have another proposal is that we can divide this issue into two steps to handle.

(1) Initialize the sound indicator when it starts audible
Do the audible-checking, stop it until the input source is producing audible sound.
However, in this case, if the element stops producing audible sound, the sound indicator would continue showing.

(2) Dynamic detect the audible state after it has already been initialized by (1) 
Because we have no consensus about whether to show the sound icon when it *becomes* non-audible, we can postpone to implement it until we have the full-spec.

In ehsan's reply [1], seems we want the different strategies between living media stream and web audio.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1235612#c29

---

Therefore, in this issue, we can only do (1), but not implement (2) yet.
This can reduce some wrong-sound-indicator cases, and won't change the current behavior.

Comment 29

[Alastor Wu [:alwu]]

Close this bug because the media stream is often used for live streaming and we don't want the audio indicator disappear in this case, see [1] for more details.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1235612#c47