Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always null-check MediaCodec with synchronized Holder #3597

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

kaidokert
Copy link
Member

@kaidokert kaidokert commented Jun 20, 2024

Wraps mMediaCodec in a synchronized holder class, guaranteeing synchronized access and prevents unhandled null pointer exceptions.

This is alternate implementation to #3596

b/331835987

@kaidokert kaidokert marked this pull request as ready for review June 20, 2024 03:03
@kaidokert
Copy link
Member Author

On-device tests are failing for unrelated reason, revert pending in #3598

@kaidokert kaidokert force-pushed the more_sync branch 3 times, most recently from fa36367 to 11fac27 Compare June 21, 2024 21:08
@kaidokert kaidokert requested a review from johnxwork June 23, 2024 16:25
Adds a simple guard getter around mMediaCodec access, to reduce chances
it gets destroyed and then accessed.
Note this should really be a synchronized access instead from all call
sites.

b/331835987
Copy link
Contributor

@zhongqiliang zhongqiliang left a comment

Choose a reason for hiding this comment

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

This change will convert the NPE to a IllegalStateException, it won't fix the bug.

@kaidokert
Copy link
Member Author

This change will convert the NPE to a IllegalStateException, it won't fix the bug.

That's the intent, yes. We won't get NPE crashes, and the IllegalState will get logged.

@kaidokert kaidokert merged commit 77126bc into youtube:main Jun 24, 2024
356 of 366 checks passed
@kaidokert kaidokert added the cp-25.lts.1+ Cherry Pick to the 25.lts.1+ branch label Jun 24, 2024
cobalt-github-releaser-bot pushed a commit that referenced this pull request Jun 24, 2024
Wraps mMediaCodec in a synchronized holder class, guaranteeing
synchronized access and prevents unhandled null pointer exceptions.

This is alternate implementation to #3596

b/331835987

(cherry picked from commit 77126bc)
@zhongqiliang
Copy link
Contributor

This change will convert the NPE to a IllegalStateException, we already have catch blocks for IllegalStateException, so this will reduce the exceptions.

kaidokert added a commit that referenced this pull request Jun 24, 2024
…Holder (#3668)

Refer to the original PR: #3597

Wraps mMediaCodec in a synchronized holder class, guaranteeing
synchronized access and prevents unhandled null pointer exceptions.

This is alternate implementation to #3596 
    
b/331835987

---------

Co-authored-by: Kaido Kert <kaidokert@google.com>
@kaidokert kaidokert deleted the more_sync branch June 30, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cp-25.lts.1+ Cherry Pick to the 25.lts.1+ branch on_device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants