JPEG XL

since things like incorrect parsing can cause values to be invalid

and it catches that

yeah throwing errors is certainly the best thing to do, even besides the debugging usefulness

the best decoder is one that decodes all compliant bitstreams and refuses to decode any non-compliant one — we are probably missing some of those cases in libjxl atm btw

TIL how ISO specification idiom works: In this case "All referenced samples are within the bounds of the reference frame" makes me think "well isn't that nice" but it's intended to convey "If any sample is outside the bounds of the reference frame then the bitstream is invalid so continue at your own risk".

silly ISO

yeah I agree the ISO language is confusing

I have heard from many people that ISO standards tend to be incredibly detailed and yet written such that it's hard to tell exactly what you're supposed to do, leading to interoperability problems.

if we say "they are within the bounds" it means "if they're not, this is not a valid bitstream"

if we say "they must be within the bounds" it means "no need to check, it's physically impossible that they are not within the bounds, e.g. it's a consequence of the bitstream syntax that they have to be"

oh what

ok i can only find one occurance of 'must' and it's not confusing

> Levels are defined in such a way that if a decoder supports level N, it must also support lower levels.

yeah this statement is a consequence of the way the levels are defined, so we can use "must" there

it feels like the ISO language was designed for countries to agree on common standards for traffic signs, not for nitpicky geeks trying to level up the internet

most of the normative statements are done with "shall" in ISO, but we avoided making all sentences sound biblical and having hundreds of "shall"s by leveraging this sentence in clause 7: > These specifications are normative in the sense that they are defining an output that alternative implementations shall duplicate.

that one single "shall" makes basically all the rest of the document have implicit shalls

half the job of an ISO staff editor is making sure the use of shall, should, must etc is done correctly, and it's very confusing if you're not used to it so I imagine most first-time spec editors get it wrong all the time

so ISO speak is a bit like legal speak in that it mostly reads like normal English but the words don't always mean what you think they do

they mostly mean what is specified in long intros many skip

but it was pretty clear at least to me that anything not specified was not part of the spec

it's just kinda how you're used to approaching specifications

Is that supposed to be a plus between 18 and u(6), rather than a new line?

It is

Yeah I also just saw that, will be fixed.

Section K.5.2, definition of SplitMix64, has `0×` instead of `0x` in front of the hexadecimal numbers

> The strength of the noise is computed as follows. Given the X and Y samples of the input pixel, let InR = Y + X and InG = Y − X. Let InscaledR = InR * 6 and InscaledG = InG * 6. These values are split into integer parts in_intR = floor(InscaledR) and in_intG = floor(InscaledG) and fractional parts in_fracR = InscaledR − in_intR and in_fracG = InscaledG − in_intG. If the value InScaledR or InScaledG is 8 or more, the corresponding integer and fractional parts are clamped back to 7 and 1 respectively. > > Using the LUT values decoded as described in K.5.1, the strengths SR and SG are defined as SR = LUT[in_intR] * (1 − in_fracR) + LUT[in_intR + 1] * in_fracR and SG = LUT[in_intG] * (1 − in_fracG) + LUT[in_intG + 1] * in_fracG. Each strength is clamped between 0 and 1.

What if `InR` or `InG` is negative?

it very well could be the case

looks like libjxl clamps to 0

should say that in the spec tho

Also

> If the value InScaledR or InScaledG is 8 or more, the corresponding integer and fractional parts are clamped back to 7 and 1 respectively.

This will overflow `LUT` at 7

because LUT is an array of size 8

so LUT[7 + 1] will fail

how did you even notice 🤣

because I copied and pasted and got a syntax error <:KEKW:643601031040729099>

it should

ahhhhh you have no idea how many times I ended up changing the number for that clamping

I thought I got it right ... what does libjxl clamp to? 6?

I double checked the LUT overflow, apparently libjxl does the following ```cpp float kScale = 6f; if (in_scaled >= kScale) in_int = kScale - 1; in_frac = 1f; } else { in_int = floor(in_scaled); in_frac = in_scaled - in_int; } ```

Yup

`in_int` is capped at 5, because of this

which means LUT[7] is never used

although I believe this is a bug in libjxl

where it should do the following:

```cpp float kScale = 6f; if (in_scaled >= 7f) in_int = 6; in_frac = 1f; } else { in_int = floor(in_scaled); in_frac = in_scaled - in_int; } ```

since `float lut[8]` is sealed in stone

so either Scale needs to change to 7, or the clamping should change, cause otherwise something is weird

in either case the spec causes an overflow as the spec is written

So bug in both libjxl and spec? Ugh

perhaps

unclear if libjxl is bugged, but probably is, given that lut[7] is not used

spec is definitely bugged as it will cause an overrun of the buffer

So spec should probably say: > If the value InScaledR or InScaledG is 7 or more, the corresponding integer and fractional parts are clamped back to 6 and 1 respectively.

And libjxl should say what you wrote above

or equivalent in HIghway-ese

this is pseudocode

it should also mention that they're clamped to 0 if negative

I hate that code 🤣 it's too hard for me

I'm confused about noise groups, and how they interact with upsampling

~~that's two of us~~

iirc the way that works is that noise is never generated in tiles bigger than 256x256, even if upsampling happens

the spec and libjxl differ in randomness seed behavior

libjxl generates one batch of 16 numbers for every 16 pixels wide, and then it generates an extra batch for the remaining stragglers

it does this even if the size of the remaining stragglers is zero

i.e. if the width of the group is a multiple of 16, which it often will be, it generates an extra batch and does nothing with it

this isn't in the spec

I don't know if this is just a libjxl bug, or if it should be changed in the spec

uh, I don't think it's supposed to do that

at the very least it's not intentional...

dec_noise.cc, line 72

``` size_t x = 0; // Only entire batches (avoids exceeding the image padding). for (; x + kFloatsPerBatch <= xsize; x += kFloatsPerBatch) { rng->Fill(batch); for (size_t i = 0; i < kFloatsPerBatch; i += Lanes(df)) { BitsToFloat(reinterpret_cast<const uint32_t*>(batch) + i, row + x + i); } } // Any remaining pixels, rounded up to vectors (safe due to padding). rng->Fill(batch); size_t batch_pos = 0; // < kFloatsPerBatch for (; x < xsize; x += N) { BitsToFloat(reinterpret_cast<const uint32_t*>(batch) + batch_pos, row + x); batch_pos += N; } ```

`x + kFloatsPerBatch <= xsize`

I think this should probably be a strict inequality

then, the remaining batch gets filled for 1-16, rather than for 0-15

notice that if `xSize` is a multiple of `kFloatsPerBatch` (i.e. 16)

then we have `x + 16 <= xSize` as the limiting condition, which is the same thing as `x < xSize`

so this fills the entire row

and then `rng->Fill(batch);` is called again, unconditionally.

in that case, i'll code to spec and assume this is a libjxl bug

can you file an issue?

https://github.com/libjxl/libjxl/issues/1888

thanks!

I propose we indeed treat this as a libjxl bug and change some noise on existing images

what about the other discrepancy, with the scale factor?

where libjxl maps `scaled >= 6f` to `intPart = 5` and `fracPart = 1f` (causing `lut[7]` to be unused)

or the spec maps `scaled >= 8f` to `intPart = 7` and `fracPart = 1f` (causing `lut[8]` to blow up)

IMO it should map `scaled >= 7f` to `intPart = 6` and `fracPart = 1f`

we also should fix, and ideally for the last time as we already "fixed" it way too many times

can you also open a bug to libjxl for this?

yup, there's one more spec issue with noise, the spec's missing a 0.5f multiplier somewhere

lemme dig it up

I gotta go do some real lifing but I'll try to open an issue about that soon

we discussed that LUT thing a while ago, I believe

it’s that code, isn’t it?

yes, but that's not the issue

the current code isn't sawtoothed in either the spec or libjxl, the issue is the integer part threshold

not the fractional part

right, but I thought we had specifically fixed that too

e.g. https://github.com/libjxl/libjxl/pull/1238/files

somehow this change isn't in git main libjxl though?

well at least the one in `dec_noise.cc`

now that code is in `stage_noise.cc`

https://github.com/libjxl/libjxl/blob/main/lib/jxl/render_pipeline/stage_noise.cc#L71

and it appears to use the older version

oh no

thanks for spotting this

yw :)

the seeding change did make it into main though

Last spec discrepancy relating to noise synthesis, btw:

> `Given the X and Y samples of the input pixel, let InR = Y + X and InG = Y − X.`

it's actually `(Y+X)/2` and `(Y-X)/2`

https://github.com/libjxl/libjxl/blob/main/lib/jxl/render_pipeline/stage_noise.cc#L192

notice that you're multiplying by `0.5` first

here `half` is just `const auto half = Set(d, 0.5f);`

I assume this is supposed to happen and a spec bug, not a libjxl bug

so I'm not going to report it on the issue tracker. but in either case spec and libjxl don't match, so at least one of the two is not correct

> The dequantization matrices are computed for each possible value of the DctSelect field as the inverse of the weights matrix.

How am I supposed to compute the inverse of the weights matrix if they are not square?

unless you mean reciprocols taken element-wise

in which case, can I assume they are nonzero?

```cpp GetDCTQuantWeights(params) { bands(0) = params[0]; for (i = 1; i < len; i++) { bands(i) = bands(i − 1) * Mult(params[i]); /* bands[i] >= 0 */ for (y = 0; y < Y; y++) for (x = 0; x < X ; x++) { dx = x / (X − 1); dy = y / (Y − 1); distance = sqrt(dx*dx + dy*dy); weight = Interpolate(distance, sqrt(2) + 1e−6, bands, num_bands); weights(x, y) = weight; } } } ```

