Enable dom/media/test/test_media_selection.html on Android

RESOLVED FIXED in Firefox 56

Status

()

Firefox for Android
Audio/Video
P3
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

This test is permanent fail on Autophone.

The root cause is that the decoder can't get the texture id during creating the surface texture [1] since no one refills the texture pool [2].

The pool would be refilled when the compositor starts rendering the frame. If we release the decoder too early prior rending the first frame, the pool won't be refilled, and it causes all media threads are blocked.

[1] http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/GeckoSurfaceTexture.java#133

[2] http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/gfx/layers/opengl/TexturePoolOGL.cpp#94
You can also open this test page, the video can't start playback successfully on Android.
https://people-mozilla.org/~alwu/textuteTest.html
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f59f397ccd1d87ad82804e446589d38260761aa7
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d215f7df0a91f96bb7f19f3f48ea7a54c3f2e8c0

Comment 7

6 months ago
mozreview-review
Comment on attachment 8878333 [details]
Bug 1373177 - part1 : automatically refill the pool if the textures number is lower than specific threshold.

https://reviewboard.mozilla.org/r/149646/#review154338

Since the TextureFiller is also a static class, is it better to move all code into TexturePoolOGL? Also remember to remove the Fill() function in BeginFrame.
(In reply to Benjamin Chen [:bechen] from comment #7)
> Since the TextureFiller is also a static class, is it better to move all
> code into TexturePoolOGL? 
Ok.

> Also remember to remove the Fill() function in BeginFrame.
We still need to call Fill() in BeginFrame() because we need to know the active GL context.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

6 months ago
mozreview-review
Comment on attachment 8878333 [details]
Bug 1373177 - part1 : automatically refill the pool if the textures number is lower than specific threshold.

https://reviewboard.mozilla.org/r/149646/#review154366

::: gfx/layers/opengl/TexturePoolOGL.cpp:134
(Diff revision 2)
>    if (sTextures->GetSize() == TEXTURE_POOL_SIZE)
>      return;

Since the Fill() only be called by MaybeFillTextures(), we should remove the early return and ASSERT(sTextures->GetSize() < TEXTURE_POOL_SIZE/2);
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9396e7acfbfc4405a44be2ffb159ec5e734975c1&selectedJob=107606984

Comment 17

6 months ago
mozreview-review
Comment on attachment 8878333 [details]
Bug 1373177 - part1 : automatically refill the pool if the textures number is lower than specific threshold.

https://reviewboard.mozilla.org/r/149646/#review154386
Attachment #8878333 - Flags: review?(bechen) → review+

Comment 18

6 months ago
mozreview-review
Comment on attachment 8878333 [details]
Bug 1373177 - part1 : automatically refill the pool if the textures number is lower than specific threshold.

https://reviewboard.mozilla.org/r/149646/#review154820

Look good to me, but I think snorp should have a look at this change since he's the author of this machinary.

::: gfx/layers/opengl/CompositorOGL.cpp:682
(Diff revision 3)
>  
>    mPixelsPerFrame = width * height;
>    mPixelsFilled = 0;
>  
>  #ifdef MOZ_WIDGET_ANDROID
> -  TexturePoolOGL::Fill(gl());
> +  TexturePoolOGL::SetGLContext(gl());

Suggest to keep `Fill()` here to reduce the chances of dispatching tasks.

::: gfx/layers/opengl/TexturePoolOGL.cpp:31
(Diff revision 3)
>  
>  static Monitor* sMonitor = nullptr;
>  static nsDeque* sTextures = nullptr;
>  
> +static bool sDispatchedFillTask = false;
> +static bool sIsPoolShutdown = false;

How about setting `sTextures` to `nullptr` in `Shutdown` and use it to check the validness of pool?

::: gfx/layers/opengl/TexturePoolOGL.cpp:46
(Diff revision 3)
>    }
>  };
>  
>  #endif // MOZ_WIDGET_ANDROID
>  
> +void TexturePoolOGL::NotifyStartedFillTask()

Nit: just reset `sDispatchedFillTask` in `Fill()`.

::: gfx/layers/opengl/TexturePoolOGL.cpp:60
(Diff revision 3)
> +    sDispatchedFillTask = true;
> +    MessageLoop* loop = mozilla::layers::CompositorThreadHolder::Loop();
> +    MOZ_ASSERT(loop);
> +    loop->PostTask(
> +      NS_NewRunnableFunction([] () {
> +        if (!sIsPoolShutdown) {

Nit: move the check into `Fill()`.
Attachment #8878333 - Flags: review?(jolin) → review+
Attachment #8878333 - Flags: review?(snorp)

Comment 19

6 months ago
mozreview-review
Comment on attachment 8878334 [details]
Bug 1373177 - part2 : add log.

https://reviewboard.mozilla.org/r/149648/#review154860
Attachment #8878334 - Flags: review?(jolin) → review+

Comment 20

6 months ago
mozreview-review
Comment on attachment 8878335 [details]
Bug 1373177 - part3 : enable test.

https://reviewboard.mozilla.org/r/149650/#review154862
Attachment #8878335 - Flags: review?(jolin) → review+
Priority: -- → P3
review ping?
Thanks!
Flags: needinfo?(snorp)

Comment 22

6 months ago
mozreview-review
Comment on attachment 8878333 [details]
Bug 1373177 - part1 : automatically refill the pool if the textures number is lower than specific threshold.

https://reviewboard.mozilla.org/r/149646/#review157886

lgtm with jolin's comments
Attachment #8878333 - Flags: review?(snorp) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89e7ba870ff7d17483d45f5f4360cef023d3405d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=002c60ea8c108aa14ab06b534160ad9ffda23643
Flags: needinfo?(snorp)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bc2a28720add1ea6335526de418af6c060359f0
See some failures on try-server, need to verify whether the crash is caused by my patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f3f8afc295eadf5107d02b546d76adfad80a2b8

Comment 40

5 months ago
mozreview-review
Comment on attachment 8881966 [details]
Bug 1373177 - part4 : add assertion to make sure the function call order.

https://reviewboard.mozilla.org/r/152976/#review158926
Attachment #8881966 - Flags: review?(jolin) → review+

Comment 41

5 months ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/149e703be0d3
part1 : automatically refill the pool if the textures number is lower than specific threshold. r=bechen,jolin,snorp
https://hg.mozilla.org/integration/autoland/rev/a031db8b171a
part2 : add log. r=jolin
https://hg.mozilla.org/integration/autoland/rev/ab3d611b6768
part3 : enable test. r=jolin
https://hg.mozilla.org/integration/autoland/rev/f8d7a8b317e3
part4 : add assertion to make sure the function call order. r=jolin

Comment 42

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/149e703be0d3
https://hg.mozilla.org/mozilla-central/rev/a031db8b171a
https://hg.mozilla.org/mozilla-central/rev/ab3d611b6768
https://hg.mozilla.org/mozilla-central/rev/f8d7a8b317e3
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Blocks: 1375389
You need to log in before you can comment on or make changes to this bug.