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.

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

Support EXIF orientation for network JPG/TIFF #692

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support EXIF orientation for network JPG/TIFF #692

wants to merge 3 commits into from

Conversation

jacobtabak
Copy link
Contributor

Uses commons-imaging to determine orientation (only if it's on the classpath, it's an optional dependency). Tests intentionally omitted for now, happy to add them if you want this feature.

Since commons-imaging is still at 1.0.0-SNAPSHOT I had to temporarily add the Apache snapshot repository but this API hasn't changed since it was brought into commons.

@@ -34,6 +42,7 @@

private final Downloader downloader;
private final Stats stats;
private static Boolean hasImaging;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be at the top, boolean, should be final and should be initialized by a purpose-built function.

@JakeWharton
Copy link
Collaborator

This functionality will need tests.

@jacobtabak
Copy link
Contributor Author

Sure, I saw a comment this comment that discouraged me from doing all the work up front, but happy to do it if there's even a little interest.

@jacobtabak
Copy link
Contributor Author

Thanks for the feedback - I'll revisit this tomorrow.

@JakeWharton
Copy link
Collaborator

Well the Sanselar jar alone is 5x the size of Picasso so you can see why we can't fully support things like that with a hard dependency. Not sure how large the commons version of it will be.

I'd really like to see what it'd take to build an EXIF parser on top of Okio and embed that. If we wanted to get fancy we could build is as a pluggable system into the bowels of Picasso so that it could stand as its own add-on module (like the Pollexor one).

@dnkoutso dnkoutso added this to the Picasso 2.4 milestone Oct 7, 2014
@jacobtabak
Copy link
Contributor Author

I changed the dependency to Sanselan until Commons Imaging is finalized. Also, I spent some time thinking about how to test this - I noticed that the functionality is not tested on FileRequestHandler either.

We can either manually construct the EXIF data using Sanselan (which I tried, and was more trouble than it was worth), or just include a small image and convert that to a stream to use in a mocked downloader.

@billylindeman
Copy link

This post on StackOverflow (http://stackoverflow.com/questions/5468098/reading-exif-data-from-byte-array-in-android) points to a very simple Exif orientation extractor that google uses in com.android.Camera.

http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android-apps/4.1.1_r1/com/android/camera/Exif.java

Maybe this could be added to have a very light-weight Orientation extractor for JPEG's.

@jacobtabak
Copy link
Contributor Author

I think an issue with this approach is that it requires the entire byte[] to sit in memory to determine the orientation which would be really inefficient for an edge case. This approach only reads the EXIF bytes and then resets the stream back to its original position.

@billylindeman
Copy link

In the Gallery2 AOSP app there is an ExifParser class that is much more robust than the old one I posted above. This one seems to use an InputStream and seeking to extract the data.

https://android.googlesource.com/platform/packages/apps/Gallery2/+/bb23e198873a6e63c34b52fe3d1f12d3e5d13a2c/gallerycommon/src/com/android/gallery3d/exif/ExifParser.java

The whole exif package is here:
https://android.googlesource.com/platform/packages/apps/Gallery2/+/bb23e198873a6e63c34b52fe3d1f12d3e5d13a2c/gallerycommon/src/com/android/gallery3d/exif

Something like this might be a good drop-in. It'd be much smaller than including all of Sanselan/Commons Imaging.

@jacobtabak
Copy link
Contributor Author

Great, nice find.

@billylindeman
Copy link

The latest camera app actually uses that too. This is an updated version of the original file, but uses the newer Exif library.
https://android.googlesource.com/platform/packages/apps/Camera/+/master/src/com/android/camera/Exif.java

@billylindeman
Copy link

https://github.com/billylindeman/picasso/tree/billylindeman/embed_exif_parser_network_rotation

It works, but right now it tries to read all exif tags. Definitely a good starting point for something light-(er) weight though.

@dnkoutso
Copy link
Collaborator

I am cleaning up all 2.4 issues. This one I'd like to take but has no tests. I can't accept this for 2.4 without tests.

@dnkoutso dnkoutso modified the milestones: Picasso Next, Picasso 2.4 Oct 23, 2014
@jacobtabak
Copy link
Contributor Author

There are no tests for file:// rotation either, though. I spent a good amount of time trying to come up with a way to test it without using an actual image but didn't have any luck. @billylindeman do you want to make a PR with your code and try to write some tests?

@phileo
Copy link

phileo commented Apr 15, 2015

Newbie question: how would code calling Picasso access the EXIF orientation that was read by this pull request?

@JakeWharton
Copy link
Collaborator

You wouldn't. It's transparent to you.

On Wed, Apr 15, 2015 at 1:52 PM Phileo [email protected] wrote:

Newbie question: how would code calling Picasso access the EXIF
orientation that was read by this pull request?


Reply to this email directly or view it on GitHub
#692 (comment).

@phileo
Copy link

phileo commented Apr 15, 2015

ok, let me try this from another angle...
How could I modify this pull request to report the EXIF orientation for the downloaded image via a consumable API?

@JakeWharton
Copy link
Collaborator

We have no way to do that. See #639.

@phileo
Copy link

phileo commented Apr 17, 2015

thanks for that nugget of info, I think that might be able help me

@rharter
Copy link
Contributor

rharter commented Apr 20, 2015

Forgive my ignorance, but I've added tests for the network exif orientation support in my fork of Picasso, and I'd like to submit it as a PR to the timehop repo, but I'm not sure how to do that across forks.

The changes are here, please feel free to pull them if they look helpful, or tell me how I can submit them to update the PR.

@howawong
Copy link

howawong commented Mar 2, 2018

Any update?

@JakeWharton JakeWharton removed this from the Picasso Next milestone Mar 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants