Skip to content

android: Direct-copy Java buffer to array for GetTransformMatrix#10777

Open
kjyoun wants to merge 2 commits into
youtube:mainfrom
kjyoun:GetTransformMatrix
Open

android: Direct-copy Java buffer to array for GetTransformMatrix#10777
kjyoun wants to merge 2 commits into
youtube:mainfrom
kjyoun:GetTransformMatrix

Conversation

@kjyoun
Copy link
Copy Markdown
Contributor

@kjyoun kjyoun commented Jun 5, 2026

Refactors VideoSurfaceTexture.getTransformMatrix() to return a cached member float array instead of allocating a new one on every call, avoiding GC churn on the 60fps rendering path.

This change also removes the error-prone env->NewFloatArray call from the C++ hot path. Ssee http://b/520007952#comment4.

Issue: 520007952
Issue: 518914404

Refactors VideoSurfaceTexture.getTransformMatrix() to return a cached
member float array instead of allocating a new one on every call, avoiding
GC churn on the 60fps rendering path.

This change also removes the error-prone `env->NewFloatArray` call from the
C++ hot path, eliminating the risk of JNI local reference leaks in this area
(see b/520007952#comment4).

Issue: 520007952
Issue: 518914404
@kjyoun
Copy link
Copy Markdown
Contributor Author

kjyoun commented Jun 5, 2026

/gemini review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🤖 Gemini Suggested Commit Message


android: Cache VideoSurfaceTexture matrix

Refactor VideoSurfaceTexture.getTransformMatrix() to return a cached
member float array instead of allocating a new one on every call.
This avoids significant garbage collection churn on the 60fps
rendering path.

This change also removes the env->NewFloatArray call from the C++
hot path, eliminating the risk of JNI local reference leaks in
this area.

Issue: 520007952
Issue: 518914404

💡 Pro Tips for a Better Commit Message:

  1. Influence the Result: Want to change the output? You can write custom prompts or instructions directly in the Pull Request description. The model uses that text to generate the message.
  2. Re-run the Generator: Post a comment with: /generate-commit-message

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request optimizes matrix retrieval in VideoSurfaceTexture by caching the transform matrix to avoid garbage collection churn on the rendering hot path, and updates the C++ JNI bridge to retrieve this cached matrix. However, the review feedback highlights a critical thread-safety issue: caching the matrix as a single instance member can lead to race conditions and corrupted data when accessed concurrently from the rendering and decoder threads. The reviewer recommends using a ThreadLocal<float[]> to safely cache the matrix per thread.

Comment thread cobalt/android/apk/app/src/main/java/dev/cobalt/media/VideoSurfaceTexture.java Outdated
Comment thread cobalt/android/apk/app/src/main/java/dev/cobalt/media/VideoSurfaceTexture.java Outdated
@kjyoun kjyoun marked this pull request as ready for review June 5, 2026 00:35
@kjyoun kjyoun requested a review from a team as a code owner June 5, 2026 00:35
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request optimizes the retrieval of the video surface texture transform matrix by caching the float array to avoid allocations on the rendering hot path. It refactors VideoSurfaceTexture in Java to return the matrix, and updates VideoSurfaceTextureBridge and MediaCodecVideoDecoder in C++ to retrieve and copy the matrix directly into a C++ array. The review feedback suggests replacing the ThreadLocal cache in VideoSurfaceTexture with a simple float[] member variable to avoid unnecessary overhead, as SurfaceTexture is not thread-safe. Additionally, it recommends narrowing the scope of the matrix4x4 variable in MediaCodecVideoDecoder to the specific blocks where it is used, in accordance with the C++ style guide.

Comment thread starboard/android/shared/media_codec_video_decoder.cc
Comment thread starboard/android/shared/media_codec_video_decoder.cc
Comment thread starboard/android/shared/media_codec_video_decoder.cc
@kjyoun
Copy link
Copy Markdown
Contributor Author

kjyoun commented Jun 5, 2026

Gemini comments are addressed

PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant