16 bit per channel image buffers and dynamic images #1085

aschampion · 2019-11-27T21:03:52Z

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 · 2019-11-27T21:43:38Z

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.

HeroicKatora

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

src/color.rs

src/image.rs

src/buffer.rs

src/dynimage.rs

HeroicKatora · 2019-11-27T21:58:16Z

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 · 2019-11-27T22:34:30Z

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

aschampion · 2019-12-04T18:38:56Z

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.

HeroicKatora · 2019-12-04T19:04:58Z

.travis.yml

@@ -10,7 +10,7 @@ notifications:
 os:
  - linux
 rust:
-  - 1.34.2
+  - 1.35.0


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

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.

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.

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.

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.

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.

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

src/buffer.rs

aschampion · 2019-12-10T11:57:51Z

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.

fintelia

I think things are in good shape at this point

src/buffer.rs

src/color.rs

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 · 2019-12-10T18:03:45Z

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

Shnatsel · 2019-12-12T13:09:30Z

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 · 2019-12-12T13:36:12Z

Additional argument for it: bytemuck has reached 1.0.

aschampion · 2019-12-12T13:53:52Z

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

Bytemuck is more feature constrained and builds on Rust 1.34.2.

aschampion · 2019-12-12T14:27:54Z

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

HeroicKatora

Looking great!

HeroicKatora reviewed Nov 27, 2019

View reviewed changes

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

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

aschampion force-pushed the features/16bpc-dynimage-squash branch from a571984 to d8f6163 Compare December 4, 2019 18:55

HeroicKatora reviewed Dec 4, 2019

View reviewed changes

fintelia reviewed Dec 8, 2019

View reviewed changes

src/color.rs Outdated Show resolved Hide resolved

fintelia reviewed Dec 8, 2019

View reviewed changes

src/color.rs Outdated Show resolved Hide resolved

src/color.rs Outdated Show resolved Hide resolved

src/buffer.rs Outdated Show resolved Hide resolved

polarathene mentioned this pull request Dec 8, 2019

General support for 16bit color components #665

Closed

aschampion force-pushed the features/16bpc-dynimage-squash branch from 0d7cd5d to 98cd35a Compare December 8, 2019 13:34

fintelia approved these changes Dec 10, 2019

View reviewed changes

src/buffer.rs Outdated Show resolved Hide resolved

src/color.rs Outdated Show resolved Hide resolved

HeroicKatora reviewed Dec 10, 2019

View reviewed changes

src/color.rs Outdated Show resolved Hide resolved

aschampion added 8 commits December 10, 2019 17:57

Create common trait for all image format encoders

4d776b7

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.

Bump png version

a32fd74

This fixes 16bpc tests.

Make ChannelsType nonexhaustive

fe06c05

Remove ColorType::bytes_per_channel

7b62a7b

Per review comments, this assumes equal channel bit depths, which will not be the case in the future.

Note trait should be removed (minor)

9de3e61

Restore as_flat_samples methods for DynamicImage

a502bee

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.

Remove zerocopy from public interface

ed47a76

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 force-pushed the features/16bpc-dynimage-squash branch from b30f4af to 73ddf01 Compare December 10, 2019 17:59

aschampion added 2 commits December 12, 2019 14:25

Fix compilation on 1.34.2

c642686

Hide 16bpc buffer aliases from public API

e12da31

aschampion added 3 commits December 12, 2019 14:25

Remove ChannelsType

35ebf63

Restore Pixel::COLOR_TYPE associated type

33cebef

Switch from zerocopy to bytemuck

5dae1fd

Bytemuck is more feature constrained and builds on Rust 1.34.2.

aschampion force-pushed the features/16bpc-dynimage-squash branch from 73ddf01 to 5dae1fd Compare December 12, 2019 14:26

HeroicKatora approved these changes Dec 12, 2019

View reviewed changes

HeroicKatora merged commit d3b3133 into image-rs:next Dec 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

16 bit per channel image buffers and dynamic images #1085

16 bit per channel image buffers and dynamic images #1085

aschampion commented Nov 27, 2019

fintelia commented Nov 27, 2019

HeroicKatora left a comment •

edited

Loading

HeroicKatora commented Nov 27, 2019 •

edited

Loading

aschampion commented Nov 27, 2019

aschampion commented Dec 4, 2019

HeroicKatora Dec 4, 2019

aschampion Dec 4, 2019

HeroicKatora Dec 4, 2019 •

edited

Loading

HeroicKatora Dec 4, 2019

HeroicKatora Dec 4, 2019

aschampion Dec 4, 2019

HeroicKatora Dec 10, 2019 •

edited

Loading

aschampion commented Dec 10, 2019

fintelia left a comment

aschampion commented Dec 10, 2019

Shnatsel commented Dec 12, 2019

HeroicKatora commented Dec 12, 2019

aschampion commented Dec 12, 2019

aschampion commented Dec 12, 2019

HeroicKatora left a comment

@@ @@ -10,7 +10,7 @@ notifications: @@
               os:
                 - linux
               rust:
-                - 1.34.2
+                - 1.35.0

16 bit per channel image buffers and dynamic images #1085

16 bit per channel image buffers and dynamic images #1085

Conversation

aschampion commented Nov 27, 2019

fintelia commented Nov 27, 2019

HeroicKatora left a comment • edited Loading

Choose a reason for hiding this comment

HeroicKatora commented Nov 27, 2019 • edited Loading

aschampion commented Nov 27, 2019

aschampion commented Dec 4, 2019

HeroicKatora Dec 4, 2019

Choose a reason for hiding this comment

aschampion Dec 4, 2019

Choose a reason for hiding this comment

HeroicKatora Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

HeroicKatora Dec 4, 2019

Choose a reason for hiding this comment

HeroicKatora Dec 4, 2019

Choose a reason for hiding this comment

aschampion Dec 4, 2019

Choose a reason for hiding this comment

HeroicKatora Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

aschampion commented Dec 10, 2019

fintelia left a comment

Choose a reason for hiding this comment

aschampion commented Dec 10, 2019

Shnatsel commented Dec 12, 2019

HeroicKatora commented Dec 12, 2019

aschampion commented Dec 12, 2019

aschampion commented Dec 12, 2019

HeroicKatora left a comment

Choose a reason for hiding this comment

HeroicKatora left a comment •

edited

Loading

HeroicKatora commented Nov 27, 2019 •

edited

Loading

HeroicKatora Dec 4, 2019 •

edited

Loading

HeroicKatora Dec 10, 2019 •

edited

Loading