Skip to content

Jpegxl codec - #70

Open
fcollman wants to merge 6 commits into
zarr-developers:mainfrom
AllenInstitute:jpegxl
Open

Jpegxl codec#70
fcollman wants to merge 6 commits into
zarr-developers:mainfrom
AllenInstitute:jpegxl

Conversation

@fcollman

Copy link
Copy Markdown

Here is a proposal for a JPEGXl codec specification. I have tried to align it with the existing imagecodecs implementation and have a draft neuroglancer implementation. I also have a wrapper around imagecodecs for zarr-python, but there is also the built in imagecodecs zarr plugin (though I am suggesting setting the codec name to be just jpegxl which will require this PR.

@normanrz

Copy link
Copy Markdown
Member

Thanks @fcollman! I think it would be useful to coordinate this PR with #66.

I am not sure, I fully understand the need for having the configuration parameters as an open set. Could we not include all parameters from the JPEGXL spec (except the ones you explicitly disallow)?
In other codecs, we have made configuration parameters required even if they were only encoding hints. The rationale is to make the encoding deliberate and reproducible so that you end up with uniform settings across the chunks of an array.

Comment thread codecs/jpegxl/README.md Outdated
## Configuration parameters

The codec has no required configuration parameters; the `configuration` object
MAY be omitted or empty. Any members that are present are **encoder hints**: an

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be better to model the configuration as a record of the settings that were used for encoding. Although the configuration isn't needed for decoding, the configuration is needed for re-encoding data with the same settings as some other jpegxl data. Without content in the configuration field, implementations must choose default settings on their own, which will lead to inconsistently encoded data.

@fcollman

Copy link
Copy Markdown
Author

Thanks @fcollman! I think it would be useful to coordinate this PR with #66.

I am not sure, I fully understand the need for having the configuration parameters as an open set. Could we not include all parameters from the JPEGXL spec (except the ones you explicitly disallow)? In other codecs, we have made configuration parameters required even if they were only encoding hints. The rationale is to make the encoding deliberate and reproducible so that you end up with uniform settings across the chunks of an array.

sounds good, I think this is mostly a change to the readme and not to the implementation? We still want to prioritize the bytestream version of the parameters on decode yes? Or readers are suppose to check that the parameters match and fail to decode if they don't match?

@fcollman

Copy link
Copy Markdown
Author

one question i have about this is the bitspersample parameter.

If i have a float32 array, and i use 16 bitspersample to compress it, do i expect that decoders return to me a float32 value?

or, should we disallow any bitpersample that is not aligned to the datatype that is being stored?

@fcollman

Copy link
Copy Markdown
Author

I've rewritten this to make the configuration properties required, and made a proposal for what to do with bitspersample

@fcollman

Copy link
Copy Markdown
Author

@d-v-b @normanrz @jbms I think i've incorporated the feedback, can i get a fresh take on what you all think?

Comment thread codecs/jpegxl/README.md

## Configuration parameters

The `configuration` object is REQUIRED and records the encoding parameters used

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Arguably they could be optional but writing fails if they are omitted. But we haven't done that for any other codecs.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

just clarifying, so you think that we should not make it required, and writers should raise errors if asked to write without this metadata being specified? I think the core question is whether or not we are providing/suggesting default values for parameters or not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I would say we should not use default values except for future evolution, or where not specifying a given option just means the corresponding feature isn't used/relevant.

Comment thread codecs/jpegxl/README.md
container. (see [Encoded representation](#encoded-representation)).

Some parameters supported by JPEG XL bindings are deliberately **excluded**: a
`planar` (channel-separated) layout, because this codec requires the sample axis

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this even needs to be mentioned, it isn't related to the stored format.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 to omitting information about omitted parameters from this section. if it's useful to disclose this information, a separate "relationship to JPEG XL bindings" section might be a better place to give implementation guidance.

Comment thread codecs/jpegxl/README.md
The encoded chunk is a JPEG XL image in either of the two forms permitted by
the JPEG XL standard: a bare codestream (beginning with `0xFF 0x0A`) or the
ISOBMFF box container (beginning with the JXL container signature `0x00 0x00 0x00 0x0C 0x4A 0x58 0x4C 0x20 0x0D 0x0A 0x87 0x0A`). Decoders
MUST accept both forms. Encoders SHOULD write a bare codestream; the container

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clarify that by encoder you really mean the tool that generates the codec configuration. The encoder must follow what is specified in the configuration.

Comment thread codecs/jpegxl/README.md
Decoders MUST return an error if the chunk shape is not one of these forms, or
if `W`, `H`, `S`, `F` derived from the codestream are not consistent with it.

**Disambiguating the two 3-D forms.** A three-dimensional chunk is either

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this heuristic should be dropped, it is confusing and error prone. Instead the configuration should somehow make explicit the number of channels or the S dimension should always be included if the F dimension is.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

so just to elaborate on these two options to make sure i understand.

if there is a zarr array that is a 3d array with no units on any of its axis, lets call the dimensions ABC, and the users says that it should be jpegxl compressed, then the configuration making it explicit would require the codec to say that C is a S dimension or if A is an F dimension? And if C is an S dimension than which of those dimensions are "extra" dimensions versus those that should go through the RGB joint encoding procedure?

if the S dimension has to be included if the F dimension is, I guess would mean that only [H, W] and [F, H, W, S] are allowed chunk dimensions, and if you have a 3d array you will need to add a reshape codec to create a singleton channel dimension to be your S or F dimension. You'd made an S dimensions if your 3d array were a spatial array, and you'd make an F dimension if it were a RGB or RGBA color image.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The general idea I had was that we have clear, and relatively inflexible rules for mapping array dimensions to image dimensions inside each image codec, and rely on transpose and reshape as needed.

For jpeg there is always just width and height and channels, so we can support either 2 or 3 dimensional arrays unambiguously. For jpegxl since we have both the frame and the channel dimension we have more potential ambiguities. We want to avoid the case where the user has a chunk size of [A, B, C] or [A, B, C, D] and it works in general but something breaks when one of the sizes happens to be 1.

The number of color channels can be determined from the color_space (renamed from photometric, I'd suggest), or could be an explicit num_color_channels parameter. Then there could be an extra_channels array specifying the parameters for each extra channel. Or it may indeed be reasonable to not support encoding extra channels yet, and wait for someone who needs that to add it to the spec.

The convention can be that color channels come first and then extra channels.

Comment thread codecs/jpegxl/README.md
values spend more time to reduce the encoded size.
- **`decodingspeed`** (integer, `0`–`4`): tier trading encoded size for decode
speed (`0` = default, smallest).
- **`photometric`** (string, one of `gray`, `rgb`, `xyb`, `unknown`): the color

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think YCbCr is also supported. Also arguably there should be a separate parameter for the decoded color space rather than always assuming RGB, as I also suggested for jpeg.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see that while it is supported by the format, in order to losslessly encode existing jpegs, this option is not even exposed by the reference libjxl implementation. Therefore I suppose it should be excluded.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

sorry which option? YcBCr? or the entire parameter?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, yeah I meant YCbCr.

Comment thread codecs/jpegxl/README.md
error for a channel count they do not support rather than silently mismatching
the chunk shape.

`S` is **not** a way to stack unrelated channels. For data with many

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think this again relates to the above question about whether zarr should default to having channels interact when encoding them or have them be independent. We can force the user to be explicit about it, it just means that more parameters are required to be specified.

Comment thread codecs/jpegxl/README.md
([`imagecodecs`](https://github.com/cgohlke/imagecodecs)), the two are told
apart by the trailing axis size: **≤ 4** is the sample axis `S` (`[H, W, S]`);
**≥ 5** is the width, so the leading axis is the frame axis `F` (`[F, H, W]`).
The interleaved form is therefore limited to `S ≤ 4` (see

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this restriction should be dropped.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this relates to the above issue regarding, but I think we drop this restriction than we have to require users to specify which of dimension is F, H, W or S, and we need to know explicitly which of the S dimensions are 'extra'. S=3+ can be RGB or grayscale and N extra dimensions (i.e S=6 could be RGB + 3 extra, or grayscale + 5 extra)

Comment thread codecs/jpegxl/README.md

## Sample layout and color

Samples are stored in C order, so for a chunk shape ending in `S` the channels

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't mention c order

Comment thread codecs/jpegxl/README.md

Samples are stored in C order, so for a chunk shape ending in `S` the channels
are interleaved (the innermost, unit-stride axis). If a different in-memory
channel order is required, use the `transpose` codec.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Transpose changes the logical order, the actual in-memory layout is up to the implementation.

Comment thread codecs/jpegxl/README.md

For data with many independent measurement channels (e.g. fluorescence or
multispectral microscopy), do not interleave them as `S`: the interleaved-sample
form is limited to `S ≤ 4`, and `S = 3` applies an RGB color transform that is

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The color transform is a parameter, not automatic.

Comment thread codecs/jpegxl/README.md
mathematically lossless; larger values allow more loss for smaller output.
- **`effort`** (integer, `1`–`10`): the encoder effort/speed setting. Higher
values spend more time to reduce the encoded size.
- **`decodingspeed`** (integer, `0`–`4`): tier trading encoded size for decode

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

worth considering normalizing this and similar names (bitspersample, usecontainer) to snake case, i.e. decoding_speed, bits_per_sample, use_container. Better literacy + style consistency with extant codec JSON metadata documents.

Comment thread codecs/jpegxl/README.md
to the `F` axis and orientation handling is fixed by this specification (see
[Orientation](#orientation)).

### Decoding does not use the configuration

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this something that needs to be explained here? I believe it's fairly common for decoding to rely entirely on parameters from the encoded stream, so perhaps this can be omitted.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

if we are writing the parameters in two places (the bytestream and in the metadata) shouldn't we be explicit about which takes precedence?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i assume that writing the parameters to the bytestream is dictated entirely by the jpegxl encoding specification. so that's not actually a degree of freedom for this spec, as long as the goal is to use regular jpegxl encoding / decoding machinery under a patina of zarr. a jpegxl decoder will not be looking inside zarr.json anyway, so I don't think there's room for confusion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

put differently, "Decoding does not use the configuration" is implicit whenever we adapt an existing codec for zarr, if the signature of the decoding side was the common decode(bytestream) -> bytes, where no configuration appears.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

okay happy to remove it

Comment thread codecs/jpegxl/README.md
> **Note:** JPEG XL can be lossy. Repeated decode/encode cycles compound
> artifacts, and lossy compression is unsuitable for label/segmentation data.

## Orientation

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

based on the comments about the JPEGXL configuration parameters that are omitted, i'm guessing that we should also remove this section as well?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No this is relevant --- it is important to say that the recorded orientation is ignored.

Comment thread codecs/jpegxl/README.md
"codecs": [
{
"name": "jpegxl",
"configuration": { ... }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Definitely don't elide the configuration, that is one of the most important parts.

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.

4 participants