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

Rotate and resize photo before saving #2015

Merged
merged 7 commits into from
May 22, 2023
Merged

Conversation

VirginiaDooley
Copy link
Contributor

@VirginiaDooley VirginiaDooley commented Apr 7, 2023

Ref python-pillow/Pillow#4703 and https://trello.com/c/gEpWTwI9/3323-photo-rotation

The problem: https://candidates.democracyclub.org.uk/moderation/photo/review

  • Many images have been uploaded sideways (we expect this is the result of mobile selfies).
  • XL images result in slow load times and facial recognition errors.

In this PR:

  • Rotate the photo at the point of saving on the person edit form : At this stage, we haven't yet converted the image to a png so the exif data is still available. I've had to adjust the convert_to_png function to account for two diff types of incoming image files. Rotating images at this point prevents the issue in the mod queue - in many but not all cases. In those cases, there is still the ability to manually rotate in the queue.
  • Render the facial detection data, if it exists, for superusers on the image review page to help debug image issues.
  • Resize the photo before saving: If the photo is larger than 5MB, resize it before saving it.

To test the changes to uploading an XL, rotated photo: As a superuser, upload a photo that is larger than 5MB and needs rotation to a person on a locked (or suggested lock) ballot. Do not use a photo from the current mod queue as that won't have exif data (message me if you need a photo with exif data or possibly use a phone selfie). Navigate to the mod queue. The photo should appear correctly rotated before review.

Make sure your AWS credentials are current. Run the detect faces management command to store detection_metadata on the queued_image. Run the server again and navigate to the review page; below the image and rotation buttons, check the message or detection data is rendered.

New Recording - 4/26/2023, 11:48:41 AM

Screenshot 2023-04-26 at 11 38 45 AM
Screenshot 2023-04-26 at 11 38 52 AM
Screenshot 2023-04-26 at 11 39 04 AM

Tasks

@VirginiaDooley
Copy link
Contributor Author

photo rotation

@VirginiaDooley VirginiaDooley changed the title Hotfix/photo rotation WIP Hotfix/photo rotation Apr 7, 2023
@VirginiaDooley VirginiaDooley marked this pull request as ready for review April 7, 2023 15:45
Comment on lines 56 to 58
try:
for orientation in ExifTags.TAGS.keys():
if ExifTags.TAGS[orientation] == "Orientation":
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than a try/except here, might an if/else be clearer:

if "Orientation" in ExifTags.TAGS.keys():
   # Do exif orientation tag rotation
else:
   # Do `Roll` based rotation

# Do saving the image

At the moment you don't save the image if you do the exif approach, because it's within the Except block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done as suggested replacing the try/except with if/else for clarity, but the for loop is still necessary.

Comment on lines 69 to 89
# if roll is positive, then the image needs to be rotated 90 degrees clockwise
if detected["FaceDetails"][0]["Pose"]["Roll"] > 0:
print("Rotate 90 degrees clockwise")
rotated = image.rotate(angle=90)
rotated.show()
# if roll is negative, then the image needs to be rotated 90 degrees counter clockwise
elif detected["FaceDetails"][0]["Pose"]["Roll"] < 0:
print("Rotate 90 degrees counter clockwise")
rotated = image.rotate(angle=270)
rotated.show()
# if roll is 180, then the image is upside down
elif detected["FaceDetails"][0]["Pose"]["Roll"] == 180:
print("Upside down")
rotated = image.rotate(angle=180)
image.show()
# if roll is 0, then the image is upright
else:
print("Roll is:", detected["FaceDetails"][0]["Pose"]["Roll"])
print("No rotation needed")
pass

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this approach will do what you want. Splitting the images into categories roll < 0, roll > 0 and roll == 180 isn't going to work. For one thing, the roll ==180 criteria will never be hit. There's also the issue that when roll == -165 you probably still want to rotate the image by 180° rather than 165°.

Instead I think what you want to do is : "Calculate the angle image is rotated by rounding the roll value to the nearest multiple of 90". Some examples:
-178 -> -180
75 -> 90
-12 -> 0
etc.
You can do this as: img_rotation_angle = 90 * round(roll / 90)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Hadn't gotten that far yet, but this is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved with 25833f9

@VirginiaDooley VirginiaDooley force-pushed the hotfix/photo-rotation branch 3 times, most recently from 5ab256a to 2b6b956 Compare April 12, 2023 14:36
@VirginiaDooley VirginiaDooley changed the title WIP Hotfix/photo rotation Hotfix/photo rotation Apr 12, 2023
@VirginiaDooley VirginiaDooley force-pushed the hotfix/photo-rotation branch 14 times, most recently from 8a652c4 to c4d8a32 Compare April 18, 2023 02:08
@@ -70,6 +71,7 @@ <h1>Uploaded photos for review</h1>
<td>{{ queued_image.election_date }}</td>
<td>{% if queued_image.user %}{{ queued_image.user.username }}{% else %}a robot 🤖{% endif %}</td>
<td><a href="{% url 'person-view' person_id=queued_image.person.id %}">{{ queued_image.person.id }}</a></td>
<td>{% if queued_image.detection_metadata %}Photo Angle: {{ queued_image.photo_angle }}{% else %}Image analysis is not available for this photo.{% endif %}</td>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this as a placeholder, but what would be useful to display here? Do we want to display the entire (collapsible)dict or ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 8d93e51

@VirginiaDooley VirginiaDooley force-pushed the hotfix/photo-rotation branch 2 times, most recently from c2d961c to c4d8a32 Compare April 18, 2023 14:20
@Bekabyx
Copy link
Contributor

Bekabyx commented Apr 27, 2023

My image is being autorotated when I view it in the moderation queue, but when I run the mgmt command I'm getting the following:
Skipping QueuedImage3: An error occurred (UnrecognizedClientException) when calling the DetectFaces operation: The security token included in the request is invalid. CommandError: Broken images found (see above)

Wondering if I've missed a config step or something?

@VirginiaDooley
Copy link
Contributor Author

My image is being autorotated when I view it in the moderation queue, but when I run the mgmt command I'm getting the following: Skipping QueuedImage3: An error occurred (UnrecognizedClientException) when calling the DetectFaces operation: The security token included in the request is invalid. CommandError: Broken images found (see above)

Wondering if I've missed a config step or something?

Yes, my fault - I forgot to mention you need current AWS creds to run the command. Will add that step above.

@Bekabyx
Copy link
Contributor

Bekabyx commented Apr 27, 2023

I've managed to get this working and meeting the test cases 🎉

I took a selfie on my phone, rotated it sideways, resized it to be smaller and then uploaded it. The image was automatically re-oriented to be up the right way and it pulled image data for me.
Screenshot 2023-04-27 at 17 18 15

In this process I discovered that if an image is too large (in this case, if I upload the selfie as taken from my phone without resizing) we can't run rekognition on it, but I don't know how much of a real world concern that is. Apparently they can accept a bytearray of up to 5MB (limit in error message suggests 5.24MB) - before resizing my image on disk was ~1.1MB but the error message suggests the bytearray for it was well in excess of 8MB.

I also discovered that Amazon's facial recognition has a very high opinion of me 😆

@symroe
Copy link
Member

symroe commented Apr 27, 2023

We're seeing a lot more larger images (mainly as phone cameras get better), so I wonder if we should resize the image to be no larger than, say, a 5MB PNG on upload? And then we'll get better results from rekognition too.

@VirginiaDooley VirginiaDooley force-pushed the hotfix/photo-rotation branch 2 times, most recently from 89507fd to dbbcb43 Compare April 30, 2023 13:57
@VirginiaDooley
Copy link
Contributor Author

We're seeing a lot more larger images (mainly as phone cameras get better), so I wonder if we should resize the image to be no larger than, say, a 5MB PNG on upload? And then we'll get better results from rekognition too.

Done with dbbcb43

@VirginiaDooley VirginiaDooley changed the title Rotate photo before saving Rotate AND resize photo before saving May 2, 2023
@Bekabyx
Copy link
Contributor

Bekabyx commented May 2, 2023

I've just tried this out with a photo from my phone camera - getting this back from the mgmt command

Skipping QueuedImage8: An error occurred (ValidationException) when calling the DetectFaces operation: 1 validation error detected: Value 'java.nio.HeapByteBuffer[pos=0 lim=8822186 cap=8822186]' at 'image.bytes' failed to satisfy constraint: Member must have length less than or equal to 5242880

File size according to my OS: 1,138,636 bytes (1.1 MB on disk)

Also tried it with the example image from the previous commit but I'm getting a weird error there. Not sure what would cause that but I suppose that could be a me problem.

AttributeError at /moderation/photo/upload/image/55719
'NoneType' object has no attribute 'read'

^ full trace on this one via Slack so I don't bulk up the comments here too much

@VirginiaDooley VirginiaDooley changed the title Rotate AND resize photo before saving Rotate and resize photo before saving May 2, 2023
@VirginiaDooley VirginiaDooley force-pushed the hotfix/photo-rotation branch 3 times, most recently from 571cfb4 to 8b1137c Compare May 4, 2023 05:44
@VirginiaDooley
Copy link
Contributor Author

I've just tried this out with a photo from my phone camera - getting this back from the mgmt command

Skipping QueuedImage8: An error occurred (ValidationException) when calling the DetectFaces operation: 1 validation error detected: Value 'java.nio.HeapByteBuffer[pos=0 lim=8822186 cap=8822186]' at 'image.bytes' failed to satisfy constraint: Member must have length less than or equal to 5242880

File size according to my OS: 1,138,636 bytes (1.1 MB on disk)

Also tried it with the example image from the previous commit but I'm getting a weird error there. Not sure what would cause that but I suppose that could be a me problem.

AttributeError at /moderation/photo/upload/image/55719
'NoneType' object has no attribute 'read'

^ full trace on this one via Slack so I don't bulk up the comments here too much

I've refactored the helpers to handle the correct file types at each of the rotate, resize, convert steps in the save process. 8b1137c

@Bekabyx
Copy link
Contributor

Bekabyx commented May 5, 2023

Seeing some odd behaviour now. The example photo is now being rotated clockwise (even though the photo being uploaded is already oriented correctly) but I am able to run facial recognition on it now.

The selfie I've been testing with is still not able to do facial recognition, but I don't know how much of a problem that is.

@VirginiaDooley
Copy link
Contributor Author

Seeing some odd behaviour now. The example photo is now being rotated clockwise (even though the photo being uploaded is already oriented correctly) but I am able to run facial recognition on it now.

The selfie I've been testing with is still not able to do facial recognition, but I don't know how much of a problem that is.

I see this too on some photos (not my selfie). On further inspection, these cases do not appear to go through the rotation step at all. It would be good to look at this together.

@VirginiaDooley
Copy link
Contributor Author

Seeing some odd behaviour now. The example photo is now being rotated clockwise (even though the photo being uploaded is already oriented correctly) but I am able to run facial recognition on it now.

The selfie I've been testing with is still not able to do facial recognition, but I don't know how much of a problem that is.

UPDATE: I dug deep back into the rotate images bug and made some changes. The behaviour @Bekabyx Bek found confirmed that my assumptions about photos with an orientation set to exif[274] == 1 were too grand, and it is in fact possible to have a selfie that does not need rotation with this data. The refactor eliminates the potential for rotating images that don’t need rotation, but limits the potential for selfies to be rotated before saving if they do have exif[274] == 1. In these cases, there is always the potential to manually rotate in the review process.

@Bekabyx
Copy link
Contributor

Bekabyx commented May 15, 2023

I've given this another run through with just a normal photo and everything still seems to be working so I'm happy to approve. We can always revisit again if we want it to be all-singing-all-dancing, but with the manual rotate functionality I think it's okay to leave it at this at least for now 😁

@VirginiaDooley VirginiaDooley merged commit a55220b into master May 22, 2023
4 checks passed
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.

None yet

4 participants