1320005 - Don't show 'play tab' icon for video without audio track

Status: VERIFIED FIXED

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

Description

[Alastor Wu [:alwu]]

Fork from bug1318462 comment4.

---

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161122030216

Play tab button is also shown when opening "Get Firefox for Android" thumbnail from the New tab.

STR:
1. Launch Firefox with a new profile
2. Open a new tab
3. Click on the "Get Firefox for Android" thumbnail.

Expected results:
The Play tab button should not be displayed in the opened tab.

Actual results:
Tab play button is displayed, even if there's no media to play and the tab is not in the foreground

Comment 1

[Alastor Wu [:alwu]]

We should check whether the blocked media element has audio or not, we don't need to send the notification if it doesn't have audio track.

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1320005 - don't show the 'play tab' icon for the media element without audio track.

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

Comment 3

[Alastor Wu [:alwu]]

Ah, comment1 didn't work well because we don't get the metadata at that time...

Comment 4

[Alastor Wu [:alwu]]

Comment on attachment 8813982 [details]
Bug 1320005 - don't show the 'play tab' icon for the media element without audio track.

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

Comment 5

[Alastor Wu [:alwu]]

Comment on attachment 8813982 [details]
Bug 1320005 - don't show the 'play tab' icon for the media element without audio track.

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

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8813982 [details]
Bug 1320005 - don't show the 'play tab' icon for the media element without audio track.

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

Comment 7

[Alastor Wu [:alwu]]

Comment on attachment 8813982 [details]
Bug 1320005 - don't show the 'play tab' icon for the media element without audio track.

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

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8813982 [details]
Bug 1320005 - don't show the 'play tab' icon for the media element without audio track.

https://reviewboard.mozilla.org/r/95290/#review98536

::: dom/audiochannel/AudioChannelService.h:69
(Diff revision 5)
>    NS_DECL_NSIOBSERVER
>    NS_DECL_NSIAUDIOCHANNELSERVICE
>  
> -  enum AudibleState : bool {
> -    eAudible = true,
> -    eNotAudible = false
> +  /**
> +   * eNotAudible : agent is no audible
> +   * eMaybeAudible : agent is no audible now, but it might be audible later

is not

::: dom/html/HTMLMediaElement.cpp:889
(Diff revision 5)
> -      return false;
> +      return AudioChannelService::AudibleState::eNotAudible;
>      }
>  
> -    // No sound can be heard during suspending.
> -    if (IsSuspended()) {
> -      return false;
> +    // No audio track.
> +    if (!mOwner->HasAudio()) {
> +      return AudioChannelService::AudibleState::eNotAudible;;

double ;;

::: dom/html/HTMLMediaElement.cpp:6741
(Diff revision 5)
>  HTMLMediaElement::SetMediaInfo(const MediaInfo& aInfo)
>  {
> +  const bool oldHasAudio = mMediaInfo.HasAudio();
>    mMediaInfo = aInfo;
>    if (mAudioChannelWrapper) {
>      mAudioChannelWrapper->AudioCaptureStreamChangeIfNeeded();

Write a comment explaining why HasAudio() can change value here.

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8813982 [details]
Bug 1320005 - don't show the 'play tab' icon for the media element without audio track.

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

Comment 10

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e431051d8bcbab15dc7ae7058b772f76c9d7aaf

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8813982 [details]
Bug 1320005 - don't show the 'play tab' icon for the media element without audio track.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95290/diff/6-7/

Comment 12

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb1d2f13506e21346aab851ec0284ca1e7843556&selectedJob=32591641

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c815adcd0cb8
don't show the 'play tab' icon for the media element without audio track. r=baku

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c815adcd0cb8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you feel it has baked for sufficiently long on Nightly.

Comment 17

[Alastor Wu [:alwu]]

Attachment: Bug 1320005 - don't show the 'play tab' icon for the media element without audio track. (for aurora)

Approval Request Comment
[Feature/Bug causing the regression]: The play icon would be showed in some wrong situations
[User impact if declined]: user would see the wrong unblocking icon and it won't disappear anymore
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: only affect icon display cases
[String changes made/needed]: no

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8819263 [details] [diff] [review]
Bug 1320005 - don't show the 'play tab' icon for the media element without audio track. (for aurora)

fix new a/v issue in aurora52

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d428919f82f

Comment 20

[Simona Badau, Desktop QA]

Verified as fixed using the latest Nightly 53.0a1 (Build ID: 20170108030212)and the latest Developer Edition 52.0a2 (Build ID: 20170108004005) on Windows 10 x64 bit, Mac OS X 10.11 and Ubuntu 16.04 x64 bit.

The play tab indicator is not displayed anymore when using the scenario from the Description or the scenario from https://bugzilla.mozilla.org/show_bug.cgi?id=1318462#c0.