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

Buffer based on canvas #1718

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

HeroicKatora
Copy link
Member

Draft for comments on the approach.

Add a byte-based buffer which is color space aware and tracks the underlying color and texel layout with fields, instead of encoding them into the type. It offers the same raw bytes access as DynamicImage but avoids the clunky matching required to access particulars. This drastically improves ergonomics where you'd otherwise need N×M type combinations to fully express all color spaces with all pixel layouts. This clunkiness is at least one of the causes of having support for only very few of these combinations in ColorType and even ExtendedColorType.

The new approach successfullly and succinctly implements n-to-n-conversion of any input combination to any output combination. It can dynamically execute optimized code (e.g skip color conversion) and architecture specific code (e.g. SIMD shuffles) with little effort because those are very local decisions and don't propagate through interfaces by type-level differences.

We don't expose all combinations in image (yet) because they aren't equally well implemented, tested and performance is not yet optimized for many of them.

@HeroicKatora
Copy link
Member Author

The main future use case for the buffer is as a separate decoding and encoding target, such that CMYK and YUV inputs can be decoded with a delayed conversion. This provides three main advantages:

  1. We can represent many new input layouts directly without having to convert them into an rgb-ish representation enforced by DynamicImage and its supported variants.
  2. The conversion may need to be aware of additional parameters such as ICC-information that is supplied later in decoding (e.g. we may convert directly into a display output color space without ever storing pixels in profile connection space; which is faster and more accurate)
  3. Conversion can happen on GPU hardware instead of being forced to execute on a slow CPU. This, again, is more efficient.

@fintelia
Copy link
Contributor

I think it would be really cool if we could replace the current enum based implementation of DynamicImage with one that wrapped an image_canvas::Canvas but was guaranteed to always be in a ColorType (either by carrying it as a separate field or deducing like is done in this draft PR).

If we want to support delayed color conversion from a wide range of pixel layouts/color spaces, that sounds like a really great expose functionality that operates on image_canvas::Canvas's directly.

@HeroicKatora
Copy link
Member Author

I chose not to replace DynamicImage right away because of compatibility, i.e. we obviously can't change the public enum to an opaque struct. Having a compatibility type sounds appealing though—a canvas guaranteed to perform constant-cost conversion to a DynamicImage.

If we want to support delayed color conversion from a wide range of pixel layouts/color spaces, that sounds like a really great expose functionality that operates on image_canvas::Canvas's directly.

Come again, there's a portion missing here?

@fintelia
Copy link
Contributor

I chose not to replace DynamicImage right away because of compatibility, i.e. we obviously can't change the public enum to an opaque struct. Having a compatibility type sounds appealing though—a canvas guaranteed to perform constant-cost conversion to a DynamicImage.

This feels like a big enough feature addition to justify a major version bump, so I'm not too worried about the API breakage aspect. Obviously, we shouldn't go out of our way to break things, but I think the majority of the existing API can be implemented with this alternative approach.

Plus, there is significant benefit to having a single DynamicImage type that is the right choice for almost all uses. Especially if it can have zero cost conversion to/from image_canvas::Canvas for its current color type, and optimized color conversions for others.

If we want to support delayed color conversion from a wide range of pixel layouts/color spaces, that sounds like a really great [edit: reason to] expose functionality that operates on image_canvas::Canvas's directly.

Come again, there's a portion missing here?

Sorry this was in reference to "The main future use case for the buffer is as a separate decoding and encoding target, such that CMYK and YUV inputs...".

Still faulty because its color type is currently sRGB, which decodes to
a single channel that is reinterpreted as (r,0,0,a) and then converted
to ~21% of the original value by multiplying with 0.2126...
@HeroicKatora HeroicKatora marked this pull request as ready for review November 2, 2022 20:20
Comment on lines +20 to +22
pub struct Canvas {
inner: Inner,
}
Copy link
Member Author

@HeroicKatora HeroicKatora Nov 5, 2022

Choose a reason for hiding this comment

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

@fintelia Alright with the name? Other ideas that have been floating around in my head:

  • Picture for a less dumb buffer that also contains the metadata.
  • How do we treat the current ImageBuffer, I really want to preserve the 'simple' pixel array container because it can be much simpler to work with and requires less unwrapping. For instance, we couldn't offer rows on Canvas. But the naming is odd now. Maybe PixelBuffer would be better?

@fintelia
Copy link
Contributor

fintelia commented Nov 5, 2022

The main question for me is when should a user prefer Canvas over the other existing image representations we support? It is probably worth writing a paragraph explaining that as part of the type's rustdoc documentation, and then including a short bit in the top-level crate documentation as well. One possible answer is that Canvas is better for writing image processing operations that are generic over pixel types.

A follow question would be whether there's any cases currently filled by DynamicImage that Canvas isn't suited for?

This acknowledges, in a forward compatible manner, that additional
layouts may be added later that can not be converted to `sRGB` colors
and for which these conversions must therefore error.
Comment on lines +3 to +13
//! The best container depends on the use:
//! * The generic [`ImageBuffer`] struct, a simplified pixel container with a standard matrix
//! layout of uniform channels. The strength is convenient access for modifications with a direct
//! mapping and access to the underlying data.
//! * The [`DynamicImage`] enumeration builds on `ImageBuffer` to provide a standard selection of
//! basic channel representations, and conventionally in sRGB color space. This is usually enough
//! to represent any source data without loosing much precision. This makes is suitable for
//! basic, but generic, image operations.
//! * The [`Canvas`] struct as general texel container of general layouts as a buffer. The strength
//! is the generality (and extensibility) that allows its use for interchange of image data and
//! reduced processing cost if the convenience above is not required.
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this somewhat clear motivation?

Copy link
Contributor

@fintelia fintelia Nov 16, 2022

Choose a reason for hiding this comment

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

Sorry for the delay responding. That explains what sorts of images the different types can store, but doesn't really think through what the user can do with them. My rough thinking:

  • ImageBuffer provides both a collection of image processing methods (resize, crop, etc.) as well as ergonomic pixel getters and setters to implement your own image processing. The serious drawback is that the color type is determined at compile time. One consequence is that saving works fine but loading files as ImageBuffers isn't directly supported.
  • DynamicImage supports even more built in image processing as well as loading/saving a whole range of file formats. Conversion between color types is supported. The enum is non-exhaustive but users can implement their own algorithms by matching on the current variants with special case logic for each one.
  • Canvas holds and converts between a broader range of color types. No other image processing algorithms are provided and users who want to write their own must... rely on raw access to aligned byte slices? Cast them to FlatSamples first? That's perhaps a bit unfair of a characterization since the type could still be useful as a common "interchange" representation for image data in Rust. However, I suspect it would have a better chance of catching on if users didn't have to immediately convert the Canvas into something else to operate on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

No other image processing algorithms are provided and users who want to write their own must... rely on raw access to aligned byte slices?

Precisely. The best reason to use the buffer is uploading it to a GPU texture as directly as possible; for which only the layout and raw bytes are necessary and sufficient, and all further CPU processing is less precise and more costly than necessary. (And I stopped expanding the interface, before anyone could get the idea of making this PR contingent on a corresponding wgpu GPU framework).

The lack of direct operations is by design. Precisely because we may not have interpreted the correct color information (color space, or ICC, or otherwise) yet, just the need to store some numeric texel matrix anyways. There's just nothing sensible to do with the matrix entries, at that point. It should get support for Yuv layouts and CMYK rather soon to fix the tragedy of wrongly applying incorrect ICC tables while loading jpeg, WebP, png and tiff, as we are doing currently... But that's for another PR in my opinion.

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