Closed Bug 1373177 Opened 7 years ago Closed 7 years ago

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

Categories

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

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(4 files)

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 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 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 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 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 on attachment 8878334 [details]
Bug 1373177 - part2 : add log.

https://reviewboard.mozilla.org/r/149648/#review154860
Attachment #8878334 - Flags: review?(jolin) → 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 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+
Flags: needinfo?(snorp)
See some failures on try-server, need to verify whether the crash is caused by my patch.
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+
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
Blocks: 1375389
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: