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

Player view to show post-processed resolution instead of the input one #1025

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

dsparano
Copy link
Contributor

@dsparano dsparano commented Jan 25, 2024

Problem
If a video effect is applied to the video, or an "enhancement" such as LCEVC, is applied, the on screen player view debug text still shows the "input" resolution and pixel aspect ratio and not the one after post-processing. This can be easily reproduced with effects such as Crop or ScaleAndRotateTransformation which change the output resolution.

Root cause
The resolution and pixel aspect ratio shown are taken from the input Format object, which is a property of the stream and not of the video decoding and post processing applied.

Proposed solution
Take with, height and pixel aspect ratio from the player's VideoSize, which does get updated in case of post-processing.

@dsparano
Copy link
Contributor Author

Found TODO b/292111083 - Remove the field and trigger the callback on every video size change. in VideoFrameRenderControl and done it as it's needed to update the reported VideoSize when switching among video tracks.

@oceanjules oceanjules requested review from claincly and removed request for claincly January 29, 2024 14:42
@claincly
Copy link
Contributor

We recently changed the way of size notification and decided to temporarily disable the size notification to the app, in dcae49a

So unfortunately this fix won't work before we bring back the size notification.

@claincly
Copy link
Contributor

To clarify on this PR -

  • I think this PR makes sense on its own and can be merged
  • I wonder what is your intended use case though? Do you want to get the video size after being processed by VideoFrameProcessor? If that's the case, you won't be getting the correct size (I think) even after merging this PR with the newest code in main

@dsparano
Copy link
Contributor Author

dsparano commented Mar 1, 2024

@claincly I think this PR would be useful for at least two use cases:

  • video effects (see above)
  • use of decoder_lcevc, in which case the output (enhanced) resolution would be correctly set in DebugTextViewHelper as opposed to the base codec resolution as it is currently from main

@claincly
Copy link
Contributor

claincly commented Mar 4, 2024

Sounds good, one thing related to pushing the PR:

Would it be possible to open a new PR from an individual-owned fork? We can't push changes to organization-owned forks like this one unless we have collaborator access. If that's not possible then we can still merge this PR but it will result in an 'evil' merge. See more info here: https://github.com/androidx/media/blob/release/CONTRIBUTING.md#push-access-to-pr-branches

@dsparano
Copy link
Contributor Author

dsparano commented Mar 4, 2024

@claincly no prob, I've given you write permission so you can commit into our originating branch

@copybara-service copybara-service bot merged commit bbdaf2b into androidx:main Mar 4, 2024
1 check passed
SheenaChhabra pushed a commit that referenced this pull request Apr 8, 2024
PiperOrigin-RevId: 612485043
(cherry picked from commit bbdaf2b)
l1068 pushed a commit to l1068org/media that referenced this pull request Apr 15, 2024
PiperOrigin-RevId: 612485043
(cherry picked from commit bbdaf2b)
@androidx androidx locked and limited conversation to collaborators May 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants