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

autorotate - issue 1162 #1252

Merged
merged 1 commit into from May 20, 2021
Merged

autorotate - issue 1162 #1252

merged 1 commit into from May 20, 2021

Conversation

Hassan-Mallah
Copy link
Contributor

Backend checklist

  • Formatted my code by running isort . && black . in app/backend
  • Added tests for any new code or added a regression test if fixing a bug
  • All tests pass
  • Run the backend locally and it works
  • Added migrations if there are any database changes, rebased onto develop if necessary for linear migration history

Frontend checklist

  • Formatted my code with yarn format && yarn lint --fix
  • There are no warnings from yarn lint
  • There are no console warnings when running the app
  • Added any new components to storybook
  • Added tests where relevant
  • All tests pass
  • Clicked around my changes running locally and it works
  • Checked Desktop, Mobile and Tablet screen sizes

@Hassan-Mallah Hassan-Mallah linked an issue May 17, 2021 that may be closed by this pull request
@aapeliv
Copy link
Member

aapeliv commented May 17, 2021

Are you able to come up with a simple test for this? If it seems very tricky (might be), then no problem

@Hassan-Mallah
Copy link
Contributor Author

Hey,

Is this what we need? :)
I wasn't able to recreate the error on Mozilla, but i applied autorotate before removing EXIF data.
cheers

@Hassan-Mallah
Copy link
Contributor Author

Are you able to come up with a simple test for this? If it seems very tricky (might be), then no problem

it looks like the test should be here:
def test_strips_exif(client_with_secrets):
i'll check after work :)

@aapeliv
Copy link
Member

aapeliv commented May 17, 2021

The way to go would be to create a small test image (maybe a 3x3 image) with some given EXIF rotation info, and then creating a new test to make sure the image was rotated correctly, etc. I'm just not sure how annoying it is to create such a test image!

@Hassan-Mallah
Copy link
Contributor Author

hmmm, that's not clear from the issue :)
so we need to autorotate the image? the error is that we rotated the image, so we shouldn't rotate it, right?

@aapeliv
Copy link
Member

aapeliv commented May 17, 2021

If I understand correctly; it's that a lot of software stores the image's orientation instead of actual 'moving' the pixels. So when you for example upload a portrait oriented photo from an iPhone it probably assumes the viewer (browser) will rotate it, but we strip that data away. So we need to rotate it ourselves to truly move the pixels in order for it to display correctly in the browser after exif stripping.

So you'd need to make a test image that "displays" correctly in full viewers, but whose data was not 'moved' and just has a roatiton exif tag; and then making sure that image shows up correctly after we save it.

Does that make sense?

@Hassan-Mallah
Copy link
Contributor Author

Oh!, ya, now it's clear. thnks for the explanation.

@aapeliv
Copy link
Member

aapeliv commented May 20, 2021

@Hassan-Mallah: I know you're busy for now, so I'll merge this so that the bug fix can be deployed. You can write a new PR with a test, or just leave it; it's not so important.

@aapeliv aapeliv merged commit ff370fe into develop May 20, 2021
@aapeliv aapeliv deleted the backend/feature/autorotate-photo branch May 20, 2021 19:17
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.

Image upload orientation data not taken into account
2 participants