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

Does not recognize rotated images? #212

Open
fzyzcjy opened this issue Dec 23, 2021 · 4 comments
Open

Does not recognize rotated images? #212

fzyzcjy opened this issue Dec 23, 2021 · 4 comments

Comments

@fzyzcjy
Copy link

fzyzcjy commented Dec 23, 2021

Hi thanks for the lib! I am using it on a rotated jpeg image. But this decoder gives the wrong size. For example, the real size by human beings and by OpenCV is 3000x4000, but this lib says 4000x3000 on it.

Looking at the source code, seems that there is no code about reading "orientations" in EXIF?

@HeroicKatora
Copy link
Member

Right, EXIF parsing is currently being treated as out-of-scope for the library. Instead, we report the raw binary form and push the interpretation of human rendering intent to an upper library. Do you agree this should be in image instead, and related to other formats that feature ICC and EXIF data?

@fzyzcjy
Copy link
Author

fzyzcjy commented Feb 7, 2022

@HeroicKatora Thank you! I am not sure about it. But IMHO rotated images are very common (I see many in production environment), and I hope Rust (image-rs/jpeg-decoder) can handle it

@kaj
Copy link
Member

kaj commented Feb 7, 2022

The important thing here is consistency. Currently, the image crate does not apply rotation, and when I read the exif I find that I should apply rotation. I think that if the image crate was to pre-apply the rotation, it needs to somehow tell me it did, maybe by hiding the exif entry. Otherwise, the rotation would be applied double.

So, I'm happy with the current state, but if the image (or jpeg-decoder) crate were to apply the rotation, much care is needed to make the api consistent. And it would probably be strange if it applied rotation but not other exif information (such as color profiles, etc).

@HeroicKatora
Copy link
Member

FWIW, my stance is that we're already doing far too much implicitly. Some of it should be available as options in image on explicit request though. Many images, including jpeg, get converted to some form of RGB and it's absolutely unclear to the caller (a) that this happened (b) which exact transformation has been performed (c) if it was the right one (quite the opposite, we know that's not always the case due to icc, see #185). This was a somewhat reasonable design choice in the initial scope but it's only been painful in the long run when other formats added even more conversion code internally on their own.

Three major reason to delay conversions:

  • All these computation would be much, much more efficient with hardware acceleration but throwing this into each individual decoder is a huge debt on any complexity budget.
  • When it's not require (i.e. transcoding) then it's wasted effort that degrades the overall image quality by subtle floating point inaccuracies or other not-quite-reversible transformation.
  • More code, more bugs (see above). Deferring to an independent library makes it easier to swap out downstream. This includes such things as icc/exif parsing.

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

No branches or pull requests

3 participants