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

Allow setting a custom pixel density in the JPEG encoder #1078

Merged
merged 2 commits into from
Nov 23, 2019

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Nov 23, 2019

This pull request allows setting a custom DPI value for JPEG images (the encoder used to hardcode a pixel aspect ratio of 1x1 for all images).

Fixes #1077

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to chose either at their option.

The encoder used to hardcode a pixel aspect ratio of 1x1
It allows setting a custom DPI value.

Fixes image-rs#1077
@fintelia
Copy link
Contributor

It might be good to have the pixel density setting be part of the ImageEncoder trait that @aschampion is looking into adding for #1076

@lovasoa
Copy link
Contributor Author

lovasoa commented Nov 23, 2019

@fintelia : do you want to wait until ImageEncoder is merged, and then re-do this PR against the new trait ? Or can we merge this one first, and then let the ImageEncoder PR use that ?

@lovasoa
Copy link
Contributor Author

lovasoa commented Nov 23, 2019

Also: Do you think it would be a good idea to reuse the types defined in image-rs/jpeg-decoder#115 once it is merged ?

@aschampion
Copy link
Contributor

It might be good to have the pixel density setting be part of the ImageEncoder trait that @aschampion is looking into adding for #1076

Many of the format encoders' constructors and parameters are similar, so it might eventually make sense to make some common subset of these or other settings part of ImageEncoder's constructor/builder/configuration, but for now I was going to abstract only the encode method as a minimal initial change. So one still manually constructs the format encoder they want (as in save_buffer_impl), which would not conflict with these changes. No need to wait on my PR.

Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

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

Other than changing the return type, I think it is worth going ahead and merging these changes. Longer term, I'd like to have more consistent interfaces for decoders (and probably merge jpeg encoding and decoding into the same crate) but we shouldn't let that delay progress now. We're still not really close to 1.0 so we are free to make breaking changes as needed

src/jpeg/encoder.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

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

I'll merge this once CI finishes

@fintelia fintelia merged commit 2e35996 into image-rs:next Nov 23, 2019
@lovasoa
Copy link
Contributor Author

lovasoa commented Nov 24, 2019

Thank you !

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.

3 participants