cordova-plugin-media-capture
cordova-plugin-media-capture copied to clipboard
apache
CB-13683: (android) cordova-plugin-media-capture rotates pictures with some devices
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
- [X] Reported an issue in the JIRA database
- [X] 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.
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?
你妈死了，傻逼
| | cljmomo 邮箱：[email protected] |
Signature is customized by Netease Mail Master
On 08/23/2018 19:05, Jan Piotrowski wrote:
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?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.
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.
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?
@janpio What do you think? Shall I return null if we have EXIF data but orientation is normal?
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.
Ok, I've updated the commit and now it uses original image if we have EXIF but no rotation
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.
... 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).
@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?
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 it's kind of natural to have conflicts after 1 month. Anyway, I'll take a look
a quick note: the current code of "Capture.java" has 34 warnings in android studio, I think this should be addressed
@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.
@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.
We are trying. You can help out over at https://github.com/apache/cordova-plugin-media-capture/issues/105 to enable us merging again.
@janpio sorry for bothering you again, but is there anything we can do to involve the parties needed for reviewing?
Short: No. Long: Materialize a Cordova committer with enough Android knowledge and time to review this.
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!
@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.
@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.
