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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset-exif #1247

Closed
wants to merge 3 commits into from
Closed

Reset-exif #1247

wants to merge 3 commits into from

Conversation

Sarabadu
Copy link

@Sarabadu Sarabadu commented Jun 23, 2023

What's Changing and Why

fixes #1104
After doing the exif rotation, we reset the exif orientation to 1

see this commit to compare before and after this changes 48179a1

What else might be affected

Note: test are still failing because the calculated distance seems quite bigger now (can be the case that the hash is also considering exif metadata in consideration 馃, should be like that or should only consider the actual image data??)

Tasks

  • Add tests
  • Update Documentation
  • Update jimp.d.ts
  • Add SemVer Label

@hipstersmoothie
Copy link
Collaborator

@Sarabadu build isn't working. could you update?

@Sarabadu Sarabadu force-pushed the reset-exif branch 2 times, most recently from 18e33f3 to a429738 Compare July 26, 2023 22:40
@Sarabadu
Copy link
Author

Sarabadu commented Jul 26, 2023

@hipstersmoothie added an empty polyfill for the util.inspect used on modify_exif

btw looking for this util use, I realised that modify_exif seems not maintained anymore (and also the lib main dependency). Didn't find any better alternative out there, so if you feel not confident enough, we can close this PR

@bobzel
Copy link

bobzel commented Dec 10, 2023

Is this branch going to be updated further? The problem it's addressing is very important for making this library be a replacement for Sharp. I

@Sarabadu
Copy link
Author

@bobzel this solution was working for my use case and also seems to work on the test, but didn't get more feedback on this.

the solution here uses an apparently unmantained dependency. so not sure if this PR will be merged anyways. i used modify-exif after jimp for make it work. hope it helps

@hipstersmoothie
Copy link
Collaborator

@Sarabadu I'd merge if the tests pass :D

@Sarabadu
Copy link
Author

@Sarabadu I'd merge if the tests pass :D

@hipstersmoothie, as i mentioned in the description, the issue with the test is the calculated distance is also calculating exif information.

not sure how to solve that tbh, changing the expected distance values doesn't give much value, I think, any suggestion? remove those tests? any idea how to solve this on the distance function(seems like a breaking changes) ?

WDYT?

Signed-off-by: Juan Pablo Garcia Ripa <sarabadu@gmail.com>
Signed-off-by: Juan Pablo Garcia Ripa <sarabadu@gmail.com>
Signed-off-by: Juan Pablo Garcia Ripa <sarabadu@gmail.com>
@Sarabadu
Copy link
Author

@hipstersmoothie in case you miss it i made the changes to the tests, feel free to run the CI

@hipstersmoothie
Copy link
Collaborator

@Sarabadu Hey! I'm currently working on a major rewrite of everything and your changes are already included. No tba, still have a few things to figure out

@Sarabadu
Copy link
Author

Glad to hear @hipstersmoothie In that case feel free to close this PR.

If you are doing
A mayor rewrite, maybe you can also try to not use the modify-exif too

@hipstersmoothie
Copy link
Collaborator

I'm not sure what that means but feel free to make a pr to the v1 branch !

@Sarabadu Sarabadu changed the base branch from main to andrew/v1 April 11, 2024 08:31
@Sarabadu
Copy link
Author

closing it as seems it will be solved in the rewrite

@Sarabadu Sarabadu closed this Apr 11, 2024
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.

Processed image is rotated from portrait to landscape
3 participants