Skip to content
New issue

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

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

Already on GitHub? # to your account

Make MediaCodecVideoRenderer::shouldUsePlaceholderSurface protected. #1905

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

khouzam
Copy link
Contributor

@khouzam khouzam commented Nov 19, 2024

This enables a derived renderer to disable the placeholder surface.

Not having a placeholder surface allows to delay instantiating the codecs until we have a surface. This allows to have more players ready without having their decoders loaded.

This follows the mode that shouldUseDetachedSurface is protected and overridable by a derived class.

Since getSurfaceForCodec must call hasSurfaceForCodec, the check for the placeholder surface in getSurfaceForCodec should not trigger if we disable the placeholder surface as hasSurfaceForCodec would have returned false.

@microkatz
Copy link
Contributor

microkatz commented Nov 22, 2024

@khouzam

Thanks for submitting your request! Just a curiosity, have you looked at MediaCodecVideoRenderer::shouldInitCodec for your use case? Its already overridable and utilized by MediaCodecRenderer to decide if the decoder should be initialized.

@khouzam
Copy link
Contributor Author

khouzam commented Nov 22, 2024

@microkatz Thanks for the suggestion, I'll experiment with it, but while this can prevent the initialization of the codec, it will still create the PlaceholderSurface when a null surface gets set.

@khouzam
Copy link
Contributor Author

khouzam commented Jan 21, 2025

@microkatz We've tried your suggestion. The results don't seem very encouraging. We see significant increases in ANRs, memory kills and native crashes. The additional resources used by the placeholder surface seems to really conflict with some of the fine tuning that we've done for our scenarios.

@tonihei
Copy link
Collaborator

tonihei commented Jan 28, 2025

@microkatz: @khouzam and I discussed this code path in person as well and concluded that the protected access makes sense because the method is also used from setOutput where there is no other way to prevent the usage of placeholder surfaces if the codec already exists and the surface is removed.

khouzam and others added 2 commits January 30, 2025 11:35
This enables a derived renderer to disable the placeholder surface.
@microkatz
Copy link
Contributor

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

@copybara-service copybara-service bot merged commit fc1d133 into androidx:main Jan 30, 2025
1 check passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants