Portrait image being cropped to smaller 4:3 piece even with autoSquareCrop false (v3.0.0alpha-14)

Murilo-Perrone commented 3 years ago

This bug has been oberved in nativescript-camera-plus component (here). Up to 3.0.8 pictures were ok, but in 3.1.0 pictures got smaller, because portrait mode was not detected by fancycamera.

As noted by @triniwiz , one reason is that mapping of FancyCamera.cameraOrientation property to internal camera (cameraView) is missing. But fixing that wasn't enough because it avoided the image crop but brought a -90 rotated image.

Another fix that may be necessary is to change Camera2.kt so that it also rotates the image when autoSquareCrop is false. I am testing this fix already, but may need more testing guidelines.

triniwiz commented 3 years ago

This should be the way to get back things working I missed this with the changes

Murilo-Perrone commented 3 years ago

Thanks for the pointers @triniwiz . The code you linked confirms that the image needs to be rotated in all cases, not only when autoSquareCrop is used.

However when autoSquareCrop is false, this code uses a different strategy. It uses the faster method from ImageCapture class (which provides a jpg file), then uses ExifInterface to "rotate" the image through the image's metadata (instead of rotating the actual pixels matrix). Although this is a common practice, it is not 100% compatible. For instance I have problems with Amazon Image Rekognition console when using this images. I then use imagemagick to rotate actual pixels from pictures that came from Samsung cell phones.

So perhaps we should offer both rotation strategies? I mean to allowing user to prioritize for fast results (exif) or the maximum compatibity (in-memory pixel rotation). Let me know your thoughts. I could add an optional parameter in FancyCamera.takePhoto(), or a boolean property. And add a boolean parameter in ICaptureOptions for nativescript-camera-plus as well.

The fix I have just committed here does the rotation via pixels, but more testing is still needed.

triniwiz commented 3 years ago

I think it should be exif by default but add in the option to rotate the pixels

Murilo-Perrone commented 3 years ago

After thousands of pictures, I have figured out some things. Also, I took the opportunity to upgrade for December's camerax release 1.0.0-rc01 / 1.0.0-alpha20, which is quite faster. The camerax guide provides the best guidelines. I found out 4 main things were missing in the code:

The targetRotation must be properly calculated when building of camerax components, and it can be updated aftewards.
The pictureSize must be specified in wither Height x Width or Width X Height depending rather the app is in portrait or landscape mode - this was the main reason for the image cropping but, cutting a 3:4 image into smaller 4:3.
The preview surface provider configuration must be revisited (link). From what I understood, it must bind to the view in a way that allows camerax to detect app App rotations and adjust accordingly. For instance, when you turn phone from portrait to landscape with screen unlocked, the 3:4 portrait preview should rotate into 4:3. Since that is not happening the preview is shown "tilted" in landscape mode (see image below). The docs recommend using PreviewView from camerax. I didn't explore this possibility yet so I don't know if using this class is an option (if it can be integrated with the nativescript plugin).
The camerax components are binded to the view's life cycle (which is correct), but they are not being re-created along with the view. When the app switches to landscape mode, I have to so stopPreview + startPreview, otherwise I get SurfaceClosedException: "Posting surface closed".

I am working on 1 to 3. I also added allowExifRotation property, defaulting to true. And I am updating demo project from nativescript-camera-plus with all new options from fancycamera API:

I and noticed the "Retake!" dialog does not support Exit rotation, because it does pixel level resampling. That can later be fixed too:

retakeDialog

In regards to targetRotation calculation, it is a bit complex, but worth it. Because once targetRotation is calculated properly, no further evaluation will have to be set during ImageCapture.takePicture callbacks execution. That's because the ImageProxy already has the targetRotation in it (for our code to do the bitmap rotation). And the callback that provides a saved JPG already adds Exif rotation in it. However, if we rewrite that file during the callback, we may loose that info.

