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

parse JFIF APP0 sections and expose Density with DensityUnits #115

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

Conversation

cormacrelf
Copy link

@cormacrelf cormacrelf commented Oct 21, 2019

See image-rs/image#1067

Possible improvements

  • Rename to match png::PixelDimensions, which is not really an accurate name because it doesn't represent the size of a pixel, but the density of an image.
  • Remove the thumbnail stuff completely (I added it because I was there, and maybe someone wants it? I drew the line at allocating a buffer for the packed thumbnail data.)
  • Add tests? The current infrastructure doesn't appear capable of testing the whole range of possible inputs as PNG's density information has fewer units options. But I did verify with a dbg!() that the reftest images with JFIF APP0 sections could decode the density information.

@cormacrelf
Copy link
Author

cormacrelf commented Oct 21, 2019

The one failure on stable-1.28.0 is because cfg-if has an edition key in its Cargo.toml, which appears like so:

jpeg-decoder v0.1.16 (/Users/cormac/git/jpeg-decoder)
├── byteorder v1.3.2
└── rayon v1.2.0
    ├── crossbeam-deque v0.7.1
    │   ├── crossbeam-epoch v0.7.2
    │   │   ├── arrayvec v0.4.12
    │   │   │   └── nodrop v0.1.14
    │   │   ├── cfg-if v0.1.10
    │   │   ├── crossbeam-utils v0.6.6
    │   │   │   ├── cfg-if v0.1.10 (*)

Seems pretty weird to me as I thought the edition flag was stable already. Maybe it's a bad message because using edition = "2018" wasn't stable until 1.32.

@@ -58,7 +60,7 @@ pub struct Decoder<R> {

restart_interval: u16,
color_transform: Option<AdobeColorTransform>,
is_jfif: bool,
jfif_app0: Option<JfifApp0>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are color_transform and jfif_app0 mutually exclusive? If so, perhaps this could hold an AppData or Option instead?

@fintelia
Copy link
Contributor

This PR generally looks good to me, though I agree that it should probably either fully decode thumbnail images or just ignore them.

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.

2 participants