Reconsider complicated machinery for releasing video buffers

NEW
Unassigned

Status

()

Firefox for Android
Audio/Video
P3
normal
11 months ago
10 months ago

People

(Reporter: snorp, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

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

Updated

10 months ago
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.