Closed Bug 1188155 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla43
blocking-b2g 2.5+
Tracking Status
firefox43 --- fixed
b2g-v2.2 --- unaffected
b2g-master --- verified

People

(Reporter: JMercado, Assigned: bechen)

References

()

Details

(Keywords: regression, Whiteboard: [2.5-Daily-Testing])

Attachments

(2 files, 4 obsolete files)

Attached file 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
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: regression
[Blocking Requested - why for this release]:

Regression so nominating this 2.5?
blocking-b2g: --- → 2.5?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
QA Contact: jmercado
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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Blake, can you take a look at this please? This looks to have been caused by the landing for bug 1166169.
Blocks: 1166169
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(bwu)
Benjamin, 
Could you help check it? 
Thanks!
Flags: needinfo?(bwu) → needinfo?(bechen)
Assignee: nobody → bechen
Flags: needinfo?(bechen)
Seems that the MediaSourceDecoder doesn't set the mDormantSupported to true.
Attached patch bug-1188155.v01.patch (obsolete) — Splinter Review
Implement the |SharedDecoderManager::ReleaseMediaResources()|, I just test the patch on my Flame, seems works.
Attachment #8642285 - Flags: feedback?(bwu)
Attachment #8642285 - Flags: feedback?(ayang)
blocking-b2g: 2.5? → 2.5+
Component: Gaia::Browser → Audio/Video
Product: Firefox OS → Core
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".
Attachment #8642285 - Flags: feedback?(ayang)
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!
Attachment #8642285 - Flags: feedback?(bwu)
Blocks: 1188153
Attached patch bug-1188155.v02.patch (obsolete) — Splinter Review
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
Attachment #8642285 - Attachment is obsolete: true
Attachment #8643460 - Flags: feedback?(bwu)
Attachment #8643460 - Flags: feedback?(ayang)
Comment on attachment 8643460 [details] [diff] [review]
bug-1188155.v02.patch

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

LGTM
Attachment #8643460 - Flags: feedback?(ayang) → feedback+
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 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;
}
Attachment #8643460 - Flags: feedback?(bwu) → feedback+
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.
Attached patch bug-1188155.v02.patch (obsolete) — Splinter Review
Address comment 14.
Attachment #8643460 - Attachment is obsolete: true
Attachment #8644204 - Flags: review?(cpearce)
Comment on attachment 8644204 [details] [diff] [review]
bug-1188155.v02.patch

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

jya should review this.
Attachment #8644204 - Flags: review?(cpearce) → review?(jyavenard)
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()
Attachment #8644204 - Flags: review?(jyavenard) → review?(ajones)
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.
Attachment #8644204 - Flags: review?(ajones) → review+
Attached patch bug-1188155.mozbuild.patch (obsolete) — Splinter Review
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
Attachment #8644204 - Attachment is obsolete: true
Attachment #8644845 - Flags: review+
Depends on: 1187542
Keywords: checkin-needed
Rebase.
Attachment #8644845 - Attachment is obsolete: true
Attachment #8646850 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/373f429e2549
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
You need to log in before you can comment on or make changes to this bug.