Workaround for hardware JPEG rotation bug on some Samsung devices

[drash-course]

Description

This PR implements a workaround to avoid this issue: #3060

What is the root cause

Android's Camera.PictureCallback::onPictureTaken(byte[] data, Camera camera) is returning a corrupted file. The byte[] data array is full of 0x00 after a few kB of data. This happens when mCameraParameters.setRotation(...) is set to any rotation besides 0 on our Galaxy Tab S2 9.7 Wi-Fi SM-T813.

What is the workaround / fix

The facilities in ResolveTakenPictureAsyncTask.java can handle the rotation in software. The workaround is to set mCameraParameters.setRotation(0), take the picture, and perform the rotation in the async task. To differentiate this "backup" rotation from the rotation performed when "fixOrientation" is set, a new parameter softwareRotation has been added to ResolveTakenPictureAsyncTask.

The softwareRotation is only used for the known affected devices, set in BROKEN_ROTATION_DEVICE_MODELS. Other devices will continue to use hardware rotation (which is probably faster).

How to test

If you don't have an affected device on hand, you can still test this alternate implementation by changing Camera1::fallbackToSoftwareRotation() to return true or add your test device model to BROKEN_ROTATION_DEVICE_MODELS.

What can be improved

I only added "SM-G532M" and "SM-T813" which we know have this problem. More devices are probably affected. Since it seems to always be Samsung devices, we could enable this alternate rotation mode for all of them.

[cristianoccazinsp]

This looks promising. We have been getting reports of gray images on SM-T813 devices too and just assumed the devices were too old.

"Blacklisting" all samsung devices is probably a really bad idea, but using this kind of list of affected devices should be good. I don't know how this list could be maintained easier than what you have done so far.

Any issues on unaffected devices? Slower captures or something? Isn't it dangerous to change the camera parameters right before requesting a capture?

[cristianoccazinsp]

Also, do you know if the code could be improved in a way that mCameraParameters.setRotation is not needed?

Here's another device we saw the gray photos issue: SM-T307U and SM-T713

[drash-course]

Any issues on unaffected devices? Slower captures or something?

I tested this on SM-T813 with 1920x1080 photos, could not tell the difference in performance. Did not measure it precisely though. Normal devices not affected (tested on Xiaomi M1803E7SG).

Isn't it dangerous to change the camera parameters right before requesting a capture?

The previous implementation changes parameters before capture as well. The Android documentation states that's how you do it (https://developer.android.com/reference/android/hardware/Camera). We do the calls on the same thread so no concurrency issues here.

Also, do you know if the code could be improved in a way that mCameraParameters.setRotation is not needed?

I don't think so. mCameraParameters.setRotation(...) is also set before you take a picture, e.g. in Camera1::setDeviceOrientation and Camera1::adjustCameraParameters. It's set when you take a picture if you provide the "orientation" option when calling takePictureAsync(options) from JS.

My fix relies on the fact that Camera1::takePictureInternal will do mOrientation = options.getInt("orientation"); if the user provides a specific orientation. For broken devices I do int requestedRotation = calcCameraRotation(orientationEnumToRotation(mOrientation)); so it will respect the option if provided. Otherwise it uses the mOrientation value set via CameraView::onDisplayOrientationChanged, just like the normal implementation.

[cristianoccazinsp]

Thanks for the details, I believe it makes sense. Impressive detective work finding this bug!

I need some time before being able to test this as I have to catch up with the MLKit changes from another PR, but I would love to see this getting merged.

Thoughts @MateusAndrade @mikehardy ?

[mikehardy]

seems fine to me. In my experience Samsung has some buggy hardware (they messed up webview hardware rendering for Android 8 too, that was fun to find) just like MIUI has a buggy AOSP implementation. There's nothing to do but trust those that are directly probing the affected devices and accept their workarounds I think.

This seems like a nice way to handle an irritating problem 👍

[n1ru4l]

🎉 This PR is included in version 4.0.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[drash-course]

@cristianoccazinsp Would it be possible to have this fix in the 3.44 line as well? That's what we use since we aren't ready to move to Google ML Kit for all projects.

[cristianoccazinsp]

@drash-course that's really up to @MateusAndrade . However, if you are in such situation, I recommend forking the 3.44 yourself and cherry picking specific commits you may be interested in (like this). You can then change your package file to point to your github fork/branch instead.

This is what we also do since we stay behind the master branch a few commits and gradually merge from it in order to test all changes thoroughly first and be able to use important fixes right away.