Skip to content

feat: extend bimg with support for autorotate#181

Open
haf wants to merge 1 commit into
h2non:masterfrom
haf:feature/auto-rotate
Open

feat: extend bimg with support for autorotate#181
haf wants to merge 1 commit into
h2non:masterfrom
haf:feature/auto-rotate

Conversation

@haf

@haf haf commented Aug 21, 2017

Copy link
Copy Markdown

@haf

haf commented Aug 21, 2017

Copy link
Copy Markdown
Author

Ref #180

@greut

greut commented Aug 21, 2017

Copy link
Copy Markdown
Contributor

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

@haf

haf commented Aug 22, 2017

Copy link
Copy Markdown
Author

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

@greut

greut commented Aug 22, 2017

Copy link
Copy Markdown
Contributor

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

@haf

haf commented Aug 22, 2017

Copy link
Copy Markdown
Author

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

greut commented Aug 22, 2017

Copy link
Copy Markdown
Contributor

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

greut commented Aug 23, 2017

Copy link
Copy Markdown
Contributor

@haf

haf commented Aug 23, 2017

Copy link
Copy Markdown
Author

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

haf commented Aug 23, 2017

Copy link
Copy Markdown
Author

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

@greut

greut commented Aug 24, 2017

Copy link
Copy Markdown
Contributor

@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

h2non commented Aug 25, 2017

Copy link
Copy Markdown
Owner

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

h2non commented Aug 25, 2017

Copy link
Copy Markdown
Owner

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

h2non commented Aug 25, 2017

Copy link
Copy Markdown
Owner

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

@greut

greut commented Aug 25, 2017

Copy link
Copy Markdown
Contributor

@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

greut commented Aug 25, 2017

Copy link
Copy Markdown
Contributor

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

h2non commented Aug 25, 2017

Copy link
Copy Markdown
Owner

Right, dammit!

@haf

haf commented Aug 25, 2017

Copy link
Copy Markdown
Author

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

h2non commented Sep 10, 2017

Copy link
Copy Markdown
Owner

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

haf commented Sep 10, 2017

Copy link
Copy Markdown
Author

Have you changed libvips to support all cases?

@greut

greut commented Sep 11, 2017

Copy link
Copy Markdown
Contributor

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

@haf

haf commented Sep 11, 2017

Copy link
Copy Markdown
Author

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

@greut

greut commented Sep 11, 2017

Copy link
Copy Markdown
Contributor

@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