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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, enhancement, P3)

Description

[Alastor Wu [:alwu]]

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

Comment 1

[Alastor Wu [:alwu]]

You can also open this test page, the video can't start playback successfully on Android.
https://people-mozilla.org/~alwu/textuteTest.html

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1373177 - part1 : automatically refill the pool if the textures number is lower than specific threshold.

The pool would be refilled when the compositor starts rendering the frame.
However, if we consume lots of textures but doesn't render any frame, then the
pool would always empty and no one can get new texture id.

One case is that when we release decoders too early before rendering the first
frame, the pool won't be refilled, and it causes all media threads are blocked.

Now we would refill the pool if the textures number is lower than the specific
threshold.

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

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1373177 - part2 : add log.

Add lazy log for debugging.

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

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1373177 - part3 : enable test.

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

Comment 5

[Alastor Wu [:alwu]]

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

Comment 6

[Alastor Wu [:alwu]]

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

Comment 7

[Benjamin Chen [:bechen]]

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.

Comment 8

[Alastor Wu [:alwu]]

(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 9

[Alastor Wu [:alwu]]

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

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

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8878334 [details]
Bug 1373177 - part2 : add log.

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

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8878335 [details]
Bug 1373177 - part3 : enable test.

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

Comment 12

[Benjamin Chen [:bechen]]

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 13

[Alastor Wu [:alwu]]

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

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

Comment 14

[Alastor Wu [:alwu]]

Comment on attachment 8878334 [details]
Bug 1373177 - part2 : add log.

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

Comment 15

[Alastor Wu [:alwu]]

Comment on attachment 8878335 [details]
Bug 1373177 - part3 : enable test.

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

Comment 16

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9396e7acfbfc4405a44be2ffb159ec5e734975c1&selectedJob=107606984

Comment 17

[Benjamin Chen [:bechen]]

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

Comment 18

[John Lin [:jhlin][:jolin]]

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()`.

Comment 19

[John Lin [:jhlin][:jolin]]

Comment on attachment 8878334 [details]
Bug 1373177 - part2 : add log.

https://reviewboard.mozilla.org/r/149648/#review154860

Comment 20

[John Lin [:jhlin][:jolin]]

Comment on attachment 8878335 [details]
Bug 1373177 - part3 : enable test.

https://reviewboard.mozilla.org/r/149650/#review154862

Comment 21

[Alastor Wu [:alwu]]

review ping?
Thanks!

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 23

[Alastor Wu [:alwu]]

Attachment: Bug 1373177 - part4 : add assertion to make sure the function call order.

The Init()/Shutdown() would be ran on the main thread, AcquireTexture() would on
media thread and Fill() would be on compostitor thread.

In order to avoid race condition, we would always require lock when running those
function.

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

Comment 24

[Alastor Wu [:alwu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149646/diff/3-4/

Comment 25

[Alastor Wu [:alwu]]

Comment on attachment 8878334 [details]
Bug 1373177 - part2 : add log.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149648/diff/3-4/

Comment 26

[Alastor Wu [:alwu]]

Comment on attachment 8878335 [details]
Bug 1373177 - part3 : enable test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149650/diff/3-4/

Comment 27

[Alastor Wu [:alwu]]

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

Comment 28

[Alastor Wu [:alwu]]

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

Comment 29

[Alastor Wu [:alwu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149646/diff/4-5/

Comment 30

[Alastor Wu [:alwu]]

Comment on attachment 8878334 [details]
Bug 1373177 - part2 : add log.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149648/diff/4-5/

Comment 31

[Alastor Wu [:alwu]]

Comment on attachment 8878335 [details]
Bug 1373177 - part3 : enable test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149650/diff/4-5/

Comment 32

[Alastor Wu [:alwu]]

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

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

Comment 33

[Alastor Wu [:alwu]]

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

Comment 34

[Alastor Wu [:alwu]]

See some failures on try-server, need to verify whether the crash is caused by my patch.

Comment 35

[Alastor Wu [:alwu]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149646/diff/5-6/

Comment 36

[Alastor Wu [:alwu]]

Comment on attachment 8878334 [details]
Bug 1373177 - part2 : add log.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149648/diff/5-6/

Comment 37

[Alastor Wu [:alwu]]

Comment on attachment 8878335 [details]
Bug 1373177 - part3 : enable test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149650/diff/5-6/

Comment 38

[Alastor Wu [:alwu]]

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

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

Comment 39

[Alastor Wu [:alwu]]

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

Comment 40

[John Lin [:jhlin][:jolin]]

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

https://reviewboard.mozilla.org/r/152976/#review158926

Comment 41

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

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