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.

Sign up for GitHub

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

Jump to bottom

fix: switch width and height of target resolution if portrait for Android #833

Closed
wants to merge 3 commits into from
Closed

fix: switch width and height of target resolution if portrait for Android #833

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
412dc2d
fix: switch width and height of target resolution if portrait for And…
xulihang Feb 17, 2022
a5789f9
add missing import
xulihang Feb 17, 2022
ee6eef2
use orientation instead of rotation
xulihang Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions android/src/main/java/com/mrousavy/camera/CameraView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import android.content.res.Configuration
import android.hardware.camera2.*
import android.util.Log
import android.util.Range
import android.util.Size
import android.view.*
import android.view.View.OnTouchListener
import android.widget.FrameLayout
Expand Down Expand Up @@ -410,9 +411,15 @@ class CameraView(context: Context, private val frameProcessorThread: ExecutorSer
// User has selected a custom format={}. Use that
val format = DeviceFormat(format!!)
Log.i(TAG, "Using custom format - photo: ${format.photoSize}, video: ${format.videoSize} @ $fps FPS")
previewBuilder.setTargetResolution(format.videoSize)
val videoSize:Size
if (context.resources.configuration.orientation == Configuration.ORIENTATION_PORTRAIT) {
Copy link
Owner

Choose a reason for hiding this comment

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

Hey, thanks again for the PR.

Unfortunately I am still not quite sure if I understand this correctly, doesn't this only check the "locked" Orientation in AndroidManifest? What if the user is holding his phone in portrait, then going to landscape? And then going back to portrait? Is it still the correct resolution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I read the docs correctly, context.resources.configuration.orientation returns the device's current orientation.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, but can we use outputOrientation instead of context.resources.configuration.orientation here? I already detect orientation in this class

Copy link

@ldstein ldstein Feb 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

outputRotation is referencing context.displayRotation. That tells us how many degrees the device is rotated from it's natural position but I believe we cannot assume zero rotation is portrait.

context.resources.configuration.orientation differs as it tells us explicitly whether the device is currently being viewed in portrait or landscape.

Similarly, there maybe an issue here in the outputRotation getter. Portrait would only equal Surface.ROTATION_0 for naturally-portrait devices:

  private val outputRotation: Int
    get() {
      if (orientation != null) {
        // user is overriding output orientation
        return when (orientation!!) {
          "portrait" -> Surface.ROTATION_0
          "landscapeRight" -> Surface.ROTATION_90
          "portraitUpsideDown" -> Surface.ROTATION_180
          "landscapeLeft" -> Surface.ROTATION_270

Also not 100% sure on this, but maybe imageAnalysis.targetRotation should be the same as preview.targetRotation ? Keeps the analysis results 1:1 with the preview (when drawing overlays, for example).

I can see the use of the 'orientation' prop to affect photo / video output, but maybe should be independent to modifying the frame processor rotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot find outputOrientation. There is orientation which is a string and outputRotation which is an integer.

Copy link

Choose a reason for hiding this comment

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

Sorry, typo on my end. I fixed up my earlier post. I meant outputRotation

Copy link
Owner

Choose a reason for hiding this comment

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

Okay guys thanks for the insights. I will test this some time this week and then merge the PR if everything works in every use-case :)

Would be helpful if anyone could help me with testing, with various devices and various orientations (e.g. start with landscape or portrait, then rotate)

Copy link

@siddharth-kt siddharth-kt Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@mrousavy @xulihang What are your views on this PR, is it ready to get merged or ...?

videoSize = Size(format.videoSize.height, format.videoSize.width)
}else {
videoSize = format.videoSize
}
previewBuilder.setTargetResolution(videoSize)
imageCaptureBuilder.setTargetResolution(format.photoSize)
imageAnalysisBuilder.setTargetResolution(format.videoSize)
imageAnalysisBuilder.setTargetResolution(videoSize)

// TODO: Ability to select resolution exactly depending on format? Just like on iOS...
when (min(format.videoSize.height, format.videoSize.width)) {
Expand Down