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)

enhancement

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.
John, what do you think?
Flags: needinfo?(jolin)
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)
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
(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.
(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
Priority: -- → P3
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
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.