1188155 - [Browser][Youtube]Only one window with a Youtube video will correctly load

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Attachment: youtube_tabs_logcat

Description:
If the user has an open Youtube video window, other windows will get an unending loading icon instead.  


Repro Steps:
1) Update a Flame to 20150727091245
2) Open the browser and load any youtube video
3) Return to the homescreen
4) Open another browser window to anoter youtube video


Actual:
The second video will not begin playing, instead showing a loading screen forever


Expected:
The second video will play.


Notes: Once each on both Flame and Aries, closing down the original browser (with the playing video) caused the phone to restart.  I was unable to reproduce this issue on all subsequent attempts.

Environmental Variables:
Device: Flame 2.5
Build ID: 20150727091245
Gaia: 302a448729ff2b336581cf94b66327ea836294c7
Gecko: d576f2cec511
Version: 42.0a1 (2.5)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Repro frequency: 10/10
See attached: logcat video

Comment 1

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue also occurs on the latest Aries spark build.

Actual Results: Subsequent windows of Youtube do not load the video.

Environmental Variables:
Device: Aries 2.5
BuildID: 20150727165113
Gaia: 302a448729ff2b336581cf94b66327ea836294c7
Gecko: d576f2cec511
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 42.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0


This issue does NOT occur on Flame 2.2.

Actual Results: Videos on other windows will load as expected.

Environmental Variables:
Device: Flame 2.2
BuildID: 20150727032503
Gaia: e1e6317f17a840b19af9dbb25f5a771d8d9fa161
Gecko: 551aee22a380
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

[Blocking Requested - why for this release]:

Regression so nominating this 2.5?

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

The changes for Bug 1166169 may have caused this issue.

B2g-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150522064751
Gaia: db93a2264a4ed0092340ccd6892237c6b32cb96b
Gecko: df59619db603
Version: 41.0a1 (2.5) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

First Broken 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150522071550
Gaia: db93a2264a4ed0092340ccd6892237c6b32cb96b
Gecko: 0cf2984f2c59
Version: 41.0a1 (2.5) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Last Working gaia / First Broken gecko - Issue DOES occur
Gaia: db93a2264a4ed0092340ccd6892237c6b32cb96b
Gecko: 0cf2984f2c59

First Broken gaia / Last Working gecko - Issue does NOT occur
Gaia: db93a2264a4ed0092340ccd6892237c6b32cb96b
Gecko: df59619db603

Gecko Pushlog:  http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=df59619db603&tochange=0cf2984f2c59

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Blake, can you take a look at this please? This looks to have been caused by the landing for bug 1166169.

Comment 5

[Blake Wu [:bwu][:blakewu]]

Benjamin, 
Could you help check it? 
Thanks!

Comment 6

[Benjamin Chen [:bechen]]

Seems that the MediaSourceDecoder doesn't set the mDormantSupported to true.

Comment 7

[Benjamin Chen [:bechen]]

Attachment: bug-1188155.v01.patch

Implement the |SharedDecoderManager::ReleaseMediaResources()|, I just test the patch on my Flame, seems works.

Comment 8

[Alfredo Yang (:alfredo)]

Comment on attachment 8642285 [details] [diff] [review]
bug-1188155.v01.patch

Review of attachment 8642285 [details] [diff] [review]:
-----------------------------------------------------------------

I think the idea of SharedDecoderManager is to avoid release/reallocate the decoder in MediaDataDecoder directly. In other word, the lifetime of hw decoder should be controlled by SharedDecoderManager, not by SharedDecoderProxy. It'd be better to release the hw deocder in the class which creates/controls SharedDecoderManager. And keep SharedDecoderProxy's original design.

::: dom/media/mediasource/MediaSourceDecoder.cpp
@@ +34,5 @@
>    : mMediaSource(nullptr)
>    , mIsUsingFormatReader(Preferences::GetBool("media.mediasource.format-reader", false))
>    , mEnded(false)
>  {
> +  mDormantSupported = true;

It should be controlled by "media.decoder.heuristic.dormant.enabled".

Comment 9

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8642285 [details] [diff] [review]
bug-1188155.v01.patch

Review of attachment 8642285 [details] [diff] [review]:
-----------------------------------------------------------------

I think today you have found a better solution!

Comment 10

[Benjamin Chen [:bechen]]

Attachment: bug-1188155.v02.patch

Enable the |mDormantSupported| by modify moz.build.
https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/SourceBufferContentManager.cpp?from=SourceBufferContentManager.cpp&case=true#39

Comment 11

[Alfredo Yang (:alfredo)]

Comment on attachment 8643460 [details] [diff] [review]
bug-1188155.v02.patch

Review of attachment 8643460 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 12

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8643460 [details] [diff] [review]
bug-1188155.v02.patch

Review of attachment 8643460 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaFormatReader.cpp
@@ +1503,4 @@
>    if (mVideo.mDecoder) {
>      mVideo.mDecoder->Shutdown();
>      mVideo.mDecoder = nullptr;
>    }

With your changes, the above codes may not be required when using SharedDecoderManager, right?
If yes, maybe we could make codes more simple.

Comment 13

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8643460 [details] [diff] [review]
bug-1188155.v02.patch

Review of attachment 8643460 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it looks good to me. 
Just have one suggestion.

::: dom/media/MediaFormatReader.cpp
@@ +1503,4 @@
>    if (mVideo.mDecoder) {
>      mVideo.mDecoder->Shutdown();
>      mVideo.mDecoder = nullptr;
>    }

if (mSharedDecoderManager) {
  mSharedDecoderManager->ReleaseMediaResources();
} else if (mVideo.mDecoder) {
     mVideo.mDecoder->Shutdown();
     mVideo.mDecoder = nullptr;
}

Comment 14

[Alfredo Yang (:alfredo)]

Comment on attachment 8643460 [details] [diff] [review]
bug-1188155.v02.patch

Review of attachment 8643460 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaFormatReader.cpp
@@ +1498,5 @@
>      container->ClearCurrentFrame();
>    }
> +  if (mSharedDecoderManager) {
> +    mSharedDecoderManager->ReleaseMediaResources();
> +  }

BTW, I think it'd be better to move it after "mVideo.mDecoder->Shutdown()".
Shut down decoder first and then releasing hardware resource.

Comment 15

[Benjamin Chen [:bechen]]

Attachment: bug-1188155.v02.patch

Address comment 14.

Comment 16

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

Comment on attachment 8644204 [details] [diff] [review]
bug-1188155.v02.patch

Review of attachment 8644204 [details] [diff] [review]:
-----------------------------------------------------------------

jya should review this.

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8644204 [details] [diff] [review]
bug-1188155.v02.patch

Review of attachment 8644204 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see the point of fixing this when SharedDecoderManager is going and this will be automatically fixed with bug 1187542 as the new mediasource doesn't suffer this fate.

So it now becomes a strategy matter: do we bother modifying code that will be removed and won't be use in a matter of days?

Passing on to Anthony

::: dom/media/mediasource/moz.build
@@ +37,5 @@
>  ]
>  
> +if CONFIG['MOZ_GONK_MEDIACODEC']:
> +    DEFINES['MOZ_GONK_MEDIACODEC'] = True
> +

shouldn't this be separate ?
I don't see much the relevance to MediaFormatReader / MediaSourceReader / SharedDecoderManager.

This seems important enough that it should be in its own patch.

::: dom/media/platforms/SharedDecoderManager.cpp
@@ +201,5 @@
> +    mDecoder->Shutdown();
> +    mDecoder = nullptr;
> +  }
> +}
> +

you now have duplicated code with Shutdown().

Remove the duplicate and call ReleaseMediaResources in Shutdown()

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8644204 [details] [diff] [review]
bug-1188155.v02.patch

Review of attachment 8644204 [details] [diff] [review]:
-----------------------------------------------------------------

Fixing this is most likely dead time. I'd rather see you spending time getting the new MSE implementation enabled on Firefox OS because this code path is no longer supported. We can't ship the old implementation in Firefox OS 2.5.

Comment 19

[Benjamin Chen [:bechen]]

Attachment: bug-1188155.mozbuild.patch

Since bug 1187542 will implement a new MSE model, so I only check-in the moz.build patch.

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

Comment 20

[Benjamin Chen [:bechen]]

Attachment: bug-1188155.mozbuild.patch

Rebase.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/373f429e2549

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/373f429e2549

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/373f429e2549

Comment 24

[Martin Shuman [:Marty]]

This issue is verified fixed on the latest Flame and Aries 2.5 Master builds.
The user is able to open multiple YouTube videos simultaneously, and switch between active windows and view the videos interchangeably.

Environmental Variables:
Device: Aries 2.5
BuildID: 20150917133514
Gaia: aede8622d780ec71f766a3ecccbff74c04aaba4e
Gecko: 8d4c37a86b576db4ef150dff715a89ff77e84bf5
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 43.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Environmental Variables:
Device: Flame 2.5
BuildID: 20150917030226
Gaia: db6664f0e07e9966283d30cfc7006151fe7103ff
Gecko: e7d613b3bcfe1e865378bfac37de64560d1234ec
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 43.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0