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

feat: extend bimg with support for autorotate #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@haf
Copy link
Author

haf commented Aug 21, 2017

Ref #180

@greut
Copy link
Contributor

greut commented Aug 21, 2017

What's wrong with NoAutoRotate? As it doesn't have any tests, it's behaviour could be wrong.

@haf
Copy link
Author

haf commented Aug 22, 2017

It's the negation of what I want? And the rotate call requires an angle?

@greut
Copy link
Contributor

greut commented Aug 22, 2017

By default, it reads the EXIF data and autorotates the picture. At least, it should do that.

@haf
Copy link
Author

haf commented Aug 22, 2017

I haven't tested this lib stand-alone, but calling rotate with a zero angle fails in imaginary, even after removing the input validation that the angle is not zero.

How do I invoke this library's auto-rotate via imaginary to have it only auto-rotate?

@greut
Copy link
Contributor

greut commented Aug 22, 2017

Does norotation changes anything? In any case, bimg could have a test case for that.

https://github.com/h2non/imaginary/blob/a1bef74cc15b78a08085a0859c51c9372a0ee593/README.md#params

@greut
Copy link
Contributor

greut commented Aug 23, 2017

@haf
Copy link
Author

haf commented Aug 23, 2017

I just spent 14 hours straight (minus lunch) implementing this feature in the browser with FileReader and ArrayBuffer and Canvas, so I'm pretty into the different rotation modes right now ;)

All the code you find online when googling is wrong.

@haf
Copy link
Author

haf commented Aug 23, 2017

All cases handled: https://codepen.io/haf/pen/OjEBao

@greut
Copy link
Contributor

greut commented Aug 24, 2017

@haf the sole fact that bimg EXIF interpretation doesn't do a flop means it's wrong.

https://github.com/h2non/bimg/blob/master/resize.go#L290-L296

I'm onto some test cases.

@h2non
Copy link
Owner

h2non commented Aug 25, 2017

Thanks for the help, @greut. I can't invest time on this right now, but feel free to send a PR with the required fixes, if needed. There might be something wrong in the implementation.

@h2non
Copy link
Owner

h2non commented Aug 25, 2017

After seeing this for a bit, I think it would be more convenient relying on libvips built-in implementation = vips_autorot() and optionally allow the user overwrite that behavior by a specific rotation angle. @haf implementation looks good for this, but resize.go logic requires some (non-breaking) changes. Opinions?

@h2non
Copy link
Owner

h2non commented Aug 25, 2017

Apparently, libvips API provides vips_autorot() since v7.42+, which is the minimum version currently supported by bimg.

@greut
Copy link
Contributor

greut commented Aug 25, 2017

@h2non libvips can even perform the autorotate when loading the file. Once we have some test cases #183, we can proceed with the refactoring.

@greut
Copy link
Contributor

greut commented Aug 25, 2017

Regarding the refactoring using autorotate, it works.

 int
-vips_init_image (void *buf, size_t len, int imageType, VipsImage **out) {
+vips_init_image (void *buf, size_t len, int imageType, int autorotate, VipsImage **out) {
        int code = 1;
 
        if (imageType == JPEG) {
-               code = vips_jpegload_buffer(buf, len, out, "access", VIPS_ACCESS_RANDOM, NULL);
+               code = vips_jpegload_buffer(buf, len, out,
+                       "access", VIPS_ACCESS_RANDOM,
+                       "autorotate", with_interlace(autorotate),
+                       NULL
+               );

But libvips doesn't handle all the EXIF cases 😭, so it's not usable as is.

https://github.com/jcupitt/libvips/blob/e241d1333926180c5258dbf6650d0c1af7538f7a/libvips/conversion/autorot.c#L62-L109

@h2non
Copy link
Owner

h2non commented Aug 25, 2017

Right, dammit!

@haf
Copy link
Author

haf commented Aug 25, 2017

Just convert my four lines of code + sin ± check into a transformation matrix? http://www.vips.ecs.soton.ac.uk/supported/8.3/doc/html/libvips/libvips-resample.html

@h2non
Copy link
Owner

h2non commented Sep 10, 2017

Can you check now that the default bimg automatic rotation implemtnation doesn't work for you as expected? @greut improvements are now available in latest bimg version. Try it again upgrading it:

go get -u gopkg.in/h2non/bimg.v1

@haf
Copy link
Author

haf commented Sep 10, 2017

Have you changed libvips to support all cases?

@greut
Copy link
Contributor

greut commented Sep 11, 2017

@haf since #183 got merged in, all cases should be handled.

@haf
Copy link
Author

haf commented Sep 11, 2017

Can you give me a sample command line to run to test it, please?

@greut
Copy link
Contributor

greut commented Sep 11, 2017

@haf bimg != libvips, afaik there are no command-line tools with bimg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants