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

16 bit per channel image buffers and dynamic images #1085

Merged
merged 13 commits into from Dec 12, 2019

Conversation

aschampion
Copy link
Contributor

Notes:

  • Tests will fail until the image-png dependency can be updated to include Only expand bit depths less than 8 to 8 image-png#174, so this is not yet mergeable. Tests pass with current image-png master. I'm going ahead and PRing since there may be revisions needed.
  • Nomenclature: the PR refers to channels and bits per channel and not subpixels (as is used in Pixel), because in the scientific and biomedical image processing communities subpixels are samples at higher spatial resolution than pixels, different from the channel components of a pixel.
  • To make this as noncontroversial as possible to merge, minimal changes are made to the DynamicImage interface, even if this results in extra boilerplate or obvious missing conversions. This can be fleshed out later.
  • One notable deletion is DynamicImage::as_flat_samples, because this cannot have a generic return type. While there are alternatives, a code search of GitHub turned up no cases where as_flat_samples was used on DynamicImage directly, only where it is used on ImageBuffer, which still works.
  • Only u16 16 bit colors are supported, because having more general support for other bit depth types isn't really feasible without specialization or some new type magic. Since, practically speaking, only u8 is fully supported for most existing images I assume this is fine.
  • This is a breaking change for anyone using the save_buffer free functions to save 16 bit per channel PNGs, because those functions now take native endian buffers rather than format-specific endian (big endian for PNG). Anyone using PNGEncoder directly is unaffected.

@fintelia
Copy link
Contributor

I don't have time for a full review right at the moment, but I do have a couple of comments from skimming:

  • The channel_bytes function might break if future color types have channels that aren't an integer number of bytes or if different channels had different bit depths (we guarantee that the overall pixel takes an integer number of bytes, but that is a weaker assumption).
  • ChannelsType should probably be non-exhaustive and ExtendedColorType::channels_type() should return an option.
  • This PR emphasizes how awkward it is that our pixel structs are generic and thus we can't fully constrain them to supported inner types. If they weren't generic, we could also have them derive zerocopy::{FromBytes, AsBytes} which would simplify a couple things/eliminate some unsafe code. At some point we probably need to just replace them with separate structs for each variant of ColorType.

Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

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

Let me preface this with saying that the bulk of these look nice, contained, and well motivated.

Nomenclature: the PR refers to channels and bits per channel and not subpixels (as is used in Pixel), because in the scientific and biomedical image processing communities subpixels are samples at higher spatial resolution than pixels, different from the channel components of a pixel.

Definitely preferred in any case. We might want to make a note to replace remaining mentions of subpixel in some naming related issue.

Only u16 16 bit colors are supported, because having more general support for other bit depth types isn't really feasible without specialization or some new type magic. Since, practically speaking, only u8 is fully supported for most existing images I assume this is fine.

I would assume so as well.

This is a breaking change for anyone using the save_buffer free functions to save 16 bit per channel PNGs, because those functions now take native endian buffers rather than format-specific endian (big endian for PNG). Anyone using PNGEncoder directly is unaffected.

One thing to remember for the change notes. Definitely do remind me if I forget to include it.

src/buffer.rs Outdated Show resolved Hide resolved
src/color.rs Outdated Show resolved Hide resolved
src/color.rs Show resolved Hide resolved
src/image.rs Show resolved Hide resolved
src/buffer.rs Outdated Show resolved Hide resolved
src/dynimage.rs Show resolved Hide resolved
@HeroicKatora
Copy link
Member

HeroicKatora commented Nov 27, 2019

ChannelsType should probably be non-exhaustive and ExtendedColorType::channels_type() should return an option.

If both ExtendedColorType and ChannelsType are marked non-exhaustive we could also require new variants of ExtendedColorType to also create a new variant in ChannelsType if required.

@aschampion
Copy link
Contributor Author

Thanks for the comments. I should have time to make changes/respond this weekend.

@aschampion aschampion force-pushed the features/16bpc-dynimage-squash branch from 2687d5d to 6bc4ca4 Compare December 4, 2019 18:25
@aschampion
Copy link
Contributor Author

ChannelsType should probably be non-exhaustive and ExtendedColorType::channels_type() should return an option.

If both ExtendedColorType and ChannelsType are marked non-exhaustive we could also require new variants of ExtendedColorType to also create a new variant in ChannelsType if required.

ChannelsType is now non-exhaustive, but per @HeroicKatora's comment I assumed that it covers the set of channel types in ExtendedColorType. The maintenance burden for adding to ChannelsType is low because it's used infrequently, unlike ColorType, so it seems ok to require it to cover extended cases.

At some point we probably need to just replace them with separate structs for each variant of ColorType.

I went down this road with the PR first, but found it a mess of macros as so much logic is and should be shared generically based upon the channel type of the pixel. There's also an n^2 number of conversions between different pixel structs, and the current ability to use u64/f32/f64 buffers is sometimes useful even if not ergonomic.

@aschampion aschampion force-pushed the features/16bpc-dynimage-squash branch from a571984 to d8f6163 Compare December 4, 2019 18:55
.travis.yml Outdated
@@ -10,7 +10,7 @@ notifications:
os:
- linux
rust:
- 1.34.2
- 1.35.0
Copy link
Member

Choose a reason for hiding this comment

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

Oh darn. Given that zerocopy only requires the feature of 1.35 for two lines of code that we do not make use of.
See: https://fuchsia.googlesource.com/fuchsia/+/master/garnet/public/rust/zerocopy/src/lib.rs#1065

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'd prefer to go to a lower version of zerocopy instead if possible, but haven't had time to go through their history since there's no changelog. The compiler bump here is just for CI testing the rest of the PR, unless doing a one version bump wouldn't be a dealbreaker.

Copy link
Member

Choose a reason for hiding this comment

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

There is none, I think. It was introduced somewhere around 0.2.2 for sure, I've waited on that feature being stabilized in image-canvas. And note that all version prior to 0.2.3 have an unsound interface see here.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this would entail restricting the version with an upper bound since cargo does not select crates based on compiler requirements (yet). So in essence, we would require an unsound version.. Not the route we want to go probably.

Copy link
Member

Choose a reason for hiding this comment

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

So, this leaves us with two choices:

  • Bump the compiler version. This is somewhat contrary to making this as noncontroversial as possible to merge but honestly, it is a very large and useful feature and the PR is against the major version branch so it is a real possibility 👍
  • Use something other than zerocopy. In the long term it is maybe the most sound solution available but this PR uses only a very small amount of its features so another crate (or porting the code in question, note that this route would require a sealed trait imo) could be a better fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacing zerocopy with code in the crate would be easy, < 4 lines of unsafe that's clearly safe as it's casting between integer slice types, but given how close image is to being unsafe-free it would be nice to use zerocopy or another similarly-proven alternative until when/if safe transmutes arrive.

If zerocopy would be useful elsewhere in the crate that could be an argument in its favor. @fintelia mentioned this for pixel structs, which could derive AsBytes if Primitive had an AsBytes bound. This might even get at removing the last two unsafes.

For Rust version bumping, usually my opinion would be that it's a one week bump in the version number five months since image's last MSRV bump, so seems fine, but I also see the motive for staying with 1.34.2 in particular because its approximate to a LTS version being in Debian buster.

Copy link
Member

Choose a reason for hiding this comment

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

This already matches my own thoughts closely, the PR makes use of &[u16] -> &[u8] conversion (trivial unsafe) and &[u8] -> Option<&[u16]> (not quite trivial but also not incredibly complex). There are no preconditions for the first conversion and the alignment checks that need to be done at runtime for the second case are clear.

With regards to safety, I don't think it matters particularly if zerocopy provides these or we include them here. First of all it is a small finite set, none depend on type parameters and the publicly exposed interface of it is rather minimal. In a way, one could say the (total) unsafety introduced by depending on zerocopy is greater since these are not true for that crate!

The potential introduction into std (although similar thoughts had been voiced in previous years and still nothing concrete has happened) also points me in the direction of not bumping the MSRV because we would certainly prefer using the standard utilities over zerocopy where possible. Using zerocopy for small benefit now means we'll be doing another one soon (compatibility with 1.24.1 was kept over ~1.5 years).

I lean towards implementing the conversions in utils, but this conclusion and reasoning should be made clear in the implementation. It should be possible to re-evaluate these argument for any future extension made, we do not want to expand to duplicate zerocopy and if it proves necessary that any of this becomes dependent on a user provided type parameter we should switch, imho.

Before the implementation, cc @fintelia Thoughts? I know you lean towards less unsafety here so I'd be interested in knowing which of these options you regard as safer.

src/color.rs Outdated Show resolved Hide resolved
src/color.rs Outdated Show resolved Hide resolved
src/color.rs Outdated Show resolved Hide resolved
src/buffer.rs Outdated Show resolved Hide resolved
@aschampion
Copy link
Contributor Author

With ChannelsType removed, I think all comments have been addressed.

The remaining decision, then, is this:

  • Bump the compiler version. This is somewhat contrary to making this as noncontroversial as possible to merge but honestly, it is a very large and useful feature and the PR is against the major version branch so it is a real possibility 👍
  • Use something other than zerocopy. In the long term it is maybe the most sound solution available but this PR uses only a very small amount of its features so another crate (or porting the code in question, note that this route would require a sealed trait imo) could be a better fit.

If we are fine with going forward with zerocopy, I'll squash the cleanup commits and rebase on next to fix the merge conflict. If we're not, let me know and I'll start looking at alternatives.

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 think things are in good shape at this point

src/buffer.rs Outdated Show resolved Hide resolved
src/color.rs Outdated Show resolved Hide resolved
src/color.rs Outdated Show resolved Hide resolved
Add 16 bit buffers for all 16 bit types in ColorType. Include these as
supported types in DynamicImage. These 16 bit images are currently only
supported with u16 representation; changing this would require
specialization and macroing of color conversions.

Load 16 bit PNGs at full bit depth rather than squashing to 8 bit.

Note that tests will not pass until upgrading to a release of image-png
including image-rs/image-png#174.

Closes image-rs#560.
Closes image-rs#665.
Closes image-rs#940.
This ImageEncoder trait is currently minimal, but establishes a uniform
interface so that all formats have native endian encoding interfaces,
which is necessary for 16 bit per channel encoding.

Closes image-rs#1076.
This fixes 16bpc tests.
Per review comments, this assumes equal channel bit depths, which will
not be the case in the future.
Because the return type depends on the buffer's bits per channel, make
the return optional and provide separate methods for 8 and 16 bit
images.
Instead create a crate-local AsBytes wrapper trait. This trait is not
sealed as Primitive was not sealed, so that users can delegate it for
newtypes if needed.
@aschampion
Copy link
Contributor Author

Nits done, rebased, and fix commits squashed. I could squash out the ChannelsType and Pixel::COLOR_TYPE noise entirely if that's preferred.

@Shnatsel
Copy link

If zerocopy requires too new compiler and/or is too complex, you can use https://github.com/Lokathor/bytemuck instead. It is rather simple internally and passes MIRI on its test suite, which checks for alignment issues as well as a number of Rust-specific rules such as &mut exclusivity, pointer provenance, etc.

@HeroicKatora
Copy link
Member

Additional argument for it: bytemuck has reached 1.0.

@aschampion
Copy link
Contributor Author

bytemuck looks great, I'll give it a go.

@aschampion
Copy link
Contributor Author

Switch to bytemuck done, removed the 1.35 bump commit and tested that it builds on 1.34.2.

Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

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

Looking great!

@HeroicKatora HeroicKatora merged commit d3b3133 into image-rs:next Dec 12, 2019
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.

None yet

4 participants