-
Notifications
You must be signed in to change notification settings - Fork 3k
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
EXIF orientation support #4529
EXIF orientation support #4529
Conversation
cvat/apps/engine/models.py
Outdated
@@ -223,6 +223,7 @@ class Image(models.Model): | |||
frame = models.PositiveIntegerField() | |||
width = models.PositiveIntegerField() | |||
height = models.PositiveIntegerField() | |||
orientation = models.PositiveIntegerField(default=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ActiveChooN , could you please explain why we cannot send to UI already "rotated" image? The idea is to incapsulate the knowledge on the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean we should store rotated images on the server side? In that case, there would be a mismatch in the resolution of the raw data and compressed. I don't see a big problem here, but the whole idea behind this PR was to try to not modify the original data if it's possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe more general solution is to store received EXIF information as a JSON in our database (as a part of meta information), instead of storing only orientation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this. What kind of information can be helpful for us? I think we can move to that solution if we will have such a request to parse and use other EXIF tags. But EXIF usually stores a lot of information like thumbnail and photo instruments info which is quite useless for us but costs a lot of storage.
cvat/apps/engine/media_extractors.py
Outdated
@@ -90,13 +90,26 @@ def _get_preview(obj): | |||
else: | |||
preview = obj | |||
preview.thumbnail(PREVIEW_SIZE) | |||
orientation = preview._getexif().get(274, 1) if preview._getexif() else 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like _getexif() is a private method. Using such methods outside is not good practice.
What these magic numbers (274, 1) means is not obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_getexif()
is a convenient method to get EXIF information from PIL Image. 274 is the Orientation key, it can be gotten via
(orientation_key,_), = filter(lambda kv: kv[1]=='Orientation', ExifTags.TAGS.items())
But it looks messy. Probably I will add const with some description.
cvat/apps/engine/media_extractors.py
Outdated
@@ -90,13 +90,26 @@ def _get_preview(obj): | |||
else: | |||
preview = obj | |||
preview.thumbnail(PREVIEW_SIZE) | |||
orientation = preview._getexif().get(274, 1) if preview._getexif() else 1 | |||
if orientation in [3, 4]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment about magic number, maybe to create a corresponding enumeration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ActiveChooN , I agree that it is not a good idea to use a private method. Please use the public API.
Does this patch work for context images? |
Yeap @bsekachev, @nmanovic, rewrote PR to prerotated images. |
Co-authored-by: Andrey Zhavoronkov <[email protected]>
I tried this patch using the archive below. Looks like there are issues when images are mirrored |
@bsekachev, fixed |
@@ -194,14 +194,18 @@ def __iter__(self): | |||
if idx in self.range_: | |||
image = next(sources) | |||
img = Image.open(image, mode='r') | |||
orientation = img.getexif().get(274, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Marishka17 , probably you want to look at these changes and test them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmanovic, Everything works fine.
Co-authored-by: Maria Khrustaleva <[email protected]>
Co-authored-by: Maria Khrustaleva <[email protected]>
Co-authored-by: Maria Khrustaleva <[email protected]>
Co-authored-by: Maria Khrustaleva <[email protected]>
Co-authored-by: Maria Khrustaleva <[email protected]>
Co-authored-by: Maria Khrustaleva <[email protected]>
Motivation and context
Resolve #1059, #738.
This PR adds ability to parse EXIF orientation information when media is uploaded and handle it on the client.
How has this been tested?
Manually
Checklist
develop
branchcvat-core, cvat-data and cvat-ui)
License
Feel free to contact the maintainers if that's a concern.