1347648 - Firefox requests audio focus for no-audio media (e.g. GIFV)

Status: RESOLVED FIXED

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

Description

[Jan Henning [:JanH]]

We shouldn't do this - users might have music/podcasts/... playing in some other app and requesting audio focus for no-audio content (e.g. GIFV videos) does nothing but unnecessarily interrupt this.

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Hi Jan.
May I know which app/page you you get this issue?

Comment 2

[Nevin Chen(Not active on Bugzilla)]

oops.. Sorry. it's the GIFV files. Maybe related to code here : http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java#300

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hey Alastor,
Any idea about this?

Comment 4

[Jan Henning [:JanH]]

Apparently you've already found an example file.

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1347648 - part1 : only request Android audio focus for audible media.

For non-audible media, we shouldn't request audio focus because it might interrupt
other app who is playing music or podcast.

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

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1347648 - part2 : paused media element is non-audible.

If the media element is paused, it should be non-audible.

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

Comment 7

[Alastor Wu [:alwu]]

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

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug 1347648 - part3 : only request audio focus from gecko.

Media control would also be displayed for non-audible media, we shouldn't
arbitrary request audio focus . Only request audio focus from gecko which know
whether the media is audible.

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

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8849967 [details]
Bug 1347648 - part1 : only request Android audio focus for audible media.

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

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8849968 [details]
Bug 1347648 - part2 : paused media element is non-audible.

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

Comment 11

[Alastor Wu [:alwu]]

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

Comment 12

[Alastor Wu [:alwu]]

Attachment: Bug 1347648 - part4 : notify audible change after finishing set suspend and pause state.

Since the audible state would depend on pause and suspend state, we should do
the check after finishing play() or pause().

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

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8849967 [details]
Bug 1347648 - part1 : only request Android audio focus for audible media.

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

Comment 14

[Alastor Wu [:alwu]]

Comment on attachment 8849968 [details]
Bug 1347648 - part2 : paused media element is non-audible.

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

Comment 15

[Alastor Wu [:alwu]]

Comment on attachment 8850275 [details]
Bug 1347648 - part3 : only request audio focus from gecko.

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

Comment 16

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

Comment on attachment 8849968 [details]
Bug 1347648 - part2 : paused media element is non-audible.

https://reviewboard.mozilla.org/r/122730/#review125312

Comment 17

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

Comment on attachment 8850338 [details]
Bug 1347648 - part4 : notify audible change after finishing set suspend and pause state.

https://reviewboard.mozilla.org/r/122962/#review125316

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8849967 [details]
Bug 1347648 - part1 : only request Android audio focus for audible media.

https://reviewboard.mozilla.org/r/122704/#review125426

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8850275 [details]
Bug 1347648 - part3 : only request audio focus from gecko.

https://reviewboard.mozilla.org/r/122904/#review125906

Comment 20

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8e327cbac38e -d 1bfd5d58b3b1: rebasing 384149:8e327cbac38e "Bug 1347648 - part1 : only request Android audio focus for audible media. r=snorp"
rebasing 384150:2222db14a5d2 "Bug 1347648 - part2 : paused media element is non-audible. r=jwwang"
merging dom/html/HTMLMediaElement.cpp
rebasing 384151:a8ff33567602 "Bug 1347648 - part3 : only request audio focus from gecko. r=sebastian"
rebasing 384152:48335ced306a "Bug 1347648 - part4 : notify audible change after finishing set suspend and pause state. r=jwwang" (tip)
merging dom/html/HTMLMediaElement.cpp
warning: conflicts while merging dom/html/HTMLMediaElement.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 21

[Alastor Wu [:alwu]]

Comment on attachment 8849967 [details]
Bug 1347648 - part1 : only request Android audio focus for audible media.

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

Comment 22

[Alastor Wu [:alwu]]

Comment on attachment 8849968 [details]
Bug 1347648 - part2 : paused media element is non-audible.

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

Comment 23

[Alastor Wu [:alwu]]

Comment on attachment 8850275 [details]
Bug 1347648 - part3 : only request audio focus from gecko.

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

Comment 24

[Alastor Wu [:alwu]]

Comment on attachment 8850338 [details]
Bug 1347648 - part4 : notify audible change after finishing set suspend and pause state.

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

Comment 25

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 77664f4f12b8 -d 1bfd5d58b3b1: rebasing 384153:77664f4f12b8 "Bug 1347648 - part1 : only request Android audio focus for audible media. r=snorp"
rebasing 384154:362d440b50a5 "Bug 1347648 - part2 : paused media element is non-audible. r=jwwang"
merging dom/html/HTMLMediaElement.cpp
rebasing 384155:ee2f0f2c205c "Bug 1347648 - part3 : only request audio focus from gecko. r=sebastian"
rebasing 384156:808e9ca1aa31 "Bug 1347648 - part4 : notify audible change after finishing set suspend and pause state. r=jwwang" (tip)
merging dom/html/HTMLMediaElement.cpp
warning: conflicts while merging dom/html/HTMLMediaElement.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 26

[Alastor Wu [:alwu]]

Comment on attachment 8849967 [details]
Bug 1347648 - part1 : only request Android audio focus for audible media.

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

Comment 27

[Alastor Wu [:alwu]]

Comment on attachment 8849968 [details]
Bug 1347648 - part2 : paused media element is non-audible.

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

Comment 28

[Alastor Wu [:alwu]]

Comment on attachment 8850275 [details]
Bug 1347648 - part3 : only request audio focus from gecko.

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

Comment 29

[Alastor Wu [:alwu]]

Comment on attachment 8850338 [details]
Bug 1347648 - part4 : notify audible change after finishing set suspend and pause state.

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29ad09fc4655
part1 : only request Android audio focus for audible media. r=snorp
https://hg.mozilla.org/integration/autoland/rev/dbcbc70b0bc7
part2 : paused media element is non-audible. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ff66592a8baf
part3 : only request audio focus from gecko. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/e8a7790bd268
part4 : notify audible change after finishing set suspend and pause state. r=jwwang

Comment 31

[Alastor Wu [:alwu]]

Uplift flag.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29ad09fc4655
https://hg.mozilla.org/mozilla-central/rev/dbcbc70b0bc7
https://hg.mozilla.org/mozilla-central/rev/ff66592a8baf
https://hg.mozilla.org/mozilla-central/rev/e8a7790bd268

Comment 33

[Jan Henning [:JanH]]

Hmm - working as expected with regards to external apps, but still interrupts playback from other tabs *within* Firefox. Opening a separate bug for that, though…

Comment 34

[Alastor Wu [:alwu]]

Attachment: Bug1347648 - don't request audio focus for non-audible media (aurora/beta)

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression
[User impact if declined]: Playing non-audible media on Fennec would interrupt other playing app and stop them.
[Is this code covered by automated tests?]: No
[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?]: Would only happen when user play non-audible media and other app is playing, won't affect the basic ability of Fennec
[String changes made/needed]: No

Comment 35

[Jan Henning [:JanH]]

(In reply to Alastor Wu [:alwu] from comment #34)
> [Feature/Bug causing the regression]: Not a regression

Hmmm, since the audio focus/media control work is still quite fresh, I think that's debatable at the very least. A few versions ago we didn't even request audio focus at all (bug 1249579). Without running mozregression I can't say offhand which bug actually caused this (the initial implementation in bug 1249579 might have been completely fine, it might have been something related to the media control notification instead), but at some point between then and now *something* did change our behaviour here.

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Jan or Alastor, did it interrupt playback from other tabs before landing this fix? 

I don't want to take this big of a change on beta without much time left for testing. We might still be able to uplift to aurora but I will leave that up to Gerry.

Comment 37

[Alastor Wu [:alwu]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #36)
> Jan or Alastor, did it interrupt playback from other tabs before landing
> this fix? 

Yes, that is intended to do.

Comment 38

[Jan Henning [:JanH]]

@Liz:
Yup, that behaviour happened before this patch as well.

Comment 39

[Gerry Chang [:gchang]]

Comment on attachment 8851906 [details] [diff] [review]
Bug1347648 - don't request audio focus for non-audible media (aurora/beta)

Fix an issue that playing non-audible media on Fennec would interrupt other playing app. Aurora54+.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d6eb36ce4c8e

Comment 41

[Bogdan Surd, QA [:bsurd, NI]]

Verified as fixed on Beta build 54.0b2.
Device: Nexus 6 (Android 7.0).