From what I understood, there are 4 parameters we may need to taken into account to calculate proper targetRotation. Below, I use rotation/orientation words interchangeably:

a) The view rotation - This value can change if the entire Android screen is rotated. Rotating the screen is like the viewer rotating it's head - he must receive a different image then. This value may change dynamically in apps that don't lock the view (such as the nativescript plugin demo). This value also must be taken into account in apps that run in a fixed orientation, such as forcing landscape in a phone whose natural orientation is portrait.
b) The device's current orientation - A dynamic value, determined by device's sensors. It's the actual camera rotating. Native phone camera apps use this parameter instead of (a), because it is less expensive to just rotate some user controls instead of allowing the entire screen to rotate.
c) The device's specific sensor rotation - A fixed reference value that may change among different devices, usually it's zero. I don't know how to get this value.
d) The camera´s relative position - switching between back and front cameras will invert all rotation coordinates.

I'm now I'm obtaining (a) from the view. And we get (b) from OrientationEventListener into CameraBase.currentOrientation. I don't know if OrientationEventListener takes (b+c) into account as well or just (b).

It's much easier to use numbers instead of enums to calculate the targetRotation. For instance, for basic scenario where the screen is rotating and the device has no sensor rotation, this works well:

// 1 = 90, 2 = 180, 3 = 270
val viewRotation = getViewRotation()
val camDirection = when (position) { CameraPosition.BACK -> 1; CameraPosition.FRONT -> -1 else -> 0}
val target = (4 + viewRotation * camDirection) % 4

Checking the deprecated API calculation of targetRotation I observed the logic found in Camera.kt. It combines Camera.CameraInfo.orientation (API 9-20) with current activity's rotation. It seems a bit experimental, as it has an exception for a specific model (Nexus6) and probably does not handle all use cases.

private var rotationAngle = 0
private fun updateCameraDisplayOrientation(activity: Activity, cameraId: Int, camera: Camera?) {
  val info = Camera.CameraInfo()
  Camera.getCameraInfo(cameraId, info)
  var degrees = activity.windowManager.defaultDisplay.rotation * 90
  if (info.facing == Camera.CameraInfo.CAMERA_FACING_FRONT) {
    if (isNexus6) angle = 90
    else angle = (info.orientation + degrees) % 360
  } else {
    angle = (info.orientation - degrees + 360) % 360

For new api, I found two new properties used by Camera2.kt:

internal var currentOrientation: Int = OrientationEventListener.ORIENTATION_UNKNOWN // CameraBase.kt
override var rotation: CameraOrientation = CameraOrientation.UNKNOWN // Camera2.kt

The rotation: was not being set anywhere and not mapped to nativescript-camera-plus, but was being used to calculate targetRotation of camerax. I'm assuming this was unfinished work, with the goal of allowing external specification. I think we must have automatic internal calculation, plus the option to override it from external code. By now I'm testing automatic getTargetRotation for Camera2.

Another project also using camerax: arindamxd/android-camerax.

Murilo-Perrone commented 3 years ago

I noticed Camera2 components are were being rebuilt until preview is re-started and that they could be reconfigured from within camera-lplus:

constructor(...) {
  on("orientationChanged",  (evt) => {
    setTimeout(() => this.onOrientationChanged(evt.eventName, evt.newValue), 1000) // Restart preview
  });
}
onOrientationChanged(eventName: string, newValue: string) {
  if (isAndroid && this.cam) {
    const fancyCamera = (<any>this.cam)._camera as com.github.triniwiz.fancycamera.FancyCamera;
    fancyCamera.stopPreview();
    setTimeout(() => fancyCamera.startPreview(), 1000);
  }
}

So I found out a simple way for doing it within Camera2.kt using onConfigurationChanged. Seems ok now ! 🥳

Murilo-Perrone commented 3 years ago

fixed by #13

triniwiz / fancycamera

Portrait image being cropped to smaller 4:3 piece even with autoSquareCrop false (v3.0.0alpha-14) #12