fix(🤖): copy the video frame on Android again - #4026
Open
dennytosp wants to merge 1 commit into
Open
Conversation
copyFrameOnAndroid exists because Android invalidates the decoder's texture as soon as the next frame is produced, so the frame has to be copied off the GPU before it is handed to the renderer. The copy was commented out in Shopify#3686, which left the worklet assigning the texture back to itself and then disposing it - the renderer gets a disposed image and the canvas stays black, with logcat repeating "EGLConsumer is not attached to an OpenGL ES context". Restore the call. Reported on a Pixel 9 and a Galaxy S23, where the frame diff goes from a flat 0 to 17-40/255 with the copy back in place. Fixes Shopify#4000
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4000.
Restores the copy in
copyFrameOnAndroid, which is currently commented out:As written it assigns the image back to itself and then disposes it, so the renderer is handed a disposed image on every frame.
Why the copy is required. The Android frame is a borrowed view of a recycled buffer, end to end:
RNSkVideo.java#nextImage()doesimageReader.acquireLatestImage()→image.getHardwareBuffer()→image.close(), which returns the slot to theImageReader's pool for the nextacquireLatestImage().RNSkAndroidVideo::nextImagepasses thatAHardwareBuffertoOpenGLContext::MakeImageFromBuffer.GR_GL_TEXTURE_EXTERNALbackend texture over the buffer and wraps it withSkImages::BorrowTextureFrom— theSkImageowns no pixels of its own.So the image is only valid until the decoder reuses that slot, which is what the surviving comment ("on android we need to copy the texture before it's invalidated") is about, and what
makeNonTextureImage()does — it reads the frame back off the recycled GPU buffer. That also explains the reported logcat,HardwareBuffer.closefollowed byEGLConsumer is not attached to an OpenGL ES context.Where it went.
git log -L 19,29:packages/skia/src/external/reanimated/useVideo.tsputs the change in #3686, a WebGPU-canvas PR that touches nothing else about video — a leftover local edit rather than an intended behaviour change.Field measurements from the reporter, physical Pixel 9 (Android 16) and Galaxy S23 (Android 13), arm64 release builds:
main0.000tex.dispose()makeNonTextureImage()The e2e suite has no video coverage on either backend (
Video.tsisit.skip), so this is not something CI will exercise.