this doesn't make sense

you're overwriting `weights[x][y]` each iteration of params

and it's not referenced again

is this supposed to be `+=`?

or is it not supposed to be a nested for loop?

> For encoding mode DCT2, params(c, i) are copied in position (x, y) as follows, where rectangles are defined by their top right and bottom left corners

Is this supposed to say "top left and bottom right"?

> For encoding mode DCT4×8, the weights matrix is obtained by copying into position (x, y) the value in position (x, y Idiv 2) in the 8 × 4 matrix

and 8x4 matrix has 8 rows and 4 columns. it's taller than it is wide

should this say 4x8?

using the mathematical convention of Rows by Columns, not the convention of width by height

DCT2 leaves weight[0][0] undefined

does it deafult to 1?

Yup (and yes, it is element by element)

*facepalm* no it is not supposed to be nestes

Doesn't matter, weight 0 0 is never used

I.6.5:The pairs (x, y) in the LLF vector is sorted in ascending order according to the value y * bwidth + x.

I assume this is supposed to say `y * (bwidth / 8) + x`

otherwise it overflows the size of LLF

Isn't bwidth the width in blocks, so already /8?

it's the matrix width for a specific block

> Let nb_block_ctx be equal to max(block_ctx_map) + 1. The decoder reads a histogram with `495 * num_hf_presets * nb_block_ctx` pre-clustered distributions D from the codestream as specified in C.1.

Where's this 495 coming from? I can't find it in libjxl code.

does it actually matter as long as it's `y * <something greater than any x> + x`?

yes because otherwise you end up with zero padding and overflow

no?

it just says how they are sorted

well, I suppose not, it's an order

not what their positions are

not an index

in practice they do end up at index `y*bw/8+x`

that's a good question, let me check

upon further inspection, it appears to be 458 + 37

where 37 is kNonZeroBuckets

and 458 is kZeroDensityContextCount

yep

> Finally, for k in the range [num_blocks, size), the decoder reads an integer ucoeff from the codestream

`size` is not defined here

unless it's the natural coeffient order size way back in hfpass

if that's what it is, this is very confusing

that spec doesn't say what happens if it reads non_zeroes directly and it's immediately zero

spec doesn't disallow this

doesn't that just make all of AC 0?

spec doesn't say that

Typo in section I.3.2: ```c BlockContext() { idx = (c < 2 ? c ^ 1 : 2) * 13 + s; idx *= (qf_thresholds.size() + 1); for (t : qf_thresholds) if (qf > t) idx++; for (i = 0; i < 3; i++) idx *= (lf_thresholds[i].size() + 1); lf_idx = 0; for (t : lf_thresholds[0]) if (qdc[0] > t) lf_idx++; lf_idx *= (lf_thresholds[2].size() + 1); for (t : lf_thresholds[2]) if (qdc[2] > t) lf_idx++; lf_idx *= (lf_thresholds[0].size() + 1); // <- TYPO HERE for (t : lf_thresholds[1]) if (qdc[1] > t) lf_idx++; return block_ctx_map[idx + lf_idx]; } ```

I believe that line should say `lf_thresholds[1].size() + 1`

not `lf_thresholds[0]`

Typo in section I.3.2: ``` CoeffNumNonzeroContext[64] = { 0, 0, 31, 62, 62, 93, 93, 93, 93, 23, 123, 123, 123, 152, 152, 152, 152, 152, 152, 152, 152, 180, 180, 180, 180, 180, 180, 180, 180, 180, 180, 180, 180, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206, 206}; ```

The `23` in the top row, 4th from the right should be 123

also, libjxl starts these arrays with `0xBAD` instead of zero

I don't know if that's because it can be anything, or if matters

typo in section I.3.2:

``` if (k == num_blocks) { if (non_zeros > size / 16) prev = 1; else prev = 0; } ``` this condition should be `prev = 0; else prev = 1`

libjxl dec_group.cc line 486 has this: `size_t prev = (nzeros > size / 16 ? 0 : 1);`

that one should never be used

Section I.2.2: there's a +1 missing in the QF Thresholds decoding

``` for (i = 0; i < nb_qf_thr; i++) qf_thresholds.push_back(U32(u(2), 4 + u(3), 12 + u(5), 44 + u(8)))); ```

This is in the pseudocode

`entropy_coder.cc`, line 52

```cpp for (uint32_t& i : qft) { i = U32Coder::Read(kQFThresholdDist, br) + 1; } ```

notice there's a +1 there.

Section I.2.4 has this code:

```cpp if (encoding_mode == RAW) { params.denominator = F16(); ZeroPadToByte(); params = /* read a 3-channel image from a modular sub-bitstream of the same shape as the required quant matrix */; } ```

That `ZeroPadToByte()` should not be there. If you do it, you'll fail to verify the submodular bitstream for example with ants.jxl

Also, something that appears to missing from Annex I (VarDCT) is an overview. Annex H (modular) had an overview of the general process of modular

VarDCT's annex appears to be missing it

It explains how to do each step, but doesn't explain when to do each step

I now have Quantized LF coefficients and Quantized HF coefficients, but I'm unsure when I dequent them, when I detransform them (via DCT or some other method), and when I decorrelate them.

I know *how* to do those things, but not when.

The spec also appears to be lacking guidance on how to handle edge cases, like if image dimensions are odd, or not multiples of 8, etc.

if the image is a multiple of 8x8 it's not hard, but what if it isn't?

then you pretend it is and crop at the very end, I'm sure it's written *somewhere*

maybe it's in the preamble, but I didn't see it in annex I

in either case, a general overview of VarDCT like there is for modular would be helpful

Minor issue, but in section 4.3: Operators, for and and or if is misspell as iff.

"iff" means "if and only if", but you're right, it should be spelled out because not everyone will be familiar with that abbreviation

Ah, well I learned something new.

Also, the examples are really helpful.

that's my fault for being a mathematician 😛

same, mathematicians ^_^

well it's not necessary for modular images so it'd have to be in VarDCT's section

and I didn't see it although I might have glossed over it

I can't see that either...

(and it's especially tricky how it is supposed to behave especially as you mix in extra channels and upsampling, so it really ought to be mentioned somewhere)

extra channels are done in modular so they don't have that crop factor right?

VarDCT is specifically about the exactly three color channels iirc

correct

Section I.2.1, Quantizer Bundle, is missing a factor of `1 << 16` in global_scale

it has `mXDC = m_x_lf_unscaled / (global_scale * quant_lf);`

but it should have `mXDC = (1 << 16) * m_x_lf_unscaled / (global_scale * quant_lf);`

and same for Y and B

..fixing this now

also watch out for taking the reciprocals of `m_foo_lf_unscaled` https://github.com/lifthrasiir/j40/blob/acfe38647a0614e0562f739d4a40d2fc8a397e7f/j40.h#L5136

<@604964375924834314> I'm fixing noise in the spec, can you fix it in the render pipeline code in libjxl?

yep, I suppose it will mostly boil down to making sure that the previous fixes are really applied

I was afraid of exactly that when we started copying code to the rendering pipeline

which makes me wonder how I didn’t catch this

> `ScaleF(N, n) { return 1 / (cos(pi / (2 * N)) * cos(pi / N) * cos(pi / (N / 2))); }`

Section I.6.6

This doesn't make sense as there's no little `n` here

I tried to trace it back to libjxl but it just has a bunch of precomputed LUTs

so I can't figure out what it did

it was actually already fixed in #12, wasn’t it?

(the spec)

I can't see #12, but i can say that the spec as written is off by 1 from libjxl in the other direction

it clamps to 7, not 6, so adding 1 a bit later will overflow

Looks like the spec was correct but libjxl was off by -1, then we fixed libjxl but also did a +1 in the spec, and then the render pipeline code regressed to the off by -1 version so we ended up having -1 in libjxl and +1 in the spec.

For DctSelect types IDENTIFY, DCT2×2, DCT4×4, DCT8×4, DCT4×8, AFV0, AFV1, AFV2, AFV3, the output is equal to the input.

What's Identify?

A typo for IDENTITY, which was renamed to Hornuss between DIS and FDIS because someone remarked that it is not at all an identity transform 🙂

Good catch!

I see, and a search and replace missed it cause it was a typo <:KEKW:643601031040729099>

I fixed the IDENTIFY/Hornuss typo in the spec draft...

for RAW quant weight decoding, the spec says that you use `params / denominator`

but it's actually `1 / (params * denominator)`

(this is before you take the reciprocols of all the weights)

LLF coefficient ScaleF is missing a multiple of `n`

```cpp ScaleF(N, n) { return 1 / (cos(pi / (2 * N)) * cos(pi / N) * cos(pi / (N / 2))); } ```

It *should* say this:

```c++ ScaleF(N, n) { return 1 / (cos(n * pi / (2 * N)) * cos(n * pi / N) * cos(n * pi / (N / 2))); } ```

alternatively it could be relabeled for clarity

```cpp ScaleF(c, b) { inverse_scale_f = cos(b * pi / (2 * c)) * cos(b * pi / c) * cos(2 * b * pi / c); return 1 / inverse_scale_f; } ```

I chose `b` and `c` here because the values that are fed into it lower in that section are `ScaleF(cy, bheight)`

which matches up

I don't like mixing `N` and `n` in the same function, that could get confusing

(section I.6.6, btw)

I think this is what it's supposed to be, at least

it's hard to tell because libjxl just has LUTs

The default value for `x_qm_scale` in the Frame Header is wrong

in the spec it says the default is 3

with a signalling condition of `!all_default and metadata.xyb_encoded and encoding == kVarDCT`

the actual default is `3` iff `metadata.xyb_encoded and encoding == kVarDCT`

and defaults to `2` iff `!metadata.xyb_encoded || encoding == kModular`

so if, for example, you have a reconstructed JPEG with `!all_default && !metadata.xyb_encoded` it should default to 2, not to 3

libjxl has this (converted to pseudocode) ```cpp if (metadata.xyb_encoded && encoding == kVarDCT) { visitor->visit_fields(&x_qm_scale, 3, /*default=*/all_default) visitor->visit_fields(&b_qm_scale, 2, /*default=*/all_default) } else { x_qm_scale = 2; b_qm_scale = 2; } ```

Oh, wow that's confusing

tripped me up when debugging CFL

already figured that one out since it's used in modular

https://github.com/libjxl/libjxl/blob/main/lib/jxl/quant_weights.cc#L238

> Horizontal (vertical) upsampling is defined as follows: the channel width (height) is multiplied by two (but limited to the image dimensions), and every subsampled pixel B with left (top) neighbour A and right (bottom) neighbour C is replaced by two upsampled pixels with values 0.25 * A + 0.75 * B and 0.75 * B + 0.25 * C.

What happens in edge cases? i.e. what happens if hshift == 1 and x=0

I assume mirroring, i.e. A=B for the leftmost pixels and C=B for the rightmost ones. <@179701849576833024> is that how we do it? We should add that to the spec then...

Somewhere the spec says that whenever we access out of bound pixels they are mirrored

I guess that's true, but in many places we explicitly say so too. It's a bit confusing if we sometimes say it explicitly and sometimes we rely on the generic statement.

This should be coefficients, not samples, right?

that's correct, those are dequantized coefficients

I'll change that then to 'coefficient' instead of 'sample', since this could easily be misinterpreted as doing things in the pixel domain, not the dct domain

Also going to reorganize things so LLF goes earlier

Thanks. Putting things in the order you do them is ideal

Namely Decode HF -> Dequant HF -> Recorrelate HF -> Overlay LF to LLF atop HF -> Inverse DCT

considering that LLF is not recorrellated it should go after CfL

Yeah, I'm going to first define DCT, then LLF (since it uses DCT_2D and it's better to avoid forward refs imo), then Coefficients to samples

