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 image reading while ignoring EXIF orientation info #1091

Merged
merged 4 commits into from
Jun 25, 2021

Conversation

gaotongxiao
Copy link
Collaborator

Motivation

Sometimes the orientation info in EXIF could lead to unexpected rotation of images, messing up the annotations in dataset. Current imread() would always take the info into account unless flagged with "unchanged", which sacrifices flexibility in data preprocessing.

Modification

Added support for ignoring the orientation info in both color and grayscale mode. It works for cv2 and pillow backend. Also added unit test for them.

Add unit test for ignore_orientation flags in imread()
@CLAassistant
Copy link

CLAassistant commented Jun 11, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 13, 2021

Codecov Report

Merging #1091 (d8b984c) into master (004c006) will increase coverage by 0.00%.
The diff coverage is 80.00%.

❗ Current head d8b984c differs from pull request most recent head 9df1b71. Consider uploading reports for the commit 9df1b71 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1091   +/-   ##
=======================================
  Coverage   67.62%   67.62%           
=======================================
  Files         159      159           
  Lines       10283    10284    +1     
  Branches     1857     1858    +1     
=======================================
+ Hits         6954     6955    +1     
  Misses       2964     2964           
  Partials      365      365           
Flag Coverage Δ
unittests 67.62% <80.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmcv/image/io.py 88.46% <80.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 004c006...9df1b71. Read the comment docs.

@ZwwWayne ZwwWayne requested a review from innerlee June 14, 2021 15:10
@ZwwWayne
Copy link
Collaborator

LGTM, see if @innerlee has any comments.

@ZwwWayne ZwwWayne mentioned this pull request Jun 14, 2021
8 tasks
@innerlee
Copy link
Contributor

How does turbo-jpeg handles orientation?

@gaotongxiao
Copy link
Collaborator Author

How does turbo-jpeg handles orientation?

Turbo-jpeg only supports "color" and "grayscale" mode now, but I found it always ignore EXIF information when loading images. So explicitly appending the "_ignore_orientation" suffix to both modes seems better?

@innerlee
Copy link
Contributor

In this case, turbo-jpeg naturally supports _ignore_orientation.
Requiring explict _ignore_orientation to turbo-jepg will break user's code. I would not suggest it.

@innerlee
Copy link
Contributor

There is also a tiff backend. You should take care of it.

@gaotongxiao
Copy link
Collaborator Author

In this case, turbo-jpeg naturally supports _ignore_orientation.
Requiring explict _ignore_orientation to turbo-jepg will break user's code. I would not suggest it.

To be consistent, we can still add supports of _ignore_orientation modes to turbo-jpeg which do exactly the same things as the normal modes, but raise depreciated_api_warning for normal modes for the current version.

@gaotongxiao
Copy link
Collaborator Author

There is also a tiff backend. You should take care of it.

By default, tifffile also ignores EXIF info.

I've done few tests, and it seems tifffile backend hasn't been thoroughly tested beforehand. I'd recommend just leaving it as it is for now until bugs have been fixed.

@innerlee
Copy link
Contributor

innerlee commented Jun 15, 2021

raise depreciated_api_warning for normal modes for the current version.

This is not needed.

If you treat the current behavior of turbo-jpeg on color flag as a bug, then you can fix this bug by supporting the exif rotations for turbo-jpeg in later pr.

@gaotongxiao
Copy link
Collaborator Author

raise depreciated_api_warning for normal modes for the current version.

This is not needed.

If you treat the current behavior of turbo-jpeg on color flag as a bug, then you can fix this bug by supporting the exif rotations for turbo-jpeg in later pr.

Letting the same flag have different behaviors on different backends could be dangerous, but changing it can break users' code. I would say there are two possible ways to go:

  1. We don't change the default behaviors supported by backends, and add clear documentation (and maybe user warning) informing users of their difference
  2. Or, just treat it as a bug and fix

The first choice seems safest to me

@innerlee
Copy link
Contributor

You can do both. First do 1, and then do 2 and undo 1.

Either 1 or 2 does not affect this pr

@gaotongxiao
Copy link
Collaborator Author

Leave 2 as a future pr

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.

4 participants