Closed
Bug 1370699
Opened 7 years ago
Closed 3 years ago
Reconsider complicated machinery for releasing video buffers
Categories
(Firefox for Android Graveyard :: Audio/Video, enhancement, P5)
Firefox for Android Graveyard
Audio/Video
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: snorp, Unassigned)
Details
Right now we have some callback stuff that is used to call MediaCodec.releaseOutputBuffer() close to the presentation time for that frame. I believe the initial reason for that is because the compositor was calling SurfaceTexture.updateTexImage() on every composite, and if there was another buffer already queued it would be advanced and presented -- possibly sooner than we want. The SurfaceTexture painting has been changed to only call updateTexImage() once per transaction now, so is this machinery still necessary? It seems like things should work correctly now with the existing sync mechanisms.
Comment 2•7 years ago
|
||
I think the callback is still needed. The issue it addresses is that Surface only keeps one buffer and if releaseOutputBuffer() is called multiple times between two transactions, all but the final frame still disappear. The new 'single buffer mode' flag supported since API 19 may change the behavior of Surface and make callback unnecessary, but it will - consume more graphic buffer memory - block MediaCodec when Gecko hold too many buffers, like what we found in bug 1009420 - require calling TextureSurface.releaseTexImage() when compositor is done with each frame, which you took care already in SurfaceTextureHost::NotifyNotUsed() One improvement(?) to current callback design, IMHO, is moving the callback from VideoData to GeckoSurfaceTexture and call it before updateTexImage() (at the expense of blocking compositor thread rather than media thread).
Flags: needinfo?(jolin)
Comment 3•7 years ago
|
||
Oops. Turns out I was wrong about 'single buffer mode'. When it's on, TextureSurface will force the surface to allocate only one buffer [1]. [1] https://android.googlesource.com/platform/frameworks/base/+/master/core/jni/android/graphics/SurfaceTexture.cpp#263
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to John Lin [:jolin][:jhlin] from comment #2) > I think the callback is still needed. The issue it addresses is that Surface > only keeps one buffer and if releaseOutputBuffer() is called multiple times > between two transactions, all but the final frame still disappear. The single-buffer mode has only one buffer, but the default configuration (which we are using now) has up to three buffers. I don't believe it will drop any frames.
Comment 5•7 years ago
|
||
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4) > (In reply to John Lin [:jolin][:jhlin] from comment #2) > > I think the callback is still needed. The issue it addresses is that Surface > > only keeps one buffer and if releaseOutputBuffer() is called multiple times > > between two transactions, all but the final frame still disappear. > > The single-buffer mode has only one buffer, but the default configuration > (which we are using now) has up to three buffers. I don't believe it will > drop any frames. You're right that Surface has more than one buffers. The problem is that it has only one queued buffer, which updateTexImage() acquires for texture contents. FWICT, a queued buffer could be dropped at [1]. In our case BufferItem::mIsDroppable is set [2] due to BufferQueueCore::mDequeueBufferCannotBlock, which is set at [3] because BufferQueueCore::mConsumerControlledByApp is set by SurfaceTexture for non-single buffer mode [4]; producerControlledByApp by Surface at [5]. [1] https://android.googlesource.com/platform/frameworks/native/+/master/libs/gui/BufferQueueProducer.cpp#868 [2] https://android.googlesource.com/platform/frameworks/native/+/master/libs/gui/BufferQueueProducer.cpp#844 [3] https://android.googlesource.com/platform/frameworks/native/+/master/libs/gui/BufferQueueProducer.cpp#1144 [4] https://android.googlesource.com/platform/frameworks/base/+/master/core/jni/android/graphics/SurfaceTexture.cpp#268 [5] https://android.googlesource.com/platform/frameworks/base/+/master/core/jni/android_view_Surface.cpp#245
Updated•7 years ago
|
Priority: -- → P3
Comment 6•6 years ago
|
||
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195 Needinfo :susheel if you think this bug should be re-triaged.
Priority: P3 → P5
Comment 7•3 years ago
|
||
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
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
•