Updated draft:

oh nice, there's a nicer looking html output

way better than Cambria :)

There's a typo in section I.9.6: `coeffs_8x4(0, 0)` should be `coeffs_4x8(0, 0)`

as the variable is called 4x8 even in the DCT8x4 transform

End of the AFV transform definition

``` samples_4x8 = IDCT_2D(coeffs_4x8); for (iy = 0; iy < 4; iy++) for (ix = 0; ix < 8; ix++) samples(ix, (flip_y == 1 ? 0 : 4) + iy) = samples_4x4(ix, iy); ```

I believe that last samples should say samples4x8

AFV transform definition

> `coeffs_4x4(0, 0) = coefficients(0, 0) − coefficients(0, 1) + coefficients(1, 0);`

I believe this should be `coeffs_4x4(0, 0) = coefficients(0, 0) + coefficients(0, 1) - coefficients(1, 0);`

I haven't double checked against libjxl, but making that change fixes a number of visual artifacts with the corner of AFV

that matches J40: https://github.com/lifthrasiir/j40/blob/216f83ba7a616685f847268f009e8fcb85106f76/j40.h#L6147

good catch, I missed the second half

Section J.4.1 appears to have some links to the wrong sections

section J.4.2 specifies how to calculate distance as a function of `(x, y, cx, cy)`

and section J.4.3 specifies how to calculate weight as a function of distance and sigma

but I think the linked sections are wrong, since the third step specifies the L1 distance in J.4.3

was this something that got shuffled, and the linked sections didn't get updated?

Section F.2, between tables F.6 and F.7, definition of `downsample[i]`:

something got messed up here, idk what

Top of Annex F, link on the right hand side to F.3 doesn't work

It links to `#toc` but the anchor for F.3 is `#toc45`

dunno how that happened

it looks like there *is* a div with an ID of "toc" but it doesn't work, and idk why

using firefox

looks like it's a reserved keyword or something that metanorma is already using for the document toc

renaming it fixed it

Why is sigma being clamped to 1e-4 ?

if it was less than 1e-4, now it is equal to 1e-4, and it's still gonna be less than 0.3

yeah that seems redundant. <@604964375924834314> can we just remove that `sigma = max(1e−4, sigma)` line?

in theory epf_sigma_for_modular could be less than 0.3 but that would just disable epf for a modular frame

so there'd be no point

when you could just disable epf

I’m not the best person to ask about EPF

I think <@179701849576833024> knows more about this

hm, okay, reading the context a bit more, it does seem redundant

oh right I saw a sigma and was thinking splines, but this is epf 🙂

It is entirely possible for things to be redundant

> `inv_sigma = step_multiplier[step] * 4 * (sqrt(0.5) − 1) / sigma;`

just to double check, is this supposed to be negative?

` v = 1 − scaled_distance * inv_sigma;` will always be positive

and there's a check for `(v <= 0)`

EPF section now:

yea, it's specified in J.4.2

What I would propose to change it to:

ah

I think that's more clear

rewrote the downscale[i] thing a bit, there was some misparsing of code blocks going on there but I also didn't like the parentheses and duplication of the range of i

I guess the question is: does `If sigma < 0.3 for a given varblock, the decoder skips all steps on the pixels of that block` also apply in the modular case? If no, then the clamping does make sense and we should clarify that the skipping only happens in the vardct case. If yes, then we should remove the redundant clamping and maybe clarify that "a given varblock" means the whole frame in the modular case, since there is no per-varblock sigma adjustment.

epf_sigma_for_modular is constant and signalled in the frame header

so if epf_sigma_for_modular < 0.3, that's identical to the frame header just disabling epf

it wouldn't make sense for epf_sigma_for_modular to be < 0.3 and have epf enabled

it might even make sense to forbid it

DCT4x4Params doesn't match the spec compared to libjxl

``` // DCT4 (quant_kind 3) static constexpr const QuantEncodingInternal DCT4X4() { return QuantEncodingInternal::DCT4(DctQuantWeightParams({{{{ V(2200.0), V(0.0), V(0.0), V(0.0), }}, {{ V(392.0), V(0.0), V(0.0), V(0.0), }}, {{ V(112.0), V(-0.25), V(-0.25), V(-0.5), }}}}, 4),```

libjxl

spec

``` The resulting quant is then multiplied by a per-block multiplier, the value of HfMul at the coordinates of the 8 × 8 rectangle containing the current sample, and, for the X and B channels, by pow(0.8, frame_header.x_qm_scale − 2) and pow(0.8, frame_header.b_qm_scale — 2), respectively. ```

This is not correct

It is multiplied by `(1 << 16) * quantizer.global_scale / HfMul`

it is not multiplied by `HfMul`

see line 145 of dec_group.cc

Section E.4.2:

``` value = 0; shift = 0; while (1) { b = u(8); value += (b & 127) << shift; if (b <= 127) break; shift += 7; /* shift < 63 */; } ```

I'm looking at this comment `/* shift < 63 */`

it's not clear to me if this is a requirement before or after adding 7

because it's on the same line

it says earlier that the final integer is at at most 63 bits, so I think it's still technically specified, but it might be clearer if the pseudocode reflected that

basically value can use uint64_t

Maybe better to write `/* shift <= 56 */` in the beginning of the while body...

that would be more obvious, yea

Section E.4.4

> `/* output nothing, stop reading the tag list, and proceed to ~\ref{iccmaincontent}~ */`

looks like you left some `\LaTeX` in there

oops, lol

``` f (tagcode == 1) { // the tag string is set to 4 custom bytes read from the data tag = u(4 * 8) from data stream; } else if (tagcode == 2) { tag = “rRTC”; } else if (tagcode == 3) { tag = “rXYZ”; ```

is `rRTC` supposed to be `rTRC`?

Uh, best to check libjxl code

later on it has the following

``` /* append the following 3 groups of 4 bytes to the output: tag, tagstart, tagsize */ if (tagcode == 2) { /* append the following additional 6 groups of 4 bytes to the output: “gTRC”, tagstart, tagsize, “bTRC”, tagstart, tagsize */ ```

I'm guessing TRC stands for Transfer Characteristics

and rRTC is probably a typo

https://github.com/libjxl/libjxl/blob/9d960611774b5b14a46f33a20768cde2cd0c219f/lib/jxl/icc_codec_common.h#L48

looks like it is a typo

Good catch. Indeed a typo for rTRC

> `In this subclause, Shuffle(bytes, width) denotes the following operation: Bytes are inserted in raster order (row by row from left to right, starting with the top row) into a matrix with width rows, and as many columns as needed.`

Why is it called `width` if the number of columns is arbitrary, and there's that many rows?

usually the word `height` is used to describe the number of scanlines, i.e. the number of rows.

How am I supposed to insert bytes in raster order into a matrix with as many columns as needed?

oh, I see. I create a matrix with `width` rows and `height` columns (yes, this is backward), because I then transpose it later

but that's very confusing

and `height` in this case is `CeilDiv(bytes.size(), width)`

Section E.4.5

> ` val = (bytes[i + j] + (p >> (width − 1 − j))) & 255;`

`p >> (width - 1 - j)` should be `p >> (8 * (width - 1 - j))`

as `width` and `j` are in units of bytes, not bits

Section J.4.2:

``` DistanceStep2(x, y, cx, cy) { dist = 0; for (c = 0; c < 3; c++) { dist += abs(sample(x + ix, y + iy, c) − sample(x + cx + ix, y + cy + iy, c)) * rf.epf_channel_scale[c]; } return dist; } ``` `iy` and `ix` are not defined here

is this a copy/paste typo?

Section I.8, ScaleF function

this is my fault, I flipped `c` and `b`

`c` should be in the numerator, and `b` in the denominator

Section I.6, Chroma From Luma:

```ytox_dc_ = static_cast<int>(br->ReadFixedBits<kBitsPerByte>()) + std::numeric_limits<int8_t>::min();```

spec says -127, but this is -128

since `signed char` goes from -128 to +127

this makes a noticeable difference if the codestream encodes 128

since it goes from 1 to 0

Section G.3:

These are in the wrong order

the correct order is `lf_coeff` *then* `mlf_group` *then* `hf_meta`

See `dec_frame.cc` line 301:

```cpp Status FrameDecoder::ProcessDCGroup(size_t dc_group_id, BitReader* br) { PROFILER_FUNC; const size_t gx = dc_group_id % frame_dim_.xsize_dc_groups; const size_t gy = dc_group_id / frame_dim_.xsize_dc_groups; const LoopFilter& lf = dec_state_->shared->frame_header.loop_filter; if (frame_header_.encoding == FrameEncoding::kVarDCT && !(frame_header_.flags & FrameHeader::kUseDcFrame)) { JXL_RETURN_IF_ERROR( modular_frame_decoder_.DecodeVarDCTDC(dc_group_id, br, dec_state_)); } const Rect mrect(gx * frame_dim_.dc_group_dim, gy * frame_dim_.dc_group_dim, frame_dim_.dc_group_dim, frame_dim_.dc_group_dim); JXL_RETURN_IF_ERROR(modular_frame_decoder_.DecodeGroup( mrect, br, 3, 1000, ModularStreamId::ModularDC(dc_group_id), /*zerofill=*/false, nullptr, nullptr, /*allow_truncated=*/false)); if (frame_header_.encoding == FrameEncoding::kVarDCT) { JXL_RETURN_IF_ERROR( modular_frame_decoder_.DecodeAcMetadata(dc_group_id, br, dec_state_)); } else if (lf.epf_iters > 0) { FillImage(kInvSigmaNum / lf.epf_sigma_for_modular, &dec_state_->sigma); } decoded_dc_groups_[dc_group_id] = uint8_t{true}; return true; } ```

notice that DecodeVarDCTDC is called before DecodeGroup

<@179701849576833024> I suppose there is something missing there? In Step0/1 the ix,iy iterate over 5 values making a cross, I assume it's not the same in Step2 because then why would there be two different functions, but is it just ix=iy=0 or is it also iterating over some set of offsets?

No just 0 0

also ping <@179701849576833024> on this one

It does apply to modular too

Latest draft:

oh, nice

Section I.2.3, the default values for `x_factor_lf` and `b_factor_lf` are 128, not 127

same line of code as here, default coded value is 0

Today's draft:

updated draft:

I changed the signalling of `save_before_ct` quite a bit, I hope it is correct and easier to understand now

much easier to read now

Just want to say than you for the excellent work <@853026420792360980> .

yw :)

<@853026420792360980> and <@268284145820631040> you could do me a big favor by checking if all the bugs you found are fixed now (and pointing me to the ones that still need fixing or where the fix is wrong/not good enough)

I can take a look

Current draft: (clarified bit depth, save_before_ct, and hshift/vshift in modular)

Section G.2.2: > If the kUseLfFrame flag in frame_header is set, this subclause is skipped and the samples from LFFrame[frame_header.lf_frame] are used instead of the values that would be computed by this subclause and I.5.2.

`frame_header.lf_frame` is not defined in the FrameHeader bundle

is this the same `lf_level` used?

does that mean `kUseLFFrame != 0` and `lf_level > 1` means you have an LF Frame which itself uses an LF Frame for its LF Coefficients?

Section I.4:

> To compute context, the decoder uses the procedures and constants in the following code, where c is the current channel (with 0=X, 1=Y, 2=B), s is the Order ID (see Table I.7) of the DctSelect value and qf is the HfMul value for the current varblock, and **qdc[3] are the quantized LF values** of (the top-left 8×8 block within) the current varblock (taking into account chroma subsampling if needed). The lists of thresholds qf_thresholds and lf_thresholds[3], and block_ctx_map are as decoded in LfGlobal ( G.1.1 and I.2.2).

specifically, these are the *quantized* LF Values

what happens if you have an LF Frame? which skips dequantization

especially considering the LF Frame values are floats

and the quantized LF Values are ints

that's a really good question — <@179701849576833024> any idea what libjxl will do here? just have all zeroes there or something? we need to put it in the spec...

I tried populating the quantized values and I got some incorrect values

I tried populating zeroes and got other incorrect values

lenna-correct.jxl

(converted to png with libjxl for discord)

using the dequantized LF values produced this

using zeroes produced this

those are varblock boundaries, btw

those seams

weirdly enough, both ANS streams verified

which I'm somewhat surprised about

I would have expected one to not verify and for that to just give it away

Yes, you can have recursive LF frames, up to 4 levels deep. So if you have a max-size image of 2^30 by 2^30 pixels (about one gigapixel by one gigapixel or one exapixel), you can have its 2^27 x 2^27 DC in LFFrame[0] which is a 1:8 image, which can have _its_ 2^24 x 2^24 DC in LFFrame[1] which is a 1:64 image, which can have _its_ 2^21 x 2^21 DC in LFFrame[2], which is a 1:512 image, which can have _its_ 2^18 x 2^18 DC in LFFrame[3], which is a 1:4096 image. That image needs to be modular or VarDCT with its own DC — in the latter case its DC would be a 2^15 x 2^15 downscale at 1:32768 of the full image, which would still be a 1 gigapixel image that would be encoded in 16384 DC groups.

I see, so `lf_level > 0` is strictly for LF Frames themselves using LF Frames

this was not clear to me

either way, the `lf_frame` is not defined, it should be `lf_level`

wait how did you get a bitstream that uses qdc in combination with LF frames?

because it's used in the HF Coefficient decoding

yeah but libjxl only uses that when recompressing jpegs and then it doesn't produce lf frames

that's not what the spec says

I mean

and also not what it does

see section I.4

it sets the thresholds so that it effectively doesn't use that

`BlockContext()` uses `qdc`

and `blockContext()` is needed to determine which context to read the HF coefficient from

libjxl will set `nb_lf_thr` to zero for each channel when doing vardct-from-pixels

so it effectively doesn't matter what the value of qdc is to determine the context

that's probably why both ANS streams verified

but they produced different results

which is odd

yeah that is odd, could be another spec bug or a jxlatte bug

oh yea, the number of LF Thresholds is empty

I see. I have a race condition somewhere because I'm getting different results on subsequent executions

I have to debug

yeah and the only time they will not be empty is with recompressed jpegs — we needed some extra context there to help with compression, and quantized DC was somewhat useful; for 'full' VarDCT images we can use the adaptive quant weights which are a much more useful piece of context (but not for recompressed jpegs since those obviously don't do real adaptive quantization)

but we never made lf frames work for recompressed jpegs so we never encountered the issue of what to do with `qdc` in that case

The real answer is that I don't remember, but I thought all 0...

https://github.com/libjxl/libjxl/blob/89926120c9e7aea0483c17be70570a7448609da5/lib/jxl/passes_state.cc#L60

just checked, all zero indeed

sure, but the spec doesn't explicitly say that the number of LF Thresholds must be zero if `kUseLFFrame != 0`

if this is always the case it should say it

yeah either we do that or we just say that quantized lf coeffs are to be considered zero if an lf frame was used

that works too

Wait, what libjxl has there are the indices it will use to add to the block ctx. Setting those to zero is effectively the same thing as pretending all qdc values are -inf

(so they are always in the first bucket of every lf threshold bucket, regardless of what the threshold values are)

Probably better to just say the nb of thresholds is zero in this case (and check it in libjxl too, I guess)

turns out the bug was in my inverse vertical squeeze

I was iterating over each row, and then each column, as usual, rather than iterating over each column and then each row

which matters as each vertical strip references the previous pixel in the column

i.e. at higher rows

so you need to do the rows in order and parallelize over the columns

after that, it's working properly

Nice, and yes, vertical unsqueeze is a bit tricky to parallelize

in my case I just swapped the order of the y and x

basically I have a function which runs one pixel and then I have a loop that iterates over the row

and then I run that row loop on the pool

in this case I just changed it so I iterate over the x coordinate on the outside, then run on pool, and then iterate over y coordinate on the inside

Yeah that works, you can simd it too but that's where it gets trickier

it's decoding correctly now except for this artifact

which looks like some sort of X channel overflow. but that's likely a jxlatte bug, not a spec bug.

that works but destroys your cache/memory bandwidth efficiency

I don't really have any in this case either way

I'd have to transpose, squeeze horizontally, and transpose back for maximum efficiency

but I agree that vertical squeeze is hard to optimize

Today's spec draft:

I'll take a look at it this afternoon

DCT-II is the forward DCT and DCT-III is the inverse DCT. Is this intentional?

I mean the pseudocode/formulas specified for the DCTs works but are these names accurate?

probably not

I don't remember when is the last time we checked those definitions, but a long time ago

Section J.2, JPEG Upsampling, doesn't specify what happens in corner cases. I'm guessing it uses `Mirror` (section 5.2) like every other restoration/upsampling filter, but this isn't explicit.

Section C.1: libjxl disallows specific nested-nested-nested etc. distributions from using LZ77 because it can become a zip bomb

the spec does not

I don't know if the spec should be disallowing these as well