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

Add rotation support #133

Closed
jamesmoss opened this issue Mar 7, 2018 · 30 comments
Closed

Add rotation support #133

jamesmoss opened this issue Mar 7, 2018 · 30 comments

Comments

@jamesmoss
Copy link

It would be good to be able to specify a rotation angle (in multiples of 90 degrees) to be applied to the image before resizing.

We have a use case where on upload we need to read exif orientation data, rotate the image correctly, resize, and then send to the server.

Currently we have to do this by reading the image into a canvas on the main UI thread which negates the point of using pica and webworkers. By only supporting 4 angles for rotation it shouldn't be too hard to read the pixel data out in a different order and skip using CanvasRenderingContext2D.rotate().

@puzrin
Copy link
Member

puzrin commented Mar 7, 2018

That's out of project scope. It works with raw binary data and doesn't know anything about image formats, exifs and so on.

@puzrin puzrin closed this as completed Mar 7, 2018
@jamesmoss
Copy link
Author

@puzrin I think you've closed this too early.

I'm not saying the library should parse EXIF data, I will do that in my own code. But I want to ask pica to do the rotation; as you say it just deals with raw binary data, its easy to do rotation by reading the pixel data out in a different way (e.g instead of left -> right, you read right -> left, same by swapping X/Y values).

@puzrin
Copy link
Member

puzrin commented Mar 7, 2018

Answer is the same. Pica is low level lib for resize, not "image editor". I don't like to mix different things, when those can be kept separate.

@jamesmoss
Copy link
Author

I agree that keeping the feature set focussed is important and i'm not proposing any "image editor" functions but right now, to fix my problem I'd have to duplicate so much logic around reading pixel data, checking browser capabilites, spawning webworkers etc. and thats before even passing the image to pica and all that process happening all over again. Performance is super important.

I believe this can be added with a few lines of code - would you be prepared to accept a PR if I can get it working?

@puzrin
Copy link
Member

puzrin commented Mar 7, 2018

I don't like this idea. "Code duplication" is not as horrible as you describe - you don't need almost all checks done by pica and don't need webworkers. Performance win will be not significant too.

@BananaAcid
Copy link

BananaAcid commented Sep 17, 2019

@jamesmoss did you start a fork supporting rotation? Rotating by WASM + WebWorker would help tremendously. I have the same topic to cover. Just adding this basic functionality as well, would have saved my (mobile) world ;)

@jamesmoss
Copy link
Author

Unfortunately I never got the time to do it, if you do write something to solve this I'd love to try it out.

@BananaAcid
Copy link

@jamesmoss any other solution you could point me to?

@garygreen
Copy link

garygreen commented Dec 5, 2019

I know this is an old issue but I too would like to share my interest in this.

Often images taken on cell/mobile cameras will have a rotatation marker in the EXIF data to show that it was taken when rotated 90degrees, etc. Currently with pica, you need to handle EXIF data yourself and rotate the image prior to upload - or you keep the EXIF data (again, you need to do this yourself) and do the rotations on the server.

I know this could be seen as "image editor" in a way but isn't "image resizing" also editing the image? This kind of rotation is very common and incredibly valuable.

It would be good to have maybe a seperate library or code that you can import to handle post-resize rotations, or restitching EXIF data back onto the image in the context of a web worker. It's those kind of technical details libraries like this could solve for developers.

@puzrin
Copy link
Member

puzrin commented Dec 5, 2019

That's completely different abstraction layers, not suitable into this package/api. If you need to patch orientation, you may wish to scrape code from here

@garygreen
Copy link

@puzrin

The fact that your doing this kind of patching in your own implementation using your own library shows that it's valuable.

@puzrin
Copy link
Member

puzrin commented Dec 5, 2019

This is absolutely correct and absolutely useless statement :). It contains nothing constructive to apply for this package :)

@garygreen
Copy link

@puzrin

Sorry but given the limited communication I've had with you over the last few comments it seems like you have an unnesscarily defensive attitude. I question why you maintain open source projects if you aren't open to suggestions from the community. To put it into perspective, if you felt you had valid suggestion or contribution to another open source project you used and you were shun and dismissed in a similar tone I'm sure you wouldn't appreciate that much either. Politeness costs nothing. Anyway, just an observation. Wish you all the best with the project. 🙂

@puzrin
Copy link
Member

puzrin commented Dec 5, 2019

@garygreen That may be problem of language barrier, english is not my native language. Please, read this statements:

  1. Your suggestions were not valid (but i understand you may think opposite, because you may be not familiar with more deep details)
  2. I suggested you alternative to save your time (link to almost ready code for outer wrapper).

Consider this as pure neutral statements. IMO that's constructive. Sorry if i expressed that not enough well.

@BananaAcid
Copy link

That's completely different abstraction layers, not suitable into this package/api. If you need to patch orientation, you may wish to scrape code from here

@puzrin Sorry - I can't seem to find it, could you point out which line the almost-ready-code for rotation code can be found at?

@puzrin
Copy link
Member

puzrin commented Dec 6, 2019

@BananaAcid sorry, that file pipes headers through as is. Because uploader should care about size but not about orientation. Take a look at this place https://github.com/nodeca/nodeca.users/blob/master/client/users/avatar/change/change.js#L417. That's avatar setter, it cares about orientation.

@garygreen
Copy link

@BananaAcid I've stumbled across https://github.com/blueimp/JavaScript-Load-Image which handles resizing, orientation fixes, exif data etc. Seems a overall more complete solution that pica. Haven't used it yet, but it's maybe something of help for you too.

@puzrin
Copy link
Member

puzrin commented Dec 6, 2019

@garygreen i'd suggest you to check resize quality first. That package use canvas built-ins. Those usually apply box filer instead of lanczos. That's a reason why pica was created - to guarantee predictable and high quality.

But in general, yes, you need more high level wrapper for pica, something like JavaScript-Load-Image.

@garygreen
Copy link

garygreen commented Dec 6, 2019

@puzrin yes your correct it doesn't appear to be as high quality as pica. That's one of the primary attractions of pica, it's just a shame that it doesn't easily support EXIF retention and optional rotation out the box.

It would be great if there was at least a easy guide in the readme on how to do that. I know you linked to the external code of your app, but that's not exactly a "ready to go" piece of code as you say, it takes a fair bit of time to decipher what it's doing.

Though I know your not willing for a PR for such functionality so we have to consider alternative options.

@grahams
Copy link

grahams commented Dec 6, 2019

@garygreen Perhaps a different approach would be to submit a PR to JavaScript-Load-Image to augment/replace it's scaling code with Pica.

As someone who's been using Pica in our product for years, I truly appreciate @puzrin's focus on keeping the scope of this library from creeping (especially since I only need pica's scaling, preserving EXIF tags or rotation would be completely wasted on me).

That said, I also understand those who want it to do more. Since @puzrin has made his position rather clear, I wonder if there is still a possible compromise? @puzrin I know you don't want to add this functionality to the library, but would you accept a PR containing some documentation on how to use pica (along with other tools, when necessary) to accomplish some of these tasks we repeatedly see?

@garygreen
Copy link

As someone who's been using Pica in our product for years, I truly appreciate @puzrin's focus on keeping the scope of this library from creeping

I completely agree. It's great when library authors are strict about what's included and keep the size of the library down. Concidentially, I made a suggestion about tree shaking but the author didn't seem that interested in it, so not entirely convinced that the author's primary goal is to keep the size of the library down. In any case, the suggestions made here are an optional inclusion and shouldn't burden anyone who doesn't want this feature.

especially since I only need pica's scaling, preserving EXIF tags or rotation would be completely wasted on me

Out of interest, how come you don't want to preserve EXIF tags and handle orientation markers? Are the images guaranteed in your application to not contain them? Pictures taken on camera phones pretty much always have them.

The problem is that we need a way of preserving the EXIF so we can fix the orientation on the server. But then that also negates the "high quality" attraction of pica because you are then doing an extra mutation on the server and reducing the quality of the picture. Ideally it should all be done at source.

As you say, it would be great if there was some kind of guide on how to achieve this with pica, using external exif libraries, rotations, etc. That doesn't sound like an unreasonable suggestion to me.

@puzrin
Copy link
Member

puzrin commented Dec 6, 2019

@puzrin I know you don't want to add this functionality to the library, but would you accept a PR containing some documentation on how to use pica (along with other tools, when necessary) to accomplish some of these tasks we repeatedly see?

See my uploader link. IMO too much code for readme (for correct exif cleanup & transfer). IMO it would be better if someone create high level wrapper as separate package. Because blob processing is completely different thing.

@garygreen
Copy link

garygreen commented Dec 6, 2019

IMO it would be better if someone create high level wrapper as separate package

My understanding is if you rotate the image with a seperate package then your likely not using lanczos sampling so you negate any quality benefits of using pica. Wouldn't the resize need to happen at source to keep the quality benefits?

@grahams
Copy link

grahams commented Dec 6, 2019

@garygreen Our product is a image/document viewer (http://html5.snowbound.com), and when we work with images they are already preprocessed by our backend.

Our use case is that in the past, we would scale the rasterized docs in the backend as the user zoomed in/out of the image, and while that worked well, it was slow/latent. By using pica we send a 'full size' (which is fuzzy in the case of document formats) raster to the client and then use Pica to scale it in real-time client side.

(These days we are generally working with SVG for the document/vector formats, so Pica isn't involved in that workflow at all...)

@puzrin
Copy link
Member

puzrin commented Dec 6, 2019

My understanding is if you rotate the image with a seperate package then your likely not using lanczos sampling so you negate any quality benefits of using pica. Wouldn't the resize need to happen at source to keep the quality benefits?

IMO you need apply orientation only if you wish display image immediately on client. If you do uploader only - then proper exif transfer is enough, and server side code will care about the rest.

I provided 2 links with both approaches.

@garygreen
Copy link

If you do uploader only - then proper exif transfer is enough, and server side code will care about the rest.

We NEVER want to show an image upside down. So we need to do resize on client and then a resize on the server. That's two quality changes. As mentioned, this ideally should be done at source to preserve maximum image quality. Anyway, it's clear this isn't something your interested in adding, even as a readme thing so we'll leave it there.

@puzrin
Copy link
Member

puzrin commented Dec 6, 2019

Seems you mix completely different things.

  1. You can resize image on client WITOUT orientation update IF you keep orientation info in output. All problems with output image display happens when people miss orientation info.
  2. See https://github.com/nodeca/pica#prior-to-use. Pica's quality is good enougth but NOT ideal, due technical constrains. Dynamic range will be reduced.

@BananaAcid
Copy link

@puzrin #1 is a point, I did not actually think about.
How would that be solved?

  1. backup exif orientation
  2. resize image (taking orientation into account for non-square)
  3. stick exif orientation back into blob

Any drawing onto a canvas removes the orientation data, but any <img> or background-image: .. would work ..

Would that be a way to go?

Reason: That would save on rotation calculations (important for me, doing stuff for mobile devices /PWA - any wasm ist a life saver, doing large img stuff in canvas will usually stall on iPhoneSE .. Galaxy S5 ..).

@puzrin
Copy link
Member

puzrin commented Dec 11, 2019

Would that be a way to go?

Yes, but you probably my need to cleanup color profile info. Please take a look at my links of uploader and avatar setter.

@puzrin
Copy link
Member

puzrin commented Jul 8, 2020

https://github.com/nodeca/image-blob-reduce - here is pica wrapper for files. It cares about orientation and other file-specific things.

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

No branches or pull requests

5 participants