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

fix: Android ResolutionSelector.Builder.forSize #3020

Closed

Conversation

j-jonathan
Copy link
Contributor

@j-jonathan j-jonathan commented Jun 25, 2024

What

This PR fixes an issue with the resolution selection logic where two formats with the same difference metric resulted in selecting the incorrect resolution.

I encountered a problem with a phone that supports the following two formats:

  • "photoHeight": 2592, "photoWidth": 4608 --> (2592*4608 = 11943936)
  • "photoHeight": 3456, "photoWidth": 3456 --> (3456*3456 = 11943936)

When the first format was selected, the resulting picture was taken with the resolution of 3456 x 3456 instead of the intended 2592 x 4608.

Changes

This PR updates the difference function in the resolution selection code to consider both width and height differences in addition to the area difference.

Tested on

  • Samsung SM-G525F

Copy link

vercel bot commented Jun 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-vision-camera ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 25, 2024 0:33am

@mrousavy
Copy link
Owner

Nice!

@mrousavy
Copy link
Owner

to consider both width and height differences in addition to the area difference.

Why not just width and height differences? Why do we need the total area difference as well?

@j-jonathan
Copy link
Contributor Author

Why not just width and height differences? Why do we need the total area difference as well?

You’re right; using just the width and height differences simplifies the logic and is sufficient for selecting the closest resolution.

I have made the change accordingly.

@mrousavy
Copy link
Owner

... and is sufficient for selecting the closest resolution.

Okay are you 100% sure with this code we now choose the right format? I didn't test this, so I am fully relying on your testing here to make sure this always chooses the expected format.

@j-jonathan
Copy link
Contributor Author

j-jonathan commented Jun 25, 2024

Okay are you 100% sure with this code we now choose the right format? I didn't test this, so I am fully relying on your testing here to make sure this always chooses the expected format.

I have tested this on 2 or 3 phones, and everything works fine.

The resolution contained in the format is supposed to exist, so the correct resolution will be chosen. However, sometimes the selected format cannot be used (I found a case where if you set the FPS to the value of format.maxFps, certain high resolutions are not accepted). The chosen resolution will then be the next valid one in the list sorted by forSize.

The change to just using the difference between abs(left.width - right.width) + abs(left.height - right.height) could alter the order slightly compared to using the area difference. However, there isn’t one solution that is definitively better than the other; the behavior will potentially change depending on the available resolutions.

The only potential issue is that the width and height might be inverted. If you think it could happen , I can add a min/max check to ensure that we are correctly comparing width and height.
Something like:

private fun difference(left: Size, right: Size): Int {
    // Use min and max to handle cases where width and height might be inverted
    val leftMin = min(left.width, left.height)
    val leftMax = max(left.width, left.height)
    val rightMin = min(right.width, right.height)
    val rightMax = max(right.width, right.height)
    return abs(leftMin - rightMin) + abs(leftMax - rightMax)
}

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.

None yet

2 participants