Closed Bug 1262053 Opened 9 years ago Closed 9 years ago

Block playback of media until document is visible

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
relnote-firefox --- 53+
firefox50 --- unaffected
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: u480271, Assigned: alwu)

References

Details

Attachments

(10 files, 10 obsolete files)

Bug 1262053 - part1 : unblock window's media when the page was first visited.
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1262053 - part2 : remove old media.block-play-until-visible behaviour.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1262053 - part3 : modify media element for blocking autoplay media.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1262053 - part4 : don't dispatch dom event for blocked media.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1262053 - part5 : register audio agent immediately when media element starts playing.
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1262053 - part6 : don't need to capture media element without audio.
58 bytes, text/x-review-board-request
baku
: review+
cpearce
: review+
Details
Bug 1262053 - part7 : give audio focus for the non-visited page.
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1262053 - part8 : remove function NotifyOwnerDocumentActivityChangedInternal.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1262053 - part9 : add test case.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1262053 - part10 : modify tests.
58 bytes, text/x-review-board-request
baku
: review+
Details
No description provided.
Another take on implementing Bug 1187778 built on Bug 1242874. Needs Bug 1235612 to improve UX.
Assignee: nobody → dglastonbury
Blocks: 1187778
Depends on: 1242874, 1235612
MozReview-Commit-ID: 8VCpRs5YnTu
Attachment #8738024 - Flags: feedback?(cpearce)
Attachment #8738024 - Flags: feedback?(alwu)
These are the changes I had to make to get block playback working. I tested against loading the following pages in a new tab via context menu: http://http://people.mozilla.org/~dglastonbury/block-playback/autoplay-test-1.html http://http://people.mozilla.org/~dglastonbury/block-playback/autoplay-test-2.html
Comment on attachment 8738024 [details] [diff] [review] Port block-playback on to AudioChannel suspending. f?cpearce, alwu Review of attachment 8738024 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Much simpler. ::: dom/html/HTMLMediaElement.cpp @@ +3985,5 @@ > // being paused. We also activate autoplay when playing a media source since > // the data download is controlled by the script and there is no way to > // evaluate MediaDecoder::CanPlayThrough(). > + > + if (!HasAttr(kNameSpaceID_None, nsGkAtoms::autoplay) || !mAutoplayEnabled) { I feel like you should land this change separately (now), so it's out of your queue. @@ +4994,5 @@ > #endif > return false; > } > > // The MediaElement can start playback until it's resumed by audio channel. That comment doesn't make sense. Can you update it while you're here?
Attachment #8738024 - Flags: feedback?(cpearce) → feedback+
MozReview-Commit-ID: JqFIahvAnU8
MozReview-Commit-ID: GDiS8eMv7Le
MozReview-Commit-ID: ILDhWCVUBNt
MozReview-Commit-ID: JG1GCpldjex
MozReview-Commit-ID: KDbCfjyBzGU
Comment on attachment 8738024 [details] [diff] [review] Port block-playback on to AudioChannel suspending. f?cpearce, alwu Review of attachment 8738024 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +4283,5 @@ > > void HTMLMediaElement::NotifyOwnerDocumentActivityChanged() > { > + NotifyOwnerDocumentActivityChangedInternal(); > + if (!IsActive() && mAudioChannelAgent) { This change stops the audio agent being deleted before unblock message arrives. The comment below suggests that this should only happen when going into bfcache. This is checked by !IsActive() which used to be what NotifyOwnerDocumentActivityChangedInternal() returned. After the changes here, NotifyOwnerDocumentActivityChangedInternal returns false in case the agent is blocked or paused.
Comment on attachment 8738024 [details] [diff] [review] Port block-playback on to AudioChannel suspending. f?cpearce, alwu Review of attachment 8738024 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +2626,5 @@ > + nsPIDOMWindowOuter* outerWindow = aDocument->GetWindow(); > + if (outerWindow) { > + WindowMediaConfig config = service->GetMediaConfig(outerWindow, > + (uint32_t)mAudioChannel); > + SetAudioChannelSuspended(config.mSuspended); alwu, Is this code needed? I'm getting the current blocking state from the global window when ever the media element is attached to document. @@ +4930,1 @@ > mPaused.SetCanPlay(true); alwu, I'm not sure what SetCanPlay() does. I had to add in mPaused = false to get the Media Element to start playing when resuming from being blocked.
Comment on attachment 8738024 [details] [diff] [review] Port block-playback on to AudioChannel suspending. f?cpearce, alwu Review of attachment 8738024 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +2626,5 @@ > + nsPIDOMWindowOuter* outerWindow = aDocument->GetWindow(); > + if (outerWindow) { > + WindowMediaConfig config = service->GetMediaConfig(outerWindow, > + (uint32_t)mAudioChannel); > + SetAudioChannelSuspended(config.mSuspended); No, we don't need them, I would update the patch to show what I modified. @@ +4930,1 @@ > mPaused.SetCanPlay(true); Ditto, we can remove this change, see following patch. @@ +4995,5 @@ > return false; > } > > // The MediaElement can start playback until it's resumed by audio channel. > + if (mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE) { The only thing I concern is that if we make blocked audio complete the playinteral(), that means the onplay() function would be triggered. Is that what you want?
Attachment #8738024 - Flags: feedback?(alwu) → feedback+
Attached patch ChangedForBlock (obsolete) — — Splinter Review
The problem we got before is because the we didn't handle |decoder->play()| in HTMLMediaElement::CheckAutoplayDataReady. The |decoder->play()| would be called after we suspended the decoder, that is why we can still hear the sound.
Attachment #8738489 - Flags: feedback?(dglastonbury)
(In reply to Alastor Wu [:alwu] from comment #14) > Created attachment 8738489 [details] [diff] [review] > ChangedForBlock > > The problem we got before is because the we didn't handle |decoder->play()| > in HTMLMediaElement::CheckAutoplayDataReady. > > The |decoder->play()| would be called after we suspended the decoder, that > is why we can still hear the sound. Thanks for the patch. I've tried it locally and it works well. I'll fold it into my local copy.
Attachment #8738489 - Flags: feedback?(dglastonbury)
Depends on: 1262358
Status: NEW → ASSIGNED
Blocks: 1265981
Mass change P2 -> P3
Priority: P2 → P3
(I understand that this feature is not finish yet.) A user coming from Chrome (who is used to the functionality) flipped on media.block-play-until-visible on release. There are actually no complaints except for one, that they like to listen to music in playlists on youtube, and when youtube automatically directs to a new video to play, it is stopped until the user selects the tab again. Given the intent of any user playing through a background playlist, we'd surely not want to block in this instance. This may already be accounted for, I just wanted to make sure that this use-case is on the radar during development / testing as I think it's not at all uncommon (and goes perfectly in line with bug 1224973!).
Assignee: dglastonbury → alwu
Tina, Could you help check the UX part first? Thanks!
Flags: needinfo?(thsieh)
I would go to PTO soon, so I would continue to move this bug forward after 7/28.
Attached file Obsoletes (obsolete) —
Attachment #8738024 - Attachment is obsolete: true
Attachment #8738392 - Attachment is obsolete: true
Attachment #8738394 - Attachment is obsolete: true
Attachment #8738395 - Attachment is obsolete: true
Attachment #8738396 - Attachment is obsolete: true
Attachment #8738397 - Attachment is obsolete: true
Attachment #8738489 - Attachment is obsolete: true
Attachment #8776981 - Attachment is obsolete: true
Comment on attachment 8776995 [details] Bug 1262053 - part1 : unblock window's media when the page was first visited. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68592/diff/1-2/
Attachment #8776999 - Attachment description: Bug 1262053 - part5 : don't need to update the same value for window's suspend. → Bug 1262053 - part5 : add test case.
Comment on attachment 8776996 [details] Bug 1262053 - part2 : remove old media.block-play-until-visible behaviour. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68594/diff/1-2/
Comment on attachment 8776997 [details] Bug 1262053 - part3 : modify media element for blocking autoplay media. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68596/diff/1-2/
Comment on attachment 8776998 [details] Bug 1262053 - part4 : don't dispatch dom event for blocked media. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68598/diff/1-2/
Comment on attachment 8776999 [details] Bug 1262053 - part5 : register audio agent immediately when media element starts playing. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68606/diff/1-2/
Comment on attachment 8776995 [details] Bug 1262053 - part1 : unblock window's media when the page was first visited. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68592/diff/2-3/
Attachment #8776995 - Flags: review?(amarchesini)
Attachment #8776996 - Flags: review?(cpearce)
Attachment #8776997 - Flags: review?(cpearce)
Attachment #8776998 - Flags: review?(cpearce)
Attachment #8776999 - Flags: review?(amarchesini)
Comment on attachment 8776996 [details] Bug 1262053 - part2 : remove old media.block-play-until-visible behaviour. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68594/diff/2-3/
Comment on attachment 8776997 [details] Bug 1262053 - part3 : modify media element for blocking autoplay media. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68596/diff/2-3/
Comment on attachment 8776998 [details] Bug 1262053 - part4 : don't dispatch dom event for blocked media. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68598/diff/2-3/
Comment on attachment 8776999 [details] Bug 1262053 - part5 : register audio agent immediately when media element starts playing. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68606/diff/2-3/
Comment on attachment 8776996 [details] Bug 1262053 - part2 : remove old media.block-play-until-visible behaviour. https://reviewboard.mozilla.org/r/68594/#review67924
Attachment #8776996 - Flags: review?(cpearce) → review+
Comment on attachment 8776997 [details] Bug 1262053 - part3 : modify media element for blocking autoplay media. https://reviewboard.mozilla.org/r/68596/#review67928 ::: dom/html/HTMLMediaElement.cpp:4532 (Diff revision 3) > - mDecoder->Play(); > + mDecoder->Play(); > + } > } else if (mSrcStream) { > SetPlayedOrSeeked(true); > } > DispatchAsyncEvent(NS_LITERAL_STRING("play")); I don't think we should be dispatching the "play" event if we've blocked playback. Chrome doesn't do this when it blocks playback: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp?q=HTMLMediaElement&sq=package:chromium&l=2119 Perhaps you should instead be checking ShouldElementBePaused() in CanActivateAutoplay()?
Comment on attachment 8776997 [details] Bug 1262053 - part3 : modify media element for blocking autoplay media. https://reviewboard.mozilla.org/r/68596/#review67932
Attachment #8776997 - Flags: review?(cpearce) → review-
Comment on attachment 8776998 [details] Bug 1262053 - part4 : don't dispatch dom event for blocked media. https://reviewboard.mozilla.org/r/68598/#review67934
Attachment #8776998 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #40) > Comment on attachment 8776997 [details] > Bug 1262053 - part3 : modify media element for blocking autoplay media. > > https://reviewboard.mozilla.org/r/68596/#review67932 Somehow my comment got lost here, but I tried to say that YouTube is broken with your patches, and we can't ship this if it breaks YouTube. I built your patch, and what I observed is that if I open a YouTube link in a background tab (CTRL+CLICK) and that video had a preroll ad, when I focus the tab, the preroll ad doesn't start playing. We need that to work before we can ship this.
(In reply to Chris Pearce (:cpearce) from comment #42) > (In reply to Chris Pearce (:cpearce) from comment #40) > > Comment on attachment 8776997 [details] > > Bug 1262053 - part3 : modify media element for blocking autoplay media. > > > > https://reviewboard.mozilla.org/r/68596/#review67932 > > Somehow my comment got lost here, but I tried to say that YouTube is broken > with your patches, and we can't ship this if it breaks YouTube. > > I built your patch, and what I observed is that if I open a YouTube link in > a background tab (CTRL+CLICK) and that video had a preroll ad, when I focus > the tab, the preroll ad doesn't start playing. > > We need that to work before we can ship this. OK, I will find why it doesn't work and fix it.
Hi, Chris, The patch works well on my OSX and Linux, the Youtube can be resumed when the tab was visited even it has preload ad. I use the latest code from m-c (changeset:308559:c6518747391f). However, there is one situation the video wouldn't resumed well if the pref "dom.audiochannel.audioCompeting" is on. In this case, I forgot to request audio focus again when the tab is going to foreground. Although I guess no one would open this pref on desktop now, I would still commit another patch to fix this problem. But except this case, the video is working well on my test environment.
Flags: needinfo?(cpearce)
(In reply to Alastor Wu [:alwu] from comment #44) > Hi, Chris, > > The patch works well on my OSX and Linux, the Youtube can be resumed when > the tab was visited even it has preload ad. I use the latest code from m-c > (changeset:308559:c6518747391f). > > However, there is one situation the video wouldn't resumed well if the pref > "dom.audiochannel.audioCompeting" is on. In this case, I forgot to request > audio focus again when the tab is going to foreground. Although I guess no > one would open this pref on desktop now, I would still commit another patch > to fix this problem. > > But except this case, the video is working well on my test environment. I rebuilt your patches in an opt build, and I didn't see the problem anymore, though I didn't see any ads. I suspect our behaviour is different enough to Chrome's here, that the ads detect that they fail to play, and then skip. Can you please try to figure out how our behaviuour differs from Chrome here, and try to make it the same so that we get the ads as well?
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #45) > I rebuilt your patches in an opt build, and I didn't see the problem > anymore (So I suspect there's some issue with debug build being very slow, i.e. there's some issue here that is exacerbated by running slow due to being in a debug build.)
Hi, Chris, Sorry for the late reply. Because I don't finish setting up my windows development environment yet, I can't test that on windows now. But the ads can be showed correctly on the Linux and OSX even the behavior is different with Chrome. Now I am working on not to dispatch the "play" event when media was blocked, I will ask for review after finishing that. Also, I will try to run the patch on the debug build to see whether there is any unexpected bad effect.
If no highly technical testing is required, give me a build and I can try it out on Windows.
Comment on attachment 8776995 [details] Bug 1262053 - part1 : unblock window's media when the page was first visited. https://reviewboard.mozilla.org/r/68592/#review69146 ::: dom/base/nsDocument.h:1215 (Diff revision 3) > // Posts an event to call UpdateVisibilityState > virtual void PostVisibilityUpdateEvent() override; > > + // Since we wouldn't automatically play media from non-visited page, we need > + // to notify window when the page was first visited. > + void DoNotifyWhenFirstVisible(); ::: dom/base/nsDocument.cpp:12633 (Diff revision 3) > NewRunnableMethod(this, &nsDocument::UpdateVisibilityState); > NS_DispatchToMainThread(event); > } > > +void > +nsDocument::DoNotifyWhenFirstVisible() The name is confusing. What about: MaybeActiveMediaComponents? Or something similar.
Attachment #8776995 - Flags: review?(amarchesini) → review+
Comment on attachment 8776999 [details] Bug 1262053 - part5 : register audio agent immediately when media element starts playing. https://reviewboard.mozilla.org/r/68606/#review69150
Attachment #8776999 - Flags: review?(amarchesini) → review+
Comment on attachment 8776997 [details] Bug 1262053 - part3 : modify media element for blocking autoplay media. https://reviewboard.mozilla.org/r/68596/#review67928 > I don't think we should be dispatching the "play" event if we've blocked playback. Chrome doesn't do this when it blocks playback: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp?q=HTMLMediaElement&sq=package:chromium&l=2119 > > Perhaps you should instead be checking ShouldElementBePaused() in CanActivateAutoplay()? The ShouldElementBePaused() can't be in CanActivateAutoplay(). We need to change mPause first, and then call UpdateSrcMediaStreamPlaying() to register the audio channel agent for getting the infomation about blocking. The flow is like, (1) ready to active autoplay and change media element's internal state (2) using its internal state to check whether needs to register audio channel agent (3) if the audio channel agent exists, audio channel service would do somehing for media element (4) if the media is blocked, we wouldn't dispatch event until it's resumed (done in next patch)
(In reply to Caspy7 from comment #48) > If no highly technical testing is required, give me a build and I can try it > out on Windows. Thanks you! I would upload a build after fully testing :)
Attachment #8781443 - Flags: review?(cpearce)
Attachment #8781443 - Flags: review?(amarchesini)
Attachment #8781444 - Flags: review?(amarchesini)
Attachment #8781445 - Flags: review?(cpearce)
Attachment #8781446 - Flags: review?(cpearce)
Attachment #8781446 - Flags: review?(amarchesini)
Attachment #8781447 - Flags: review?(amarchesini)
Attachment #8781448 - Flags: review?(amarchesini)
Attachment #8776996 - Flags: review?(cpearce)
Attachment #8776997 - Flags: review?(cpearce)
Attachment #8776998 - Flags: review?(cpearce)
Attachment #8776999 - Flags: review?(amarchesini)
Comment on attachment 8776999 [details] Bug 1262053 - part5 : register audio agent immediately when media element starts playing. https://reviewboard.mozilla.org/r/68606/#review69800
Attachment #8776999 - Flags: review?(amarchesini) → review+
Comment on attachment 8781443 [details] Bug 1262053 - part6 : don't need to capture media element without audio. https://reviewboard.mozilla.org/r/70646/#review69804 ::: dom/html/HTMLMediaElement.cpp:5151 (Diff revision 1) > nsresult rv = nsGenericHTMLElement::CopyInnerTo(aDest); > NS_ENSURE_SUCCESS(rv, rv); > if (aDest->OwnerDoc()->IsStaticDocument()) { > HTMLMediaElement* dest = static_cast<HTMLMediaElement*>(aDest); > dest->mMediaInfo = mMediaInfo; > + MediaInfoChanged(); dest->MediaInfoChanged() ? Plus: what about a simple: dest->SetMediaInfo(mMediaInfo) where you set the mediaInfo and you do the rest? ::: dom/html/HTMLMediaElement.cpp:6192 (Diff revision 1) > > return false; > } > > +void > +HTMLMediaElement::MediaInfoChanged() rename this to : SetMediaInfo(MediaInfo* ..)
Comment on attachment 8781444 [details] Bug 1262053 - part7 : give audio focus for the non-visited page. https://reviewboard.mozilla.org/r/71780/#review69810
Attachment #8781444 - Flags: review?(amarchesini) → review+
Comment on attachment 8781443 [details] Bug 1262053 - part6 : don't need to capture media element without audio. https://reviewboard.mozilla.org/r/70646/#review69812
Attachment #8781443 - Flags: review?(amarchesini) → review+
Comment on attachment 8781446 [details] Bug 1262053 - part9 : add test case. https://reviewboard.mozilla.org/r/71784/#review69816 Can you tell me why it&#39;s ok to remove these if-statements?
Attachment #8781446 - Flags: review?(amarchesini)
Comment on attachment 8781447 [details] Bug 1262053 - part10 : modify tests. https://reviewboard.mozilla.org/r/71852/#review69818
Attachment #8781447 - Flags: review?(amarchesini) → review+
Comment on attachment 8781446 [details] Bug 1262053 - part9 : add test case. https://reviewboard.mozilla.org/r/71784/#review69816 Yes, I described that in the patch description. 1. mPause would always be true if having "loop" keyword 2. seeking won't affect mPause 3. having external stream doesn't mean media elment starts playing, we still need to check mPause
Comment on attachment 8781448 [details] Bug 1262053 - part11 : modify tests. https://reviewboard.mozilla.org/r/71872/#review69822
Attachment #8781448 - Flags: review?(amarchesini) → review+
NI for comment71 just in case you don't see my reply :)
Flags: needinfo?(amarchesini)
Comment on attachment 8781446 [details] Bug 1262053 - part9 : add test case. https://reviewboard.mozilla.org/r/71784/#review69838 Please, check if we have enough tests for this. In particular for the seeking thing and the stream.
Attachment #8781446 - Flags: review+
(In reply to Andrea Marchesini [:baku] from comment #74) > Please, check if we have enough tests for this. In particular for the > seeking thing and the stream. Yes, you're right. We only have "browserElement_AudioChannelSeeking.js" for the seeking case, but it was not used for "audio-playback" event. I would remove this patch and move it to bug1296163. (Also, add enough test cases in that bug.)
Flags: needinfo?(amarchesini)
Blocks: 1296163
Attachment #8781446 - Attachment is obsolete: true
Attachment #8781446 - Flags: review?(cpearce)
Attachment #8781446 - Flags: review+
Comment on attachment 8781443 [details] Bug 1262053 - part6 : don't need to capture media element without audio. https://reviewboard.mozilla.org/r/70646/#review71588 r+ with comments addressed. ::: dom/html/HTMLMediaElement.cpp:5151 (Diff revision 1) > nsresult rv = nsGenericHTMLElement::CopyInnerTo(aDest); > NS_ENSURE_SUCCESS(rv, rv); > if (aDest->OwnerDoc()->IsStaticDocument()) { > HTMLMediaElement* dest = static_cast<HTMLMediaElement*>(aDest); > dest->mMediaInfo = mMediaInfo; > + MediaInfoChanged(); I agree with both baku's comments here. Please add a SetMediaInfo() which does what MediaInfoChanged() does. ::: dom/html/HTMLMediaElement.cpp:6195 (Diff revision 1) > > +void > +HTMLMediaElement::MediaInfoChanged() > +{ > + if (HasAudio() && mAudioChannelAgent) { > + WindowAudioCaptureChanged(mAudioCapturedByWindow); The only thing that sets mAudioCapturedByWindow or mCaptureStreamPort is WindowAudioCaptureChanged() (other than the destructor). So it's weird that you call WindowAudioCaptureChanged(mAudioCapturedByWindow) here. That is, you're calling a WindowAudioCaptureChanged to set mAudioCapturedByWindow to the value that it's already set to. I'm guessing you're calling this to pickup changes when HasAudio() switches from returning false to true? I think you should factor out lines [5869,5907] into a helper function, and call that from WindowAudioCaptureChanged() and from SetMediaInfo(). Then the code should be clearer.
Attachment #8781443 - Flags: review?(cpearce) → review+
Comment on attachment 8781445 [details] Bug 1262053 - part8 : remove function NotifyOwnerDocumentActivityChangedInternal. https://reviewboard.mozilla.org/r/71782/#review71812
Attachment #8781445 - Flags: review?(cpearce) → review+
Comment on attachment 8776997 [details] Bug 1262053 - part3 : modify media element for blocking autoplay media. https://reviewboard.mozilla.org/r/68596/#review71582 ::: dom/html/HTMLMediaElement.cpp:6194 (Diff revision 4) > + return true; > + } > + > + return false; > +} > + One blank line between function definitions.
Attachment #8776997 - Flags: review?(cpearce) → review+
Attachment #8776996 - Flags: review?(cpearce) → review+
Attachment #8776998 - Flags: review?(cpearce) → review+
I'm debugging the test case fails for "test_getUserMedia_audioCapture.html", now I have no idea why it fails. In order not to be blocked by this fail, I would separate the patch6 to another bug if I can't find the way to fix it today.
Blocks: 1298777
I have already found the fail reason (because some changing of the bug1259788) and file a bug1298777 for that. I will still land the patch6 but disable the |HasAudio()| checking in AudioCaptureStreamChangeIfNeeded() to make sure the code works well.
Attachment #8781448 - Attachment is obsolete: true
Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/939cd6bbfe12 part1 : unblock window's media when the page was first visited. r=baku https://hg.mozilla.org/integration/autoland/rev/ab7f9029b66a part2 : remove old media.block-play-until-visible behaviour. r=cpearce https://hg.mozilla.org/integration/autoland/rev/9795f135b544 part3 : modify media element for blocking autoplay media. r=cpearce https://hg.mozilla.org/integration/autoland/rev/4fcb25172321 part4 : don't dispatch dom event for blocked media. r=cpearce https://hg.mozilla.org/integration/autoland/rev/4e9367f90eb3 part5 : register audio agent immediately when media element starts playing. r=baku https://hg.mozilla.org/integration/autoland/rev/e7a17f7ee53d part6 : don't need to capture media element without audio. r=baku,cpearce https://hg.mozilla.org/integration/autoland/rev/9926e73d05a7 part7 : give audio focus for the non-visited page. r=baku https://hg.mozilla.org/integration/autoland/rev/7cc913ccd1f0 part8 : remove function NotifyOwnerDocumentActivityChangedInternal. r=cpearce https://hg.mozilla.org/integration/autoland/rev/70af5e29fa34 part9 : add test case. r=baku,cpearce https://hg.mozilla.org/integration/autoland/rev/cf46024ec64e part10 : modify tests. r=baku
Blocks: 1302350
Flags: needinfo?(thsieh)
Depends on: 1305338
No longer depends on: 1305338
Blocks: delay-autoplay
Verified that the videos opened in new tabs in the background are blocked until the tab that contains them has focus. Verified as fixed using: Firefox 51.0b1 (Build ID: 20161115182233), Aurora 52.0a2 (Build ID: 20161120004006) and the latest Nightly 53.0a1 (Build ID: 20161121030224)on: Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox50: --- → unaffected
status-firefox51: fixed → verified
status-firefox52: --- → verified
status-firefox53: --- → verified
in Firefox 51 BETA this bug affects Media Source Extensions (video live streaming, etc), so 2 questions: 1. why you do not copy Chrome's method of blocking MSE (defer MSE opening, since Chrome 49)? this is most compatible and safe way because video players already supports this method. 2. on inactive tab JS calls HTMLVideoElement.play(), and later HTMLVideoElement.pause(). then tab become active, playback begins. this HTMLVideoElement behavior lead to errors in video player's logic. playback must not begin after pause().
Flags: needinfo?(alwu)
and it is a pity that this bug isn't there: https://developer.mozilla.org/en-US/Firefox/Releases/51
(In reply to CoolCmd from comment #97) > in Firefox 51 BETA this bug affects Media Source Extensions (video live > streaming, etc), so 2 questions: > > 1. why you do not copy Chrome's method of blocking MSE (defer MSE opening, > since Chrome 49)? this is most compatible and safe way because video players > already supports this method. Could you help me describe what's the problem MSE got? > 2. on inactive tab JS calls HTMLVideoElement.play(), and later > HTMLVideoElement.pause(). then tab become active, playback begins. this > HTMLVideoElement behavior lead to errors in video player's logic. playback > must not begin after pause(). Thanks for report that! I file bug1322505 to solve this issue.
Flags: needinfo?(alwu)
Release Note Request (optional, but appreciated) this is a behavioural change in firefox that isn't surfaced anywhere in the preferences UI and the visual indicator that makes it clearer for users what is happening is only in 52 with bug 1308153. should we document the change in the release notes for 51?
relnote-firefox: --- → ?
Depends on: 1330963
Oops. Chris, I wonder if we have had enough testing of this feature. Is this going to substantially change behavior that users expect? Should we wait to make this the default until you have the UI in place?
Flags: needinfo?(cpearce)
Gerry, what do you think here? n-i to you so that you can be aware of this change in 51.
Flags: needinfo?(gchang)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #102) > Oops. Chris, I wonder if we have had enough testing of this feature. Is > this going to substantially change behavior that users expect? Should we > wait to make this the default until you have the UI in place? Dolske told me that he's OK shipping this feature without UI. Dolske: are you still OK shipping this feature without UI? This feature is blocking playback of audio and video in a tab until that tab has been in he foreground. The UI would be a check box to disable this new behaviour, i.e. to allow audio/video to be played in a page opened in the background, as currently happens in Firefox 50 and older.
Flags: needinfo?(cpearce) → needinfo?(dolske)
Alastor: with your changes here, playback is broken on Amazon Prime video when it's opened in a background tab (bug 1330963). We can't ship with that. Also, it looks like you have a "play" button that appears on the tab bar for blocked media in Nightly that's not in Aurora. Given that, I think it best that we let this feature ride the trains with its tab-bar UI in 53 (Nightly), and disable this feature in Aurora and Beta. So, can you please create a patch to pref-off this feature and request uplift to aurora and beta for that patch please? Liz/gchang: How does that sound?
Flags: needinfo?(alwu)
Hi, I agree with Chris's suggestion. Given playback is broken on Amazon Prime video and we will have RC build today, I don't think we have enough time to bake. I would like let this feature ride the trains on 53. Alastor, please create a patch to pref-off this feature. Thanks.
Flags: needinfo?(gchang)
Sure.
Flags: needinfo?(alwu)
Approval Request Comment [Feature/Bug causing the regression]: Close the pref about blocking autoplay media to fix the Amazon Prime video issue [User impact if declined]: User can't watch Amazon Prime video correctly [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: This issue only happens on beta [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?]: Close the pref means we won't do any extra thing to affect media playback [String changes made/needed]: No
Attachment #8827052 - Flags: review+
Attachment #8827052 - Flags: approval-mozilla-beta?
Depends on: 1331317
Comment on attachment 8827052 [details] [diff] [review] Bug 1262053 - close pref 'media.block-autoplay-until-in-foreground' in beta. I would like to close the pref on both Beta and Aurora, and only enable the feature on Nightly. Open bug1331317 to track it.
Attachment #8827052 - Attachment is obsolete: true
Attachment #8827052 - Flags: review+
Attachment #8827052 - Flags: approval-mozilla-beta?
added to the release notes-to-be for 53.0a2 as "Media playback on new tabs is blocked until the tab is visible"
relnote-firefox: ? → 53+
(In reply to Chris Pearce (:cpearce) from comment #104) > Dolske told me that he's OK shipping this feature without UI. > > Dolske: are you still OK shipping this feature without UI? This feature is > blocking playback of audio and video in a tab until that tab has been in he > foreground. The UI would be a check box to disable this new behaviour, i.e. > to allow audio/video to be played in a page opened in the background, as > currently happens in Firefox 50 and older. Sorry, I've forgotten the original discussion. I hope it wasn't too recent, for my sake. ;) We definitely don't need UI to ship this disabled-by-default, and we definitely don't need UI to be testing it on pre-release channels. Shipping this (to Release) really depends on what the impact is... I was probably more worried about this in the distant past, when it seemed like we were still trying to find a way to do this without breaking sites and Chrome hadn't shipped their implementation. Assuming we're basically doing what Chrome does now, and in any case have had this baking in 51 for a few months without any widespread problems (afaik -- WFM, and the only broken site I see listed from this bug is Amazon Prime), I don't see any reason to have UI for it. If this was causing breakage on sites, we'd want to think hard if "Firefox is broken until you flip this obscure checkbox" is worth it (or if we should address that some other way). And if it's not causing breakage, I don't currently see the user value in adding UI.
Flags: needinfo?(dolske)
isn't bug 1308153 shipping in 52? why can't this also ship in 52?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: