1272422 - Sound playbacks from start when click tabs, audio and video are completely out of sync.

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Youtube broken

Steps to Reproduce:
1. Open any youtube video(e.g. https://www.youtube.com/watch?v=BH0BNbwqNK8 ) in tab[A]
2. Open any page in a new tab[B]
3. Switch between tabs by (double)clicking tab and repeat Step3

Actual Results:
Sound playbacks from start.
Video picture is no problem. So completely out of sync.


Regression Window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=13e4cb81d4eb1c4996daf34c68301ca8a8ab0183&tochange=2fe467985f04512869ca7daa1a2db286642ce127

Regressed by: Bug 1224973

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Dan: Can you look into this please?

Comment 2

[u480271]

Sure can. Looks like it's caused by the resuming from suspended video decoders, as it can be worked around by setting pref "media.suspend-bkgnd-video.enabled" to false.

Comment 4

[u480271]

Attachment: MozReview Request: Bug 1272422 - Part 1: Expose control of suspending background video. r?cpearce

This will show media.suspend-bkgnd-video.enabled to users.

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

Comment 5

[u480271]

Attachment: MozReview Request: Bug 1272422 - Part 2: Vidoe -> Video. r?cpearce

Fix spelling of video.

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

Comment 6

[u480271]

Attachment: MozReview Request: Bug 1272422 - Part 3: Don't reset audio queue. r?jya

When resuming from suspended video, don't reset the audio queues or
audio demuxing state in MFR.

ResetDemuxers() is being called via MFR::ResetDecode() on seek setup via
MDSM::InitiateSeek(), so don't call it again from MFR::AttemptSeek().

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

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8752012 [details]
MozReview Request: Bug 1272422 - Part 1: Expose control of suspending background video. r?cpearce

https://reviewboard.mozilla.org/r/52373/#review49305

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8752013 [details]
MozReview Request: Bug 1272422 - Part 2: Vidoe -> Video. r?cpearce

https://reviewboard.mozilla.org/r/52375/#review49307

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8752014 [details]
MozReview Request: Bug 1272422 - Part 3: Don't reset audio queue. r?jya

https://reviewboard.mozilla.org/r/52377/#review49309

::: dom/media/MediaFormatReader.cpp:1535
(Diff revision 1)
>    mSeekScheduled = false;
>  
>    if (mPendingSeekTime.isNothing()) {
>      return;
>    }
> -  ResetDemuxers();
> +  // Bug 1272422: ResetDemuxers is already called via ResetDecode(), which is

I would prefer if you renamed ResetDemuxers to something else. The name doesn't indicate it has anything to do with the seek target.

And please, no commented code like that

::: dom/media/MediaFormatReader.cpp:1538
(Diff revision 1)
>      return;
>    }
> -  ResetDemuxers();
> +  // Bug 1272422: ResetDemuxers is already called via ResetDecode(), which is
> +  // invoked before Seek() is called via MDSM InitiateSeek(). Don't call it
> +  // again.
> +  //ResetDemuxers();

You will get assertions here that mVideo/mAudio.mSeekRequest isn't empty. In particular in the mochitest testing multiple seeks in a row quickly.

The ResetDemuxers also happen to disconnect any pending seek requests.

I hit that during the refactoring.

So you do need to reset the demuxers. or at least disconnect the promises.

And please, dont comment code, just remove it. (but here you don't)

Comment 10

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

Comment on attachment 8752014 [details]
MozReview Request: Bug 1272422 - Part 3: Don't reset audio queue. r?jya

Not familiar with MFR. Will leave the review of MediaFormatReader.* to jya.

Comment 11

[u480271]

Comment on attachment 8752014 [details]
MozReview Request: Bug 1272422 - Part 3: Don't reset audio queue. r?jya

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

Comment 12

[u480271]

https://reviewboard.mozilla.org/r/52377/#review49309

> I would prefer if you renamed ResetDemuxers to something else. The name doesn't indicate it has anything to do with the seek target.
> 
> And please, no commented code like that

I wasn't going to land the comment, it there was to aid review. Instead of you just seeing the call removed and wondering "why?"

> You will get assertions here that mVideo/mAudio.mSeekRequest isn't empty. In particular in the mochitest testing multiple seeks in a row quickly.
> 
> The ResetDemuxers also happen to disconnect any pending seek requests.
> 
> I hit that during the refactoring.
> 
> So you do need to reset the demuxers. or at least disconnect the promises.
> 
> And please, dont comment code, just remove it. (but here you don't)

Spoke to jya on IRC and implemented his suggestion.

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8752014 [details]
MozReview Request: Bug 1272422 - Part 3: Don't reset audio queue. r?jya

https://reviewboard.mozilla.org/r/52377/#review49315

::: dom/media/MediaFormatReader.cpp:1533
(Diff revisions 1 - 2)
> +  }
> +
> +  // Don't reset the audio demuxer not state when seeking video only
> +  // as it will cause the audio to seek back to the beginning
> +  // resulting in out-of-sync audio from video.
> +  if (HasAudio() && !mOriginalSeekTarget->IsVideoOnly()) {

Does thie compile? mOriginalSeekTime is a maybe  

Im guessing its missing a ref()

Comment 14

[u480271]

https://reviewboard.mozilla.org/r/52377/#review49315

> Does thie compile? mOriginalSeekTime is a maybe  
> 
> Im guessing its missing a ref()

Yes. operator ->() is short hand for &.ref()

https://dxr.mozilla.org/mozilla-central/source/mfbt/Maybe.h#246

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f1bc6cbd5bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/91be5a3cd5bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/74fcb8f55064

Comment 17

[Marcia Knous [:marcia]]

Definitely tracking for 49 as videos need to be in sync.

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/5f1bc6cbd5bd
https://hg.mozilla.org/mozilla-central/rev/91be5a3cd5bd
https://hg.mozilla.org/mozilla-central/rev/74fcb8f55064

Comment 23

[Camelia Badau [:cbadau], Desktop QA]

I've tested on Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.9.5 using Firefox 49 Beta 2 (buildID: 20160808002253) and I didn't see the issue anymore (I've tested on several Youtube videos).