1305869 - Media controls flicker on and off when playing a html game or other page that repeatedly uses short sounds

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Description

[Kevin Brosnan [Ex-Mozilla]]

When playing a browser based game such as Browser Quest [1] or running the IE Speed Reading [2] the icon for the media controls flickers in and out of the notification area. Maybe we should have a minimum length 15s? 5s?

[1] http://browserquest.mozilla.org/
[2] https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/speedreading/

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1305869 - don't display media control for very short sound.

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

Comment 2

[Alastor Wu [:alwu]]

Comment on attachment 8798334 [details]
Bug 1305869 - don't display media control for very short sound.

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

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8798334 [details]
Bug 1305869 - don't display media control for very short sound.

https://reviewboard.mozilla.org/r/83866/#review87582

I prefer to have this logic into MediaElement instead in the AudioChannel.
Why do you think this is the correct solution?

::: dom/audiochannel/AudioChannelService.cpp:1304
(Diff revision 2)
>    mAgents.RemoveElement(aAgent);
>  
>    MOZ_ASSERT(mChannels[channel].mNumberOfAgents > 0);
>    --mChannels[channel].mNumberOfAgents;
>  
> +  if (channel == int32_t(AudioChannel::Notification)) {

Also this seems wrong. Why don't we want to notify the channel active for 'Notification' ?

::: dom/html/HTMLMediaElement.cpp:5805
(Diff revision 2)
>      // too late. In that case (block autoplay in non-visited-tab), we need to
>      // create a connection before decoding, because we don't want user hearing
>      // any sound.
> +    if (Duration() < AUDIO_NOTIFICATION_THRESHOLD) {
> +      // set the type manually to avoid showing the media control for short sound.
> +      mAudioChannelAgent->SetAudioChannelType(int32_t(AudioChannel::Notification));

This is an horrible hack :)
What about if we have a different setup for Notification in the future?

Comment 4

[Alastor Wu [:alwu]]

Comment on attachment 8798334 [details]
Bug 1305869 - don't display media control for very short sound.

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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8798334 [details]
Bug 1305869 - don't display media control for very short sound.

https://reviewboard.mozilla.org/r/83866/#review88518

::: mobile/android/chrome/content/browser.js:4381
(Diff revision 3)
> +    // We don't want to show the media control interface for the short sound
> +    // which duration is smaller than the threshold.
> +    let mediaDurationThreshold = 1.0;

What unit is 1.0? Is this 1 second? Can you add this to the comment or property name?

::: mobile/android/chrome/content/browser.js:4385
(Diff revision 3)
> +    let audioElements = this.browser.contentDocument.getElementsByTagName("audio");
> +    for each (let audio in audioElements) {
> +      if (!audio.paused == active && audio.duration > mediaDurationThreshold) {
> +        return true;
> +      }
> +    }
> +
> +    let videoElements = this.browser.contentDocument.getElementsByTagName("video");
> +    for each (let video in videoElements) {
> +      if (!video.paused == active && video.duration > mediaDurationThreshold) {
> +        return true;
> +      }
> +    }

Do we always have a duration? Are there streaming formats or whatever where we don't have any?

::: mobile/android/chrome/content/browser.js:4387
(Diff revision 3)
> +    // which duration is smaller than the threshold.
> +    let mediaDurationThreshold = 1.0;
> +
> +    let audioElements = this.browser.contentDocument.getElementsByTagName("audio");
> +    for each (let audio in audioElements) {
> +      if (!audio.paused == active && audio.duration > mediaDurationThreshold) {

"!audio.paused == active" and "!video.paused == active" where a bit hard to understand. Isn't this the same as audio.paused != active and video.paused != active?

Comment 6

[Alastor Wu [:alwu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> What unit is 1.0? Is this 1 second? Can you add this to the comment or
> property name?

It's second, and I'll add the description in the comment.

> ::: mobile/android/chrome/content/browser.js:4385
> Do we always have a duration? Are there streaming formats or whatever where
> we don't have any?

Yes, the streaming format's duration is infinite, and we don't need to show media control if we don't have any element.

> ::: mobile/android/chrome/content/browser.js:4387
> "!audio.paused == active" and "!video.paused == active" where a bit hard to
> understand. Isn't this the same as audio.paused != active and video.paused
> != active?

OK, I'll modify that.

Comment 7

[Alastor Wu [:alwu]]

Comment on attachment 8798334 [details]
Bug 1305869 - don't display media control for very short sound.

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

Comment 8

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0869dc7d019a4617f353afea19b6ec8d457ad416

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f21095b038a
don't display media control for very short sound. r=sebastian

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6f21095b038a