Repository navigation
feat(gamut-jpeg): encode 4:4:0 chroma subsampling - #1028
Merged
Merged
Conversation
Add sampling_factors(), which reads each component's (Hi, Vi) with libjpeg-turbo's own header parser, so a gamut-jpeg test can check the grid the reference decoder upsamples by without trusting gamut's marker parser. Add Subsampling::S440 (luma 1x2) so the oracle can produce the 4:4:0 streams the decode-direction batteries need. Part of #1014
Add ChromaSubsampling::Ycbcr440: luma sampling factors 1x2 and chroma 1x1, so Cb/Cr are halved vertically only. T.81 Table B.2 allows any factor from 1 to 4, and the grid's four data units per MCU are within the B.2.3 limit of ten. The encoder already handled general H/V factors in its scan, progressive and downsample paths, so the change is the mapping alone. JpegStreamInfo::subsampling() now reports a (1,2),(1,1),(1,1) frame as Some(Ycbcr440) where it used to report None. A pushed decode backend that keyed on None to read sampling_factors() will now get the named grid instead. 4:4:0 joins every oracle battery through Mode::ALL. With it included, no measured worst case moves. Two new gates cover every colour mode: - libjpeg-turbo's header parser reads back the declared factors, for both baseline and progressive. - libjpeg-turbo's decode of gamut's stream stays within 2 dB PSNR of its decode of its own encode at equal quality. Measured worst: 4:2:2 1.33 dB; 4:4:0 0.41 dB. Part of #1014
Map the new value to gamut-jpeg's ChromaSubsampling::Ycbcr440, so the sandbox can produce 4:4:0 JPEGs, and pin all four values (and the refusal of an unknown one) in an inline test. Part of #1014
…upsampling The link to the private decoder module fails rustdoc's private-intra-doc-links lint under the workspace doc gate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The gamut-jpeg encoder can now write 4:4:0 YCbCr JPEGs: luma sampling factors
1×2, chroma1×1, so Cb/Cr are halved vertically only. This lets gamut-jpeg join encoder sweeps that cover 4:4:4 / 4:2:2 / 4:4:0 / 4:2:0, the modes libjpeg-turbo offers. T.81 Table B.2 allows any factor from 1 to 4. This grid uses four data units per MCU, within the §B.2.3 limit of ten. The encoder's scan, progressive and downsample paths already handled general H/V factors, so the codec change is only the mapping.Changed paths:
crates/gamut-jpeg/src/encoder.rs: newChromaSubsampling::Ycbcr440variant, appended last, mapped to luma(1, 2). Its doc cites T.81 B.2.2/B.2.3 and T.871 §9 NOTE 3 (JFIF discourages forms other than 4:2:0).crates/gamut-jpeg/src/backend.rs:JpegStreamInfo::subsampling()now names a(1,2),(1,1),(1,1)frameSome(Ycbcr440); it used to returnNone. Its unit test asserts the new arm and keeps(2,4)as the example of an unnamed grid.crates/gamut-jpeg/src/lib.rs,README.md,STATUS.md: list 4:4:0 among the encoder's subsampling modes. STATUS records the two new oracle gates and their measured figures, and the decode battery now includes 4:4:0.crates/gamut-jpeg/tests/oracle.rs:Mode::C440joinsMode::ALL, so 4:4:0 now runs through every encode and decode battery: the libjpeg-turbo decode battery, progressive and optimized-table exactness, the size ratio against libjpeg-turbooptimize_coding, and decode parity on libjpeg-turbo's 4:4:0 streams.encode_cross_parity_gamut_vs_libjpeg.encode_sampling_factors_are_what_libjpeg_turbo_readsandencode_fidelity_tracks_libjpeg_turbo_own_encode.crates/gamut-jpeg/tests/encode.rs,tests/progressive.rs: 4:4:0 joins the dimension/restart battery and the progressive==baseline battery.tooling/libjpeg-oracle/src/shim.c,src/lib.rs:sampling_factors(), a header-only read of each component's(Hi, Vi)by libjpeg-turbo's own parser, so the test does not rely on gamut's marker parser.Subsampling::S440.sampling_factors_report_each_mode_and_gray.crates/gamut-cli/src/commands/convert.rs,crates/gamut-cli/README.md:--jpeg-subsampling 440, with an inline test pinning all four values and the refusal of an unknown one.How the acceptance criteria are met
Decodes through libjpeg-turbo with the expected sampling factors.
encode_sampling_factors_are_what_libjpeg_turbo_readsreads gamut's baseline and progressive streams with libjpeg-turbo'sjpeg_read_header. It asserts[(1,2),(1,1),(1,1)]for 4:4:0 and the right grid for every other mode.encode_battery_decoded_by_libjpeg_turboasserts libjpeg-turbo decodes every 4:4:0 stream at the right geometry across dims × q{50,75,90} × restart{0,3}.Same tolerance against libjpeg-turbo's own encode at equal quality as the existing modes.
encode_fidelity_tracks_libjpeg_turbo_own_encodechecks every colour mode against one bound. libjpeg-turbo decodes gamut's stream and its own stream of the same source at the same quality. gamut's PSNR may trail libjpeg-turbo's by at most 2 dB.With
optimize_coding, the existing size gate puts 4:4:0 at a 0.961–0.970 byte ratio against libjpeg-turbo, under the shared 1.15 bound.Adding 4:4:0 to the existing batteries moved none of their measured worst cases: 30.11 dB, 34.18 dB, 22.90 dB, max-diff 11/5, size ratio 1.083. In cross parity, 4:4:0's worst is 38.23 dB against the 35.22 dB 4:2:0 figure the bound cites. (Superseded at
d37a1048: after merging #1016's libjpeg-turbo fancy upsampling frommaster, cross parity is exact, max-diff 0, in every mode including 4:4:0; see decision 8.)Validation
GitHub CI workflows are disabled on this repository, so no CI ran. Every result below is a local run at
cdc5023b(rebased onorigin/master1f421090) unless noted.mise run fmt-check: passmise run fmt-tooling-check: passmise run lint(cargo clippy --workspace --all-targets --all-features -- -D warnings): passcargo clippy --all-targets -- -D warningsintooling/libjpeg-oracle: passcargo test -p gamut-jpeg -p gamut -p gamut-cli -p gamut-ffi -p gamut-wasm --all-features: pass, 515 tests across 26 binaries. These are gamut-jpeg plus every crate that depends on it, directly or transitively, percargo metadata. This replaces a workspace-widemise run test, which was not run.cargo testintooling/libjpeg-oracle: pass, 9 tests (ran on the pre-rebase tree, same oracle sources)mise run mutants-diff(tooling/mutants/run.sh --diff, memory-capped): 7 mutants, 6 caught, 1 unviable, 0 missed, exit 0.luma_factorswhole-body replacements,JpegStreamInfo::subsampling→None, and the deleted(1, 2)arm.subsampling→Some(Default::default()).tooling/are excluded from mutation by config.cargo llvm-cov -p gamut-jpeg --all-features --ignore-filename-regex '(crates/gamut-(cli|wasm|ffi|jxl-sys)|tooling)/' --fail-under-lines 80: pass, 98.22% lines overall.encoder.rs99.48% lines,backend.rs98.03%.mise run check-tests,mise run check-commits,mise run check-agents-edges: passCoverage gaps
(1, 2)literal insideluma_factorscannot be mutated, a blind spot AGENTS.md describes. It is held by the oracle sampling-factor test and the batteries, not by the mutation gate.tests/encode.rs, and the progressive restart path is shared with 4:2:0.n > cap) is unreachable, because libjpeg rejects more than 10 components and the cap is 10.Ycbcr440to a pushed encode backend throughJpegEncodeRequest::subsampling(). The value is passed through unchanged, andtests/backends.rscovers 4:2:0 and 4:2:2.encode_fidelity_tracks_libjpeg_turbo_own_encodeuses the smooth gradient only, at baseline, and its 2 dB bound is about 5× the 0.41 dB 4:4:0 gap. It meets the issue's "same tolerance as existing modes" wording, but would not see a 4:4:0-specific chroma loss under about 1.5 dB. What pins the 4:4:0 grid itself is the sampling-factor test plus the exactness batteries.Risks and rollout
JpegStreamInfo::subsampling()changes behaviour for 4:4:0 frames: it returnsSome(Ycbcr440)where it returnedNone. A pushed decode backend that readNoneas "exotic, consultsampling_factors()" now gets the named grid.ChromaSubsamplingis#[non_exhaustive], so adding a variant is not a SemVer break, and thefeatcommit body records the change.AGENTS.md(line 73) still lists the encoder as "gray + YCbCr 4:4:4/4:2:2/4:2:0". That file is the agent instructions file, which this run is not permitted to edit, so the one-line fix is left for a maintainer.Review notes
Two independent read-only reviews ran: one before the commits and one at
cdc5023b. Neither found a Critical, High or Medium defect. The first review's Low items were fixed before the push: a stale shim doc, stale battery comments, 4:4:0 added to cross parity, a doc reflow, an unsourced jpegli claim replaced byTJSAMP_440, and an append-only doc sentence the type did not enforce. Remaining review questions and their answers:Mode::ALL? Yes. Every battery was re-measured with 4:4:0 included, and none of the quoted worst cases moved: 30.11 / 34.18 / 22.90 dB, max-diff 11 / 5, size ratio 1.083. Only the encode battery's comment and cross parity (where 4:4:0's figure is added) were annotated; the other comments stay true as written.Mode::ALL. It costs no code, and STATUS records it.tests/oracle.rs,shim.c): cosmetic, and rustfmt passes them. Left as is.Decisions taken
These were decided without a human because the maintainer delegated public-API shape choices. Each entry gives the edit that reverses it.
Variant name and position. Chose
ChromaSubsampling::Ycbcr440, appended afterYcbcr420. It follows the existingJ:a:bnaming. libjpeg-turbo calls the same gridTJSAMP_440, which the doc cites. RejectedYcbcr422Verticaland similar, because they break the naming scheme. To reverse: rename the variant.No
repror explicit discriminants added. The enum had neither before this change, and no C surface consumes it: gamut-ffi's JPEG module exposes only backend registries. Adding arepris an ABI-shape change outside gamut-jpeg: 4:4:0 chroma subsampling on encode #1014. The first review also flagged a doc sentence promising "append-only" variants that the type does not enforce, and that sentence was removed. To reverse: add#[repr(u8)]withYcbcr444 = 0 … Ycbcr440 = 3in a separate change.JpegStreamInfo::subsampling()names the new grid. The alternative, keepingNonefor(1,2), would make the encoder emit a grid that its own stream-info accessor calls unnamed. To reverse: delete the(1, 2)arm inbackend.rs.CLI value included.
--jpeg-subsampling 440is a small, direct consumer of the new variant, and gamut-cli is the sandbox that exercises implemented features. It is in its own commit (feat(gamut-cli)) so it can be dropped without touching the codec. To reverse: revert that commit.Fidelity tolerance as one PSNR-gap bound over every colour mode. The issue asks for "the same tolerance … that the existing modes meet", and no existing test compared gamut against libjpeg-turbo's own encode by pixels; only by size, under
optimize_coding. The new test applies one 2 dB bound to all four modes, so 4:4:0 meets the same bar 4:4:4, 4:2:2 and 4:2:0 do, measured above. To reverse: change the bound inencode_fidelity_tracks_libjpeg_turbo_own_encode.Oracle gains a sampling-factor reader. Gamut's own
info()could have checked the factors, but that is gamut checking itself. The issue says "decodes through libjpeg-turbo with the expected sampling factors", so the factors are read by libjpeg-turbo's header parser. To reverse: dropsampling_factorsand assert withgamut_jpeg::info.AGENTS.md corrected in this pull request rather than filed. The repository's doc-authority rule asks that a document contradicting its crate be corrected in the same change that notices it, or filed. Correcting it is a one-token edit, so
AGENTS.mdline 73 now lists the encoder as "gray + YCbCr 4:4:4/4:2:2/4:2:0/4:4:0" (commitaf70b0da). This supersedes the Risks bullet above that leaves the line for a maintainer. To reverse: revert that commit and file an issue instead.Merge conflict with
master(gamut-jpeg, gamut-webp, gamut-avif: decoders upsample chroma by sample replication, diverging from reference decoders #1016) resolved toward master's exact-parity assertions. gamut-jpeg, gamut-webp, gamut-avif: decoders upsample chroma by sample replication, diverging from reference decoders #1016 made the decoder's default chroma upsampling byte-identical to libjpeg-turbo, replacing theupsampling_matchessplit withfull_chromaand turningencode_cross_parity_gamut_vs_libjpeginto one exactmax-diff 0assertion overMode::ALL. The merge (d37a1048) keeps that form, so 4:4:0 now runs under the exact assertion rather than a PSNR floor; this branch'ssampling_factorshelper is kept besidefull_chroma.STATUS.mdkeeps master's wording with 4:4:0 added to the progressive battery list.cargo test -p gamut-jpeg(including the 20 libjpeg-turbo oracle tests) passes at the merged head. To reverse: none needed; the old PSNR floor is weaker than what master now asserts.Clippy & Doctestsfailure repaired here, though master introduced it. Thedoctask (RUSTDOCFLAGS=-D warnings) failed withrustdoc::private_intra_doc_links: theJpegDecoder::with_chroma_upsamplingdoc linked[module docs](self), and thedecodermodule is private. The line came from feat(gamut-color, gamut-jpeg, gamut-webp, gamut-avif)!: upsample chroma like each reference decoder by default #1044 (452aa2ae) and is identical onmaster, so this pull request did not cause it, but it blocks this merge. The repair (45d6dd2d) drops the parenthetical link.ChromaUpsampling::Interpolated, already linked in the same sentence, carries the explanation.crates/gamut-jpeg/src/decoder.rsis the only path this touches.GAMUT_JXL_SYS_SKIP_NATIVE=1 mise run doc,mise run fmt-checkandmise run lint-quickpass at that head. To reverse: revert45d6dd2donce master fixes the link on its own.No human approved this plan beforehand; it is recorded here for review.
Unresolved review notes
C1
AGENTS.md:73still listed the gamut-jpeg encoder as "gray + YCbCr 4:4:4/4:2:2/4:2:0" though this pull request shipsYcbcr440. Repaired: the entry now reads "4:4:4/4:2:2/4:2:0/4:4:0". A repository-wide search found no other stale encoder-mode list for gamut-jpeg.mise run check-agents-edgespasses (32 crates, 131 edges) andmise run check-commitspasses.Resolved at: af70b0d
Issue
Closes #1014