CB-13683: (android) cordova-plugin-media-capture rotates pictures with some devices #100

mcelotti · 2018-08-23T10:47:35Z

Platforms affected

Android

What does this PR do?

It fixes this issue: https://issues.apache.org/jira/browse/CB-13683

What testing has been done on this change?

Tested on a real device that was having this issue (samsung s2)

Checklist

Reported an issue in the JIRA database
Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
Added automated test coverage as appropriate for this change.

janpio · 2018-08-23T11:03:37Z

src/android/Capture.java

+
+    private Uri fromBitmapToUri(Context context, Bitmap inImage) {
+        ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+        inImage.compress(Bitmap.CompressFormat.JPEG, 85, bytes);


hardcoded quality? Is this smart?

As far as I know there's no param in this plugin to set a specific image quality for the standard image capture. If we add a param to set quality it should work for standard images and rotated images, but this was not related to the issue. Anyway I agree, it's not smart, what would you suggest?

Normally the plugin doesn't really touch the image, right?
But if it was rotated, the result is uncompressed and has to be compressed to get to a similar size than what was returned by the device, correct?

Why can't we just set it to 100?

That's an option, sure

85 seems a bit random. 100 is still hard-coded, but at least it won't interfere with the quality wanted - they can always change the quality later.

Is that what we'll go with then?

Ok, the compression is now 100 abed367

janpio · 2018-08-23T11:05:24Z

Did I understand correctly that this PR adds some functionality that reads the EXIF data of the pictures and does a rotation if the EXIF data indicates it might be necessary?

Can this break functionality on older/other devices?
Is it possible something depends on the current behavior?

mcelotti · 2018-08-23T11:23:43Z

Yes, with some devices (ie samsung) the capture image is rotated and rotation details are in EXIF data.
If something goes wrong the "rotateAccordingToExifOrientation" should return null (or the original bitmap) hence no rotation will be applied.

janpio · 2018-08-23T11:28:58Z

I was more wondering if some device maybe reports something in EXIF data, but the pictures came out right anyway - but wouldn't any more with this patch applied. Possible?

mcelotti · 2018-08-23T13:06:42Z

Well, we could return null if ExifInterface.ORIENTATION_NORMAL

mcelotti · 2018-08-24T09:12:11Z

@janpio What do you think? Shall I return null if we have EXIF data but orientation is normal?

janpio · 2018-08-24T09:17:03Z

Did I understand correctly that this would avoid having to touch the image at all if not rotation is necessary? Then that sounds like the proper thing to do.

…h some devices

mcelotti · 2018-08-24T09:23:39Z

Ok, I've updated the commit and now it uses original image if we have EXIF but no rotation

janpio

Looks good.

As the default behavior for some devices changed, this should probably be considered a breaking change.

I am also not 100% if 1) there are no ways in which this could break behavior some users might rely on and 2) the default compression of the returned image is a good thing (although I can't think of an alternative).

mcelotti · 2018-08-24T10:42:45Z

Well, I understand your position but in my opinion the bug itself is more critical than 1) or 2)
There are so many samsung devices out there, and there might be also other non-samsung with the same problem.

janpio · 2018-08-24T10:55:04Z

... which is why I approved the Pull Request and just left those as a comment for whoever decides to merge (which might as well be me if no-one else shows up).

ffMathy · 2018-09-12T15:16:32Z

It has been 19 days and this fixes a critical issue. Can we merge it?

ffMathy

Works like a charm - tested on brand new Samsung Galaxy S9.

ffMathy · 2018-09-20T07:44:28Z

@janpio it could seem as if the tests fail for other reasons than actual code. Could seem as if the build process itself is broken. Would you agree?

janpio · 2018-09-20T08:45:18Z

Ahem yes, very much possible. Still has to be taken into account before reviewing and merging by a maintainer, which is why it is reflected in the project board state.

ffMathy · 2018-09-22T23:10:00Z

@mcelotti this PR has conflicts. Can you take a look?

mcelotti · 2018-09-24T07:08:51Z

@ffMathy it's kind of natural to have conflicts after 1 month. Anyway, I'll take a look

mcelotti · 2018-09-24T10:36:32Z

a quick note: the current code of "Capture.java" has 34 warnings in android studio, I think this should be addressed

janpio · 2018-09-24T10:38:12Z

@mcelotti To make sure this doesn't get forgotten/buried, please open a new issue. Feel free to look into it and create a PR yourself of course if you have the time and interest.

mcelotti · 2018-09-24T10:53:26Z

@janpio sure I could, but with all these open PRs (14) people will end up with a ton of conflicts if you merge this "code review" first. And if you merge the review in the next months then ... I'll have a lot of conflicts. I think there should not be so many open PRs, you should try to accept/reject more often.

janpio · 2018-09-24T11:05:07Z

We are trying. You can help out over at #105 to enable us merging again.

ffMathy · 2018-09-29T22:13:49Z

Build has now been fixed, but how do we re-run the build for this PR?

janpio · 2018-09-29T22:20:32Z

I can do that and just did. Build is now green.

ffMathy · 2018-09-29T23:02:30Z

Awesome! Can't wait until this is merged!

ffMathy · 2018-11-14T01:45:25Z

@janpio sorry for bothering you again, but is there anything we can do to involve the parties needed for reviewing?

janpio · 2018-11-14T09:45:04Z

Short: No.
Long: Materialize a Cordova committer with enough Android knowledge and time to review this.

ffMathy · 2018-11-14T09:45:50Z

Do you have a list of committers somewhere that would elligible for that description?

My concern is that there won't ever be random Cordova committers that will come by and review this. Only new people that won't fit that description.

This is a critical bug. It's unusable on Android devices. The fix works. It has been tested by the author and myself on real devices. The approach we are using right now doesn't seem very pragmatic to be honest.

I get that we shouldn't try to rush things in, but this could take years!

ffMathy · 2018-11-14T10:59:55Z

@jcesarmobile you (relatively) recently pushed an orientation fix for iOS. This issue is regarding Android which has the same problem. If you have Android experience also, can you help out by reviewing it?

I suppose you have interest in getting this bugfix out too, if you are targeting Android devices.

valgen · 2019-07-17T06:14:20Z

Hi, does anyone know when this PR will be merged?

janpio · 2019-07-17T08:11:52Z

No.

rogervanwile · 2020-01-19T16:28:20Z

@janpio : What have to be done to merge this PR into the master? Who can do this?

This PR was opened 1,5 years ago.

Fix Android Photo Rotation apache#100

mcelotti mentioned this pull request Aug 23, 2018

CB-14260: (android) captureImage permission denial on android 8.1 #95

Merged

3 tasks

janpio reviewed Aug 23, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

CB-13683: (android) cordova-plugin-media-capture rotates pictures wit…

77d7ab5

…h some devices

mcelotti force-pushed the CB-13683 branch from a1db2c9 to 77d7ab5 Compare August 24, 2018 09:22

janpio approved these changes Aug 24, 2018

View reviewed changes

CB-13683: (android) set compression level to 100 for rotated images

abed367

ffMathy approved these changes Sep 14, 2018

View reviewed changes

janpio added the platform: android label Sep 16, 2018

janpio added the bug label Sep 17, 2018

mcelotti force-pushed the CB-13683 branch 2 times, most recently from f713a5b to 97f658d Compare September 24, 2018 09:46

CB-13683: (android) merge upstream/master

84fba26

mcelotti force-pushed the CB-13683 branch from 97f658d to 84fba26 Compare September 24, 2018 10:27

Merge branch 'master' into CB-13683

56979a6

valgen mentioned this pull request Jul 17, 2019

media-capture plugin rotates my image by 90 degree left on potrait mode.Any fixes on this #132

Open

3 tasks

rogervanwile added a commit to rogervanwile/cordova-plugin-media-capture that referenced this pull request Jan 20, 2020

Merge pull request #1 from mcelotti/CB-13683

ab6cdf3

Fix Android Photo Rotation apache#100

CB-13683: (android) cordova-plugin-media-capture rotates pictures with some devices #100

Are you sure you want to change the base?

CB-13683: (android) cordova-plugin-media-capture rotates pictures with some devices #100

Conversation

mcelotti commented Aug 23, 2018

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janpio commented Aug 23, 2018

This comment has been minimized.

mcelotti commented Aug 23, 2018

janpio commented Aug 23, 2018

mcelotti commented Aug 23, 2018

mcelotti commented Aug 24, 2018

janpio commented Aug 24, 2018

mcelotti commented Aug 24, 2018

janpio left a comment

Choose a reason for hiding this comment

mcelotti commented Aug 24, 2018

janpio commented Aug 24, 2018 • edited Loading

ffMathy commented Sep 12, 2018

ffMathy left a comment

Choose a reason for hiding this comment

ffMathy commented Sep 20, 2018

janpio commented Sep 20, 2018

ffMathy commented Sep 22, 2018

mcelotti commented Sep 24, 2018

mcelotti commented Sep 24, 2018

janpio commented Sep 24, 2018

mcelotti commented Sep 24, 2018

janpio commented Sep 24, 2018

ffMathy commented Sep 29, 2018

janpio commented Sep 29, 2018

ffMathy commented Sep 29, 2018

ffMathy commented Nov 14, 2018

janpio commented Nov 14, 2018

ffMathy commented Nov 14, 2018 • edited Loading

ffMathy commented Nov 14, 2018

valgen commented Jul 17, 2019

janpio commented Jul 17, 2019

rogervanwile commented Jan 19, 2020

janpio commented Aug 24, 2018 •

edited

Loading

ffMathy commented Nov 14, 2018 •

edited

Loading