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)
Firefox for Android Graveyard
Audio/Video
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
Assignee | ||
Comment 1•7 years ago
|
||
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) |
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f59f397ccd1d87ad82804e446589d38260761aa7
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d215f7df0a91f96bb7f19f3f48ea7a54c3f2e8c0
Comment 7•7 years 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.
Assignee | ||
Comment 8•7 years ago
|
||
(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•7 years 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) |
Assignee | ||
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9396e7acfbfc4405a44be2ffb159ec5e734975c1&selectedJob=107606984
Comment 17•7 years 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•7 years 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+
Assignee | ||
Updated•7 years ago
|
Attachment #8878333 -
Flags: review?(snorp)
Comment 19•7 years 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•7 years 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+
Updated•7 years ago
|
Priority: -- → P3
Comment 22•7 years 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) |
Assignee | ||
Comment 27•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89e7ba870ff7d17483d45f5f4360cef023d3405d
Assignee | ||
Comment 28•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=002c60ea8c108aa14ab06b534160ad9ffda23643
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(snorp)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bc2a28720add1ea6335526de418af6c060359f0
Assignee | ||
Comment 34•7 years ago
|
||
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) |
Assignee | ||
Comment 39•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f3f8afc295eadf5107d02b546d76adfad80a2b8
Comment 40•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•