JPEG XL

JPEG XL spec bugs, issues and questions.

I'd like to post any recently discovered spec bug to here, since it is not really about J40

negative special distance during LZ77: ```c int32_t special = (int32_t) SPECIAL_DISTANCES[distance]; J40__ASSERT(dist_mult <= J40__MAX_DIST_MULT); // TODO spec bug: distance can be as low as -6 when dist_mult = 1 and distance = // dist_mult * 1 - 7; libjxl clamps it to the minimum of 1, so we do the same here distance = j40__max32(1, ((special >> 4) - 7) + dist_mult * (special & 7)); ```

a related edge case where distance is zero at the very first symbol: ```c distance = j40__min32(j40__min32(distance, code->num_decoded), 1 << 20); code->copy_pos = code->num_decoded - distance; if (J40_UNLIKELY(distance == 0)) { // TODO spec bug: this is possible when num_decoded == 0 (or a non-positive special // distance, handled above) and libjxl acts as if `window[i]` is initially filled with 0 J40__ASSERT(code->num_decoded == 0 && !code->window); code->window = j40__calloc(1 << 20u, sizeof(int32_t)); if (!code->window) return J40__ERR("!mem"), 0; } ```

I pondered about these two

`width * height` here refers to "the largest dimensions (horizontally and vertically, respectively) of the image and any of its frames, in pixels", but splines and patches are per-frame, so this means that checking for those limits can only be done when every frame is read (!)

for example in 100x100 image the first frame can have 100x100 dimensions and 2501 splines. this can be valid if the second frame has, say, 200x200 dimensions which later get cropped.

similarly, ICC `output_size` is distinct from `enc_size`, and it is really inconvenient (if not impossible) to estimate the minimum possible `output_size` from `enc_size` AFAIK

libjxl at least assumes that each command emits at least one byte, I'm yet to validate or invalidate this assumption

I've analyzed the specification and yes, this assumption is invalid

specifically main command byte 1 through 4 reads the length of output bytes `num` from the command stream which is allowed to be zero, so it is possible to artifically inflate `enc_size` without altering `output_size`

fortunately libjxl doesn't seem to emit any zero-sized command, so I think we can simply forbid zero-sized command, and note that every command generates at least one output byte, which should be enough hint for implementers

this possibility of ICC codec DoS has been reported to the issue tracker maintained by <@794205442175402004>.

For the splines and patches limits, we probably need to make it refer to the frame they're in

Libjxl also has a limit on the total pixel area of the splines

we may want to add such a limit to the profile too

yeah, that makes an intuitive sense, I can see some cases where a smaller frame wants to complement prior frames and thus can have more splines than the individual frame needs, but taking account for image dimensions seems enough for that.

1. when calculating `num_groups`, one must divide `ceil(width / group_dim)`, using `width` in the frame header. However, if `lf_level != 0`, then one must set `width = ceil(width / (1 << (3 * lf_level))` *before* making this `num_groups` calculation. (This is the same for height.) This is in the spec, but since it's mentioned after `num_groups` is defined, it's very confusing.

2. The same is also true if `upsampling != 1`. If `upsampling > 1` then you must divide `width = ceil(width / upsampling)` before calculating `num_groups`. Do note that upsampling is present provided that `!(flags & kUseLfFrame)`, but the spec doesn't actually say that this flag *must* be enabled if `frame_type == kLFFrame`. If so, it should say so. Since you call `ceil` first, this is not commutative either.

3. The condition for `save_before_ct` is incorrect. The spec says that it's signalled provided that `!all_default && frame_type != kLFFrame`, with a default value `d_sbct` defined later in the spec. The actual condition for `save_before_ct` being signaled is ``` !all_default && (frame_type == kFrameReferenceOnly || full_frame && (frame_type == kFrameRegular || frame_type == kFrameSkipProgressive) && (frame_duration == 0 || save_as_ref != 0) && !is_last && blend_mode == kReplace) ``` based on libjxl code.

4. The spec should be clear that if `num_toc_entires > 1`, then all the TOC entries listed there are present but the size may be zero for some of them, e.g. in the case of `encoding == kModular`. This is technically stated but it's a bit confusing IMO.

5. The spec says this (F.3.3) > Before decoding the TOC, the decoder invokes `ZeroPadToByte()` (B.2.7). It's not entirely clear what "before decoding the TOC means" here. The actual behavior is that `permuted_toc = Bool()` is read *immediately* after the frame header, and then if it is nonzero, the TOC permutation is read *immediately* after `permuted_toc`, *then* `ZeroPadToByte()` is invoked, then the TOC lengths are each read, and *then* `ZeroPadToByte()` is invoked again after they have all been read. This should be more explicit.

6. `[0, num_clusters - 1)` appears twice in section C.2.1. It should be `[0, num_clusters)`

7. Alias Mapping section C2.2, provides this line of code: ```cpp if (cutoffs[i] > bucket_size) overfull.push_back(i); else underfull.push_back(i); ``` this is incorrect. The correct line here is: ```cpp if (cutoffs[i] > bucket_size) overfull.push_back(i); else if (cutoffs[i] < bucket_size) underfull.push_back(i); ```

8. Alias Mapping section C.2.2 is missing an extra loop: ```cpp for (i = alphabet_size; i < table_size; i++) { underfull.push_back(i); } while(overfull.size() > 0) { // right before this line ``` This same loop should also zero out `cutoffs[i]` if we're not assuming it's initially zero.

9. Alias Mapping is missing a comment that says underfull never runs out: ```cpp while (overfull.size() > 0) { /* underfull.size() > 0 */ o = overfull.back(); u = underfull.back(); overfull.pop_back(); underfull.pop_back(); ``` While this is impossible if the code is bug-free, I ran into this issue when I had bugs earlier in the spec, so it's not a bad idea to include it as a sanity thing.

10. Section C.2.2. defines AliasMapping for a specific distribution as `AliasMapping(x)`, a function of one variable, and yet later on in Section C.2.3, you call this: ```cpp (symbol, offset) = AliasMapping(D, index); state = D[symbol] × (state >> 12) + offset; ``` It's probably a bit clearer if you use `D.AliasMapping(index)`, i.e.: ```cpp (symbol, offset) = D.AliasMapping(index); state = D[symbol] * (state >> 12) + offset; ```

11. In section C.2.6, you define `lsb = token & ((1 << config.lsb_in_token) - 1);`. I'd probably rename this variable to `low` since I think `lsb` gives the wrong implication of what this is supposed to do.

12. Section 2.4 doesn't define `alphabet_size` for simple distributions, only for flat and for complex distributions. Since `alphabet_size` can be less than `table_size` this is important.

These are all things I found before, but I'm reposting them from <#794206170445119489> here for clarity.

Just want to double check, Extension lengths are all read before any extension payloads are read, in the extensions bundle, right?

to my knowledge yes.

I'm reading through the new spec, and here's the issues I've found so far

(1) `alphabet_size` is not defined for simple distributions in section C.2.5. It's read for flat and for complex distributions. If it's supposed to be equal to `table_size` in the case of a simple distribution, then it should say so. (2) There's still a bug in the pseudocode in section C.2.6: ``` if (cutoffs[i] > bucket_size) overfull.push_back(i); else underfull.push_back(i); ``` This should read: ``` if (cutoffs[i] > bucket_size) overfull.push_back(i); else if (cutoffs[i] < bucket_size) underfull.push_back(i); ``` `underfull` shouldn't be filled if `cutoffs[i]` exactly hits the bucket size. (3) Section C.2.1 includes two cases of `[0, num_clusters - 1)` but it should say either `[0, num_clusters)` or `[0, num_clusters - 1]`.

(4) Section D.2, SizeHeader description, contains a typo. It says `!div` where it should say `!div8`

can someone double-check these proposed fixes so we don't accidentally introduce new bugs?

Sure, I encourage a double-check

although some of these are just typos

Also, there's still some c-isms in the code such as `!ratio` instead of `ratio == 0`

since you changed `&&` to `and` I'm not sure this is desired

worth considering it

Section E.2 has a formatting issue:

look at the ovlerap on the last row

I suggest moving the `type` column leftward as there is enough empty space

Section E.4.3 his missing some code tags. there's a `min(128, output_size)`, a `(p + e) & 255`, and an `i == j` that are in in textrm.

c-isms are fine, the only reason I changed `&&` to `and` is that it caused trouble when using pandoc to convert our tex sources to docx (which is the format ISO insists on, frustratingly), since pandoc misinterpreted it as two column separators

ah

formatting issues are caused for the same reason, I had to drop the manual column width adjustments because they caused tables not to get parsed correctly anymore... i'll have to figure out a way to make it work in both latex and pandoc

I see. These aren't really major issues, I'm just pointing out what I see when I see it

yeah don't stop doing that, it is useful 🙂

The spec doesn't appear to specify that `flats & kUseLfFrame` *must* be set if `frame_type == kLFFrame`

Is this a requirement?

no it isn't: that flag means the _current_ frame gets its DC from some previous LfFrame

I see

so typically if there is no recursive DC but just progressive DC encoded using modular+squeeze, you'll have first a modular frame with frame_type == kLfFrame (which doesn't even use vardct so kUseLfFrame is meaningless), and then a vardct frame with frame_type == kRegularFrame and flags & kUseLfFrame set (which means there is no DC data in the frame itself, but the previous kLfFrame gets used as DC instead)

it's somewhat confusing and probably a NOTE explaining this would be good

If `upsampling > 1` then you must divide `width = ceil(width / upsampling)` before calculating `width = ceil(width / (1 << 3 * lf_level))`, which in turn must be done before calculating `num_groups`. Both these can be present according to the spec. These operations do not commute because of the intervening `ceil` but the spec doesn't say that you have to do it in that order. (I figured it out from inspecting libjxl code).

Upsampling is signaled provided that `!(flags & kUseLfFrame)` which will probably be the case for `frame_type == kLfFrame`

`lf_level` is signaled as `U32(1, 2, 3, 4)` which seems like a roundabout way of saying `1 + u(2)`

Same with `BlendingInfo.source` which is signaled as `U32(0, 1, 2, 3)` in the spec. why not just `u(2)`?

Section F.3.3 specifies: > Before decoding the TOC, the decoder invokes ZeroPadToByte() (B.2.7). I'd recommend, for clarity, writing "Before decoding the TOC (but after decoding the TOC permutation, if present), the decoder invokes `ZeroPadToByte()`."

Section G.2.3 has a formatting issue with `ceil(height/8)` being in textrm instead of in code

same with the width

This also occurs in section G.2.4 several times.

`log2` should be code in section G.3.3, along with two `< 3`s for hshift and vshift.

at the end of the second-to-last paragraph of section H.1, there's a `-1` that's done in textmode, not mathmode, so the `-` looks like a hyphen, not a negative one.

I haven't attempted to try to skim past that but I'll point anything out that I see when I continue to write jxlatte

why does `alphabet_size` need to be defined in that case? that variable is not referenced then, or is it?

it's used when you generate the alias map

oh, right. I see, because you define `D[i]` outside of alphabetSize to be zero.

Okay, I see, that makes sense now.

Section H.2 mentions this right at the end:

> The decoder then starts an ANS stream (C.1)

does it actually *have* to be ANS? or can it be a prefix-code distribution?

Section H.6.3 Squeeze transform appears to have an etraneous `-1` in the psuedo-code:

> `for (i = 0; i < sp.size() - 1; i++) {`

I believe this should say `i < sp.size();`

otherwise the final element of `sp` doesn't do anything

Right.

It can be either. Maybe we need a word for that...

Entropy-coded stream?

Section C.1 is linked anyway

<@795684063032901642>

Section H.5.2, top of page 52: `4 + (maxweight × ((1 << 24) Idiv ((err_sum >> shift) + 1))) >> shift;` this is missing parentheses, should be: `4 + ((maxweight × ((1 << 24) Idiv ((err_sum >> shift) + 1))) >> shift);`

`+` is grabbier than `>>` in C.

aka it has higher precedence

Same section, same page: `subpred[3] = N3 - (((true_err_NW × wp_p3a + true_err_N × wp_p3b + true_err_NE × wp_p3c + (NN3 - N3) × wp_p3d + (NW3 - W3) × wp_p3e) >> 5);`

this has an extra opening `(` right after `N3 -`

we will probably switch from LaTeX to this: https://www.metanorma.org/

which might introduce some new spec bugs but it will give us a better way to produce Word files the way ISO wants them while still having a text-based spec with version control so we don't have to deal with Word to do the editing

looks good & designed exactly for this purpose

section H.5.1 in the definition of `err[i]` the parentheses are misplcaced

It *says*

`abs((subpred[i] - (true_value << 3)) + 3) >> 3`

it should be

`(abs(subpred[i] - (true_value << 3)) + 3) >> 3`

that is, you are supposed to add 3 after taking the absolute value of the difference

Section J.1, it states the condition for `gab_custom` to be signaled is `!all_default && gab`

this is actually not true, it's just `gab`

so `gab_custom` will be signaled when `all_default == true`

🤦‍♂️

if anyone feels like comparing this metanorma version of the spec (in html) to the latex version (in pdf) to see if there's anything that got lost in translation, feel free!

I dislike Cambria as a font choice but otherwise I don't notice anything immediate

Table H.2 could use better rendering

as it's not really a table

Haha yes Cambria is a stupid font but what makes you think we have any choice in that? 😜

ISO forces us to use Cambria for the main text and Courier New for the source code snippets, even though both of those are ugly and there are nicer fonts

can doc tooling do different versions with different fonts?

jxl spec the community edition (good fonts) vs iso edition (cambria)

haha sure

I migrated part 2 to asciidoc too (until now we just used Word for that since the pain was doable, it's a smaller spec)

and then I made the above part 1+2 community edition 🙂

damn we missed an opportunity to save 1 header bit!

It won't end up mattering without a permuted TOC as it gets eaten by ZeroPadToByte

current versions of part 1 and part 2 drafts for 2nd edition

Section K.3.1:

> `blending`: arrays of `count` blend mode information structures, which consists of arrays of mode, alpha_channel and clamp

Later, when blending is populated, the loop looks like this:

> `for (k = 0; k < num_extra_channels+1; k++) {`

These numbers aren't necessarily the same.

How big is `blending[]`? is it one per `count` or is it one per `num_extra_channels + 1`

The spec doesn't say what order the passgroups are in

do you let `pass = 0` and then do *all* the groups?

and then let `pass = 1` and do all the groups?

etc.

ah it does say, nvm.

The Tendency function in the section on squeeze is incorrect

It says this: ```c tendency(A, B, C) { X = (4 * A − 3 * C − B + 6) Idiv 12; if (A >= B and B >= C) { if (X − (X & 1) > 2 * (A − B)) X = 2 * (A − B) + 1; if (X + (X & 1) > 2 * (B − C)) X = 2 * (B − C); return X; } else if (A <= B and B <= C) { if (X + (X & 1) < 2 * (A − B)) X = 2 * (A − B) − 1; if (X − (X & 1) < 2 * (B − C)) X = 2 * (B − C); return X; } else return 0; } ```

This is in libjxl/squeeze.h:

```c++ inline pixel_type_w SmoothTendency(pixel_type_w B, pixel_type_w a, pixel_type_w n) { pixel_type_w diff = 0; if (B >= a && a >= n) { diff = (4 * B - 3 * n - a + 6) / 12; // 2C = a<<1 + diff - diff&1 <= 2B so diff - diff&1 <= 2B - 2a // 2D = a<<1 - diff - diff&1 >= 2n so diff + diff&1 <= 2a - 2n if (diff - (diff & 1) > 2 * (B - a)) diff = 2 * (B - a) + 1; if (diff + (diff & 1) > 2 * (a - n)) diff = 2 * (a - n); } else if (B <= a && a <= n) { diff = (4 * B - 3 * n - a - 6) / 12; // 2C = a<<1 + diff + diff&1 >= 2B so diff + diff&1 >= 2B - 2a // 2D = a<<1 - diff + diff&1 <= 2n so diff - diff&1 >= 2a - 2n if (diff + (diff & 1) < 2 * (B - a)) diff = 2 * (B - a) - 1; if (diff - (diff & 1) < 2 * (a - n)) diff = 2 * (a - n); } return diff; } ```

Notice that `diff` is calculated differently based on the branches

🤦‍♂️

good catch, made a pr to fix it in the spec (I think the code is correct...)

does it actually make a difference? I wonder why the spec didn't just copy the code

yeah it changes rounding

it makes it so diff is at most off-by-one here

+6 vs -6 is always is always different by exactly 12, so when dividing by 12 it's different by exactly 1

which affects the parity, i.e. `(diff & 1)`

diff ends up 1 smaller than it was

so if diff was even, then `diff + (diff & 1)` will end up being the same

but if diff was odd, then `diff + (diff & 1)` is two lower than it was

which potentially affects the thresholds

it also slightly affects the output of the squeeze transform because `buffer[x][2 * y + 1] = first - diff`

whose parity matters

it's probably not exceptionally noticeable but it does affect it slightly

whether or not it *was* correct, it is now, I suppose <:kek:857018203640561677>

because libjxl in some ways defines the bitstream

the C++ version makes sense in that it's [positive]+6 or [negative]-6, i.e. adds "away from 0"

rn I can't figure out what that means for the division

integer division rounds down, toward negative infinity

er wait

interesting

it's toward 0 in C, but it's toward -infinity in python

how about that

``` $ cat test.c && gcc test.c && ./a.out #include <stdio.h> int main(int argc, char *argv[]) { printf("%d\n", -13 / 12); return 0; } -1 ```

It's right shift that rounds toward minus infinity

```c #include <stdio.h> int main(int argc, char *argv[]) { printf("%d, %d\n", -13 / 8, -13 >> 3); return 0; } ```

this outputs `-1, -2`

well strictly speaking, right shifting a signed negative integer is undefined behavior in C

on x86 it sign-extends

but the spec should probably say that when right shifting a negative-integer, it is assumed that the extra bits are populated by the sign bit

it might say that but if it doesn't it should, as it's Undefined Behavior in C

I ran into this issue with the weighted predictor because there's a number of right shifts

which cannot be replaced with integer divisions

luckily the JVM has two operators, `>>` and `>>>` for signed ints, which are fully specified: `>>` sign-extends and `>>>` zero-extends

that's the spirit...

So at this point I'm confused. I have a modular stream that's being decoded identically to libjxl

gaborish is disabled in the frame header, as is epf

many of the values in this modular stream are out of range. the BitDepth header in the image header field tells me that `exp_bits = 0` and `bits = 8`

There is exactly one frame in the image

but simply clamping these values between 0 and 255 yields the wrong image

what is libjxl doing that lets it take this modular stream and reconstruct it properly?

`factor` in libjxl is `1f / 255f`

And whatever it is doing, why is this not described in the spec?

patches, splines, and noise are all disabled.

XYB is false, as is YCbCr.

oh *fuck* it *IS* xyb encoed. holy crap, ignore me, lmao.

Okay, here's something. The default values of `m_x_lf_unscaled` are 4096, in table G.2, but I'm pretty sure it should be `1/4096`

since later in Table L.2.2 it says `X = X’ * m_x_lf_unscaled`

and I believe X is supposed to be divided by 4096 here, not multiplied

OH!

^ not only is this true, but the spec fails to mention that they're supposed to be divided by 128.0f

if not all default

```c++ Status DequantMatrices::DecodeDC(BitReader* br) { bool all_default = br->ReadBits(1); if (!br->AllReadsWithinBounds()) return JXL_FAILURE("EOS during DecodeDC"); if (!all_default) { for (size_t c = 0; c < 3; c++) { JXL_RETURN_IF_ERROR(F16Coder::Read(br, &dc_quant_[c])); dc_quant_[c] *= 1.0f / 128.0f; // Negative values and nearly zero are invalid values. if (dc_quant_[c] < kAlmostZero) { return JXL_FAILURE("Invalid dc_quant: coefficient is too small."); } inv_dc_quant_[c] = 1.0f / dc_quant_[c]; } } return true; }```

specifically the 1.0f / 128f

oh damn, so many places where these quantizers get rescaled or inverted, so easy to get lost in that

Yea, they're encoded *in* the bitstream as 1f / foo

The actual behavior is as follow:

`m_lf_x_unscaled` defaults to `1 / 4096`

instead, if it's in the bitstream, you just let `m_lf_x_unscaled = F16() / 128f`

It's already in the bistream in the denominator mode

This means that if it's encoded in the bistream as `1/256f` then it'll end up as `1/32768f` when multiplied by `X'`

Does that make sense?

don't divide the default values by 1/128, those are baked in.

I thought I reported these issues about quantizers, weren't them incorporated into the current draft?

must have been missed

or I forgot to report them... let me check

oh, my bad, that was indeed the case

<@853026420792360980> maybe you want to keep eyes on "TODO spec" comments in J40

I'm just reporting them as I go

this channel didn't exist at the time you made tho spec comments

btw, you have a bug in j40

I think I did mention them back when I was talking in <#794206087879852106> , but I must have forgotten to report them formally

in predictor 8 you have `p.w - (p.ww + p.nw - p.ne)` or something

which is not the same thing as `p.w + p.n - p.nee` or whatever it is, evaluated one pixel over

most of the time it's the same, but `if x = 1`, then `p.ww` evaluates to `p.w`, but `p.w` one pixel to the left evalutes to `p.nw`

so in j40 you need an `if x == 1` in predictor 8

so is J40 diverging from libjxl in that case?

I think I have taken extra care to handle corner cases (there are *three* sets of them, I recall) but I may have missed one

in that specific case, yea

Edge cases are so annoying

I'm gonna take some time to document all remaining spec bugs that are only in the J40 comments today

cause sooner or later, someone (in this case thebombzen) will be hit by those bugs anyway

and available bug reports are better than thorough bug reports

I'm currently confused about how you're supposed to crop the LF groups if the image isn't a perfect power of 2

in the case of Pass Groups since they (mostly) correspond 1-1 to pixels, you just crop the group by the amount it overflows

I have a 3264x2448 image, which has four LF groups

3264/8 is 408

and 2448/8 is 306

so I'd assume that LF Groups would be cropped if they overflowed, i.e. 408 - 256 = 152

but somehow that isn't working

and I'm not entirely sure why

I'm not sure if this is documented or not, but as I understand, you first round dimension to next multiples of 8; this means that there are some blocks where some edges are left unused.

everything here is a multiple of 8 though, 408, 256, and 152

for groups and LF groups, assuming you've rounded dimensions accordingly, they have no further restrictions; any integral number in width and height (in terms of blocks) should work

ah wait

~~sorry, my comment should only apply to *groups*, not LF groups~~ I rechecked my code and I think this applies both to groups and LF groups

Yea, I figured out Pass Groups

``` Index Width Height x0 y0 0, 128, 256, 0, 0 0, 64, 128, 0, 0 0, 64, 128, 0, 0 0, 256, 256, 0, 0 0, 128, 128, 0, 0 0, 128, 128, 0, 0 0, 128, 256, 0, 0 0, 128, 256, 0, 0 0, 256, 256, 0, 0 0, 256, 256, 0, 0 1, 128, 256, 128, 0 1, 64, 128, 64, 0 1, 64, 128, 64, 0 1, 152, 256, 256, 0 1, 128, 128, 128, 0 1, 128, 128, 128, 0 1, 128, 256, 128, 0 1, 128, 256, 128, 0 1, 152, 256, 256, 0 1, 152, 256, 256, 0 ```

I get through the channel list like so, and then I hit a TOC boundary

I've discovered through debugging that overrunning a TOC boundary typically means that you are decoding the wrong thing

so in your case (assuming group size of 256 and LF group size of 2048), you have four LF groups of dimensions 2048x2048, 1216x2048, 2048x400, 1216x400 respectively

which are 256x256, 152x256, 256x50, 152x50 in terms of block counts

that's what it appears to be decoding

but then it barfs. it's possible that there's a bug somewhere else though that's causing it to barf

there would be 130 groups, where first 8 groups are the topmost groups in the first LF group, next 5 groups are for the topmost groups in the second LF group, next 8 groups are for the first LF group, next 5 are for the second LF group and so on

yea, and the passgroups are decoding fine

all 130 of them

the issue is the LF groups

can you pinpoint where you are stuck?

I'm trying to figure out if I'm calculating the LF dimensions incorrectly

cause I thought if you've done groups LF groups should follow naturally...

I may just have a bug elsewhere for all I know though.

I believe your code does recognize there are 4 LF groups, otherwise TOC wouldn't work, right?

then I believe you are going to decode the LfGroup bundle

absolutely

OH

stream index was wrong

which consists of ModularLfGroup (for LQIP I believe), LF coefficients and HF metadata where the last two are for VarDCT; which one are you stuck at?

OH

causing the MA tree to collapse

it was a bug elsewhere

that's pretty understandable

``` for ModularLfGroup: 1 + num_lf_groups + LF group index; ``` <@794205442175402004> This is incorrect, it's supposed to be `1 + LF Group index`

you don't add the number of LF groups to the ModularLfGroup stream index

I'm not sure what the LF Coefficients and HF Metadata are supposed to be though

hmm?

but the stream index was incorrect

I'm checking that part of spec, it used to be pretty much wrong

as soon as I changed that it decoded fine

and I think I've checked that

do your image have ModularLfGroup?

Yes, this is modular.

I haven't started working on VarDCT yet.

I mean, ModularLfGroup is normally empty if you don't have squeeze

so I guess that's why I missed that part

this is lossy modular, as a test.

I have squeeze

yeah, ModularLfGroup only appears when you have hshift or vshift of at least 3

which doesn't happen if you don't have squeeze

so the question is: normally we are supposed to have different (fragments of) MA tree for LF coefficients, ModularLfGroup and HF metadata

because of course these three may have different optimal MA trees

but as it stands, there is no way in an MA tree to distinguish LF coefficients from ModularLfGroup

so is there any impact of this bug?

stream index, property 1

but stream indices for LF coefficients *are* already `1 + LF group index`, which I believe is correct because otherwise J40 wouldn't work

... huh.

yeah, I've checked J40 and it is indeed `1 + LF group index` (or `sidx0`, as J40 says)

`sidx1` (`1 + num_lf_groups + LF group index`) was reserved for ModularLfGroup which is not yet implemented

I'll check libjxl to be sure

wait, hm.

Looks like illegal final ans state

maybe my test images just didn't have stream index nodes

so maybe I'm just decoding garbage

ah, I was just decoding garbage

that's mb

I set it back to what it had been, and the first LF Group verified successfully

ignore me, bug not happened.

ah, good

so that part of spec seems okay

I'm still confused about the LF group sizes though

where am I supposed to truncate the LF Groups when they overflow out of the side of the image?

I really appreciate it that we now have two independent implementers sorting out potential spec issues amongst themselves, by the way. This will lead to a 2nd edition of the spec that will be so much better than the 1st edition. Thanks so much!

I have a 408/306 image after dividing by 8, and the LF groups then should be variations of 256x256

so I'm producing a 256x256 upper left corner

152x256 upper right

but upon decoding that second one I overrun the TOC buffer, which usually means I'm decoding the wrong thing

Yes, should be same splitting logic as the hf groups

so in this specific case, 152x50 for the lower right is correct, right?

if so, I must have a bug elsewhere

Overrunning toc is indeed a sign of a bug, no encoder produces overlapping sections and we should probably forbid it

no I mean I hard-isolate the TOC into buffers because it makes that easier to detect

Same thing, no section should read more bytes than the length indicated in toc

I thought that was pretty self-evident

at least

but either way, if those dimensions are correct then my bug is elsewhere

The dimensions are correct — you rounded up when dividing by 8, right?

it's a multiple of 8, so no, but....

Must be bug elsewhere then

gonna do my usual trick of dump output of libjxl and diff it to see where it differs

and, I figured it out, I wasn't cropping the corresponding higher-shift channels either

so the 256x256 channels needed to be cropped to 152x256, but the 128x256 channels needed to be cropped to 76x256

ugh, how do you handle it if these are *odd*

Just wondering, do you use `jxlinfo` for this?

(oh, I misread, it's probably `printf()`-debugging)

yea, I printf to stdout and redirect

basically I just straight up `printf("%zu, %zu, %d\n", x, y, p[x])` or something

and compare the output

most useful compression debugging technique *ever*

I just started glancing at the spec - pseudocode operators are defined as being the same as those in C++, but signed integer overflow is undefined in C++. Is this intentional?

all operators are defined as if there is an integral type of infinite size (there should be a mention), so it should never overflow

in reality, of course, the implementer should be aware that the calculation can overflow

yeah I don't see such a mention

> Expressions and variables of which types are omitted, are understood as real numbers.

section 4.4

speaking of which

Section 4.3, missing a closing paren `)`

mostly* defined as being the same in C/C++ btw in C, right shifting a negative signed integer is undefined behavior

but in the spec is clearly defined. in practice right shifting signed integers will sign-extend on x86

which is also what happens in the spec

Section K.3.1

``` patch[i].x[j] = UnpackSigned(DecodeHybridVarLenUint(5)) + patch[i].x[j − 1]; patch[i].y[j] = UnpackSigned(DecodeHybridVarLenUint(5)) + patch[i].y[j − 1]; ```

these should say `DecodeHybridVarLenUint(6)`

the context is wrong

note that in `patch_dictionary_internal.h`, it lists ` kPatchOffsetContext = 6,` on line 22

and in `dec_patch_dictionary.cc` on line 110 we have this: ```c++ pos.x = positions_.back().x + UnpackSigned(read_num(kPatchOffsetContext)); pos.y = positions_.back().y + UnpackSigned(read_num(kPatchOffsetContext)); ```

furthermore I tested this against `patch_lossless.jxl` conformance sample and the final ANS state fails (i.e. != 0x13000) if you use the 5 in the spec, but it succeeds if you use 6

I'm trying to understand the examples in B.2.2 (U32)

for the first example, you read two bits, 0, then 1, which is 0b10 = 2, which results in 32

following the same bit ordering for the second example, you read two bits, 1, 1, which is 0b11 = 3, so you then read 8 more bits, 1, 0, 1, 0, 0, 0, 0, 0, which is 0b00000101 = 5 != 7

or am I getting the bit ordering for the u(n) operation wrong

okay, I think this is a typo in the spec, I just yanked out jxlatte's bit reader (because it was convenient) and the second example returned 5

It's in little endian order

010111 means first you read two bits-> 01, with 0111 remaining

then you look at those two bits

01 is in *little-endian* mode so that's actually 0b10

which is 2

hm, yea it's definitely a typo in the spec

at that point it should pick u(6)

but there's only 4 bits remaining

it should say `101110`

if it had said 101110, then you'd pick 0b10, with 0b1110 remaining

then that would go to 1, b/c little endian, and then go to u(4), reading 0b1110, little endian, equals 7

so it should say `U32(u(2), u(4), u(6), u(8)): 0b101110 -> value = 7`

and it should say `U32(8, 16, 32, u(7)): 0b01 -> 32`

someone messed up endianness

good catch!

or should this be 0b011101?

as in the first digit is most significant, as it is in most programming languages

I'm using it in little-endian order. so if it occurs in the bitstream as 101110 then it would end up being 011101 if you wanted to populate it in a programming language, yes

The spec doesn't specify what happens if you have a property whose value is out of range

the correct behavior is that for properties >15, the property defaults to 0, unless computed as in the bottom of section H.4.1.

The spec doesn't mention this, though.

ah, good point

ugh, I thought it was simply an incorrect bitstream

no it's not, e.g. you could use properties that refer to the previous channel in a global tree that also gets used for the very first channel

I ran into this issue when attempting to decode this JXL art piece: https://discord.com/channels/794206087879852103/824000991891554375/1038632475372560436

channel 0 of the stream has property19

In C.2.1, what exactly does `lz_cfg` refer to? is it a part of the entropy decoder state?

If lz77.enabled, the decoder sets lz_cfg = num_dist++, and reads lz_len_conf = HybridUintConfig(8)

so basically one extra pre-clustered distribution is added for the lz77 distances

the decoder reads a HybridUintConfig for lz77, which is separate from the HybridUintConfig added for the extra distribution

However, `lz_len_conf` is the variable name used in section C.1. if It later calls it `lz_cfg`, that's a typo.

when reading a symbol, first a token is read which can be either a literal or represent an lz77 length, and if it's an lz77 length, then another token is read using a fixed (but signaled) hybriduintconfig to get the distance

I think the spec is correct but the variable names and way it is described is not very clear

`lz_cfg` is the index of the lz_len_conf, I see.

lz_len_conf is a hybriduintconfig used to interpret lengths

lz_cfg is a context used to read distances

It might make sense to call it `lz_distance_ctx`

just then it's really clear that it's a context ID

lz_len_conf and lz_distance_ctx would be good names, I agree

the confusing thing is the lengths don't need a context because the token is just a regular token that happens to be larger than some offset

oh while we're on the topic of section C.3.3.: ```c++ else if (distance < 120) { distance = kSpecialDistances[distance][0]; distance += dist_multiplier * kSpecialDistances[distance][1]; ``` this doesn't work you probably mean something like ```c++ else if (distance < 120) { offset = kSpecialDistances[distance][0]; distance = offset + dist_multiplier * kSpecialDistances[distance][1]; ```

in particular, consider what happens when the offset is negative

the first one crashes the code

libjxl handles it correctly by doing it in one expression, but the spec is incorrect

right, d'oh

so easy to write silly bugs in code that doesn't get executed

Yea, that's the main difference between proofreading code and trying to implement it

and at ISO they only did the former, if even that

many of the comments we got from national bodies were more about the dots and commas in boilerplate text than about the actual technical meat

one thing I find convenient about doing this in java is attempting to index an array with a negative number immediately throws an exception, which crashes the process. the JVM enforces those memory barriers. it means those sorts of off-by-one errors fail-fast

unlike in C where you need valgrind or a similar tool to tell you, unless x[-1] happens to hit a dead page

so `lz_cfg` (which should probably be named `lz_distance_ctx`) is a field in the entropy decoder state that refers to some index in `clusters`?

when Lz77 is enabled, it reads a token. if the token is too big, it goes "oh, this must be an Lz77 length"

but Lz77 uses length/distance *pairs*

so it then reads the distance, using `lz_distance_ctx` as the context.

also keep in mind that `clusters` is a map. it maps contexts to clusters. since in practice some of the contexts will have very similar frequencies, clustering lets the stream consider two contexts to be the "same"

so it maps a context ID to a cluster ID

which can and frequently will have a lot of overlap

it's easy to treat it as an array, but conceptually it's a function `int mapContextIDToClusterID(int contextID) // returns cluster ID`

context clustering was invented in brunsli/pik (maybe already in brotli / lossless webp?) as a way to have relatively large context models while keeping the number of histograms to signal manageable

MA trees were invented in FLIF as a way to have even larger context models (ones where even the context map itself would be too large) while keeping the number of histograms (or in case of FLIF, CABAC chance tables) manageable

and in JXL's modular mode you get both, which basically boils down to the MA tree effectively being a DAG instead of a tree

(i.e. you can merge two leaf nodes however distant they are)

in jxlatte I save some effort by flattening static properties but less so than in libjxl. unfortunately jxlatte is pretty slow

the way I have it implemented in jxlatte is basically I have an MATree that contains a pointer to two child trees, also of type MATree. and I walk down the tree by doing basically `while (!tree.isLeaf()) tree = tree.decide();`

i recommend starting with slow and correct and worrying about speed later, because some things really get quite a lot more complicated and bug-prone if you want to do them fast too

yeah the naive branchy MA tree evaluation is good enough, it works, it's just slow

luckily this method makes walking down static properties pretty easy since I have a function that just walks down the tree until it hits a non-static property then aborts and returns the resulting tree

so I can save a bunch of cycles that way

I also compute properties on the fly, which can be slower if they need to be used again

but can be faster if they don't need to be used

eventually we might want to do property computation and MA tree walking with some kind of dynamic code generation, but that's scary and tricky so for now we just stick to some specializations and some speedup tricks like turning binary trees into quadtrees

but there's a few optimizations I could do, like I could make the row-arrays slightly bigger than they need to be, and populate them with the edge-case predictors that way there's no branching required when doing west() or north()

since doing `y > 0 ? buffer[y - 1][x] : x > 0 ? buffer[y][x -1] : 0`

ugh

so much branching

this can be optimized as a CMOV by a compiler but on the JVM I'm pretty sure it's a branch

not entirely sure tho

I'm targeted the JVM

yes, all of that helps, and you can do whatever is easy enough to do, but I think at this point having a full independent decoder that works though slowly is more valuable than having a partial one that is fast

yea, this is all after I get full feature support

it's lower priority

the contest submission art you posted is going to be good for testing splines and noise tho, as well as frame blending

so I'm happy about that

yeah it was an art in the spirit of what jxl_from_tree was originally made for: producing test bitstreams 🙂

pretty on-point since a lot of my debug test samples came from <#824000991891554375>

I like it when fun and art can be at the same time a useful engineering tool used in something as 'boring' as spec verification

human-assisted fuzzing

in C.2.1/C.2.2, where does `num_clusters` come from if `num_dist != 1`?

https://discord.com/channels/794206087879852103/794206170445119489/1011659525385883788

oh, it's supposed to be calculated by the max of clusters[i] + 1

this should probably be specified

It is in sectino C.2.2

> The output of this procedure is an array clusters with num_dist entries (one for each pre-clustered context), with values in the range [0, num_clusters). All integers in [0, num_clusters) are present in this array, and num_clusters < 256. Position i in this array indicates that context i is merged into the corresponding cluster.

since values arei in the range of `[0, num_clusters)` it implies that it occupies all values x for which 0 <= x < num_clusters

which then tells you that num_clusters is the smallest integer greater than all of the xs

It could be more explicit, but it it technically *is* specified

if channel data ranges from `[0.0f, 1.0f]`

``` R = Y + 0.5 + 1.402 * Cr; G = Y + 0.5 − 0.344136 * Cb − 0.714136 * Cr; B = Y + 0.5 + 1.772 * Cb; ```

how does R or B ever end up as less than 0.5?

if Cr = Cb = 0 you end up with Y + 0.5, not Y

or is this one of those scenarios where Cb and Cr range from `[-0.5, 0.5]`?

same with Y?

because I'd assume that grayscale would be R=G=B=Y

Cr and Cb get negative, yes

And Y too, yes

Y=0 is gray

I think

Y=0 being 50% gray means these all range from [-0.5, 0.5]

which is fine, I just wanted to double check

Section K.4.1 says this:

> Then, UnpackSigned(DecodeHybridVarLenUint(5)) is used four times thirty-two times to obtain, in this order, sequences of thirty-two integers representing the following coefficients of the spline along its arc length: > q_dctX[32], the decorrelated and quantized DCT32 coefficients of the X channel of the spline > > q_dctY[32], the quantized DCT32 coefficients of the Y channel of the spline > > q_dctB[32], the decorrelated and quantized DCT32 coefficients of the B channel of the spline > > q_dctSigma[32], the quantized DCT32 coefficients of the σ > parameter of the spline (defining its thickness)

are all 32 of the `q_dctX[32]` read first, and then all 32 of the `q_dctY[32]`, and then all 32... etc.

or are they interleaved?

It's not clear to me from how this is phrased.

Another question

> These coefficients are dequantized and recorrelated (see I.2.3) as follows:

Splines can be used in Modular Mode (e.g. lossy XYB), but the LFChannelCorrelation bundle (defined in I.2.3) is only present in LFGlobal in VarDCT mode.

Do you use the default value in this case?

yes

pretty sure cb and cr are in +-.5 range, and y is in 0-1 range...

if Y is `[0, 1]` then the inverse YCbCr transform appears to be incorrect

let me check in libjxl

ok, no, y is also +-.5

I am not quite sure why we did that as XYB's Y isn't, but oh well

Because that is how jpeg does it

I think

Section K.4.1 provides pseudocode but it decodes quant_adjust in the wrong order

this is in section K.4.1: ```c num_splines = DecodeHybridVarLenUint(2) + 1; quant_adjust = UnpackSigned(DecodeHybridVarLenUint(0)); last_x = 0; last_y = 0; for (i = 0; i < num_splines; i++) { // etc. ```

splines.cc line 549:

```cpp JXL_RETURN_IF_ERROR(DecodeAllStartingPoints(&starting_points_, br, &decoder, context_map, num_splines)); quantization_adjustment_ = UnpackSigned( decoder.ReadHybridUint(kQuantizationAdjustmentContext, br, context_map)); ```

notice that `quantization_adjustment_` is decoded after reading the starting points, not before, unlike what it says in the spec.

> Suggestion for a spec extension / bit of a long shot: Allow horizontal and vertical flips of patches. The point is that you've already done the work by defining mirrored coordinates - so keep the source coordinates of the patch normal but allow the destination coordinates to be mirrored. It's not really a bitstream change... (suggestion from <@967519502977888367> )

We did consider adding bits for flips and it could be useful for symmetric things like many letter shapes or circles/ellipses, but we ended up not doing it.

Doing it via negative / out of bounds offsets and using mirroring semantics could work with just a small spec change, but it would make it impossible to use patches that cross the image edge, like e.g. a sprite that is partially off-screen (since the off-screen part would end up getting mirrored and messing up the on-screen part)

(though such patches could still be done by doing them separately with smaller source rectangles, which would be a small extra signaling cost but not hugely)

The implementation of patch blitting would be a bit trickier though if it needs to do mirroring — naive per-pixel coordinate mirroring would be easy to implement but to do it efficiently there would be one code path for each of the 4 possible orientations, and the case of image edge crossing would need to be handled too (either by not allowing it or by splitting the blitting in 2 or 4 operations)

I'm glad it was considered 🙂 Though pixelwise patch blitting is surely pretty quick compared to other steps of the image decode..?

I wonder how practical 'fake' mirroring is with the current spec. Say you want to do a vertical mirror. Then take one n x 1 patch per row and assemble them in reverse order. Finally hope that the large amount of patch metadata compresses well.

Not that it really matters, I think I stated it the wrong way round from the way I first thought of it: instead allow the source patch to be mirrored but insist that the destination is completely in-bounds. Then crossing the image edge isn't an issue - you get a partially mirrored patch rather than corruption.

as far as I'm aware, the design of extensions though is based on PNG, in that a decoder is required to ignore extensions it doesn't understand

which makes implementing extensions that change rendering behavior a bit iffy

Right, and then if the source region overlaps the edge, it could actually make sense, e.g. you could put the left half of an A at the right end of the patch frame, take a source that goes over the right edge, and that would blit as a full A

yeah this would not be done as an extension but by defining behavior for bitstreams where the current behavior is not really clear: what if the source rect for a patch is outside the source frame?

currently the spec says "All referenced samples are within the bounds of the reference frame." so this is an invalid bitstream atm

oh yea, if that happens I return an error and refuse to decode

we could make it a valid bitstream; existing decoders will not decode it of course or at least not correctly, so it's probably a bad idea

what I discovered is that for building a decoder, it's helpful to aggressively throw errors at non-compliance

because a lot of those errors end up being caused by bugs

rather than by actual noncompliance and they get caught

I'll implement this, creating the first fragmentation of the JXL ecosystem 😏

lol

well decoders are free to do what they want on non-compliant bitstreams

the real problem is when encoders start to rely on what decoders do with them

but I mean, throwing errors rather than making best-effort decodes is easier for building a decoder