PR #25847 opened by luoye-cpu
URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25847
Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25847.patch
The AV1 decoder exports film grain parameters for hardware-accelerated
output as well, but the auto-regression coefficients are unbiased
unconditionally, so entries that were never coded are exported as -128.
# Summary of changes
**What happens now.** `export_film_grain()` subtracts the coding bias
from the auto-regression coefficients unconditionally: all 24 luma
entries (`ar_coeffs_y`) and all 25 chroma entries per plane
(`ar_coeffs_uv[0]`, `ar_coeffs_uv[1]`) are converted as if they had
been coded.
The bitstream codes them conditionally. `cbs_av1_syntax_template.c`
reads the luma AR coefficients only when `num_y_points` is non-zero,
and the chroma coefficients only when `chroma_scaling_from_luma` is
set or the respective plane has grain points (`num_cb_points` /
`num_cr_points`); even then only the first
`2 * ar_coeff_lag * (ar_coeff_lag + 1)` entries — plus one luma
coefficient for chroma when `num_y_points` is non-zero — are read.
The parsed structure is zero-allocated, so entries that were never
read stay at zero.
Subtracting the bias anyway turns those zeroes into -128, so the
exported side data carries -128 in coefficients the stream never
described. libdav1d memcpy's the parsed dav1d structure, which zeroes
unparsed fields (dav1d 1.5.4 parses the AR coefficients under the
same conditional guards), so for the same stream libdav1d exports 0
where av1dec exports -128 — the same decoder-to-decoder disagreement
that 2a8c6d30b9 ("avcodec/av1dec: remove the coding bias from
exported chroma grain params") set out to remove for the chroma
multipliers.
**Scope of the fix: representation, not synthesis.**
`AVFilmGrainAOMParams` documents the effective coefficient counts,
and they are derivable from `ar_coeff_lag` and the point counts; the
built-in synthesizer (`aom_film_grain_template.c`) only consumes
those entries, so FFmpeg's film grain synthesis is unchanged by this
patch. What the patch fixes is the side data representation itself:
entries that were never coded now keep a deterministic zero value —
what an unparsed field means, and what libdav1d reports for the same
stream — instead of a -128 bias artifact. External consumers are not
required to re-derive which entries are valid.
**The patch.** Each of the three conversions is guarded on the same
conditions cbs uses, one to one: the luma loop under
`film_grain->num_y_points`, the Cb loop under
`film_grain->chroma_scaling_from_luma || film_grain->num_cb_points`,
and the Cr loop under `film_grain->chroma_scaling_from_luma ||
film_grain->num_cr_points`. The loop bounds `num_pos_luma` and
`num_pos_chroma` mirror the syntax counts. Skipping the assignments
keeps the fields at zero.
Upstream context: #25834 ("avcodec/av1dec: do not subtract the chroma
coding bias without points") covers the six chroma
multipliers/offsets in this same function; this patch covers the
three AR coefficient arrays, which have different syntax conditions
(they can be coded when chroma scaling comes from luma, and the coded
coefficient count matters). The automated review of #25834
independently recorded exactly this issue as a pre-existing,
non-blocking finding — "unused AR coefficients still differ between
exporters" (av1dec.c:1126 / cbs_av1_syntax_template.c:1290) — and
noted that keeping that normalization separate is reasonable,
explicitly not requesting expansion of that PR. The two patches touch
adjacent, non-overlapping regions of the same function and are
complementary; if #25834 merges first, this patch needs only a simple
rebase (verified: auto-merge succeeds on a tree carrying both).
## Testing
Verified on NVIDIA GeForce RTX 4060 (CUDA device 0) with a film-grain
AV1 stream (320x240, 4:2:0 8-bit, ar_coeff_lag=2, 8 frames decoded
through the NVDEC hardware path and the shared export path):
| array | segment | before | after |
|---|---|---|---|
| ar_coeffs_y[0..11] | coded | 1,5,9,...,45 | unchanged |
| ar_coeffs_y[12..23] | uncoded tail | -128 (12 entries) | **0** |
| ar_coeffs_uv[0][0..12] (cb) | coded | -17,-16,...,-5 | unchanged |
| ar_coeffs_uv[0][13..24] | uncoded tail | -128 (12 entries) | **0** |
| ar_coeffs_uv[1][0..12] (cr) | coded | 23,24,...,35 | unchanged |
| ar_coeffs_uv[1][13..24] | uncoded tail | -128 (12 entries) | **0** |
- The coded segments are bit-exact before and after; only the uncoded
entries change (-128 → 0). Per frame, `FILM_GRAIN_COUNT=1`, the
grain seeds and all other AOM parameters are unchanged, and
`av_film_grain_params_select()` behaves identically (8 frames,
every export-path shape checked, a single unique form).
- Cross-decoder consistency: for the same stream libdav1d reports the
same coded segments and zeroes for the tails — exactly what this
patch makes av1dec export.
- `git apply --check` and `git am` pass on the current master tip
(+25/−5, `libavcodec/av1dec.c` only); `tools/patcheck` reports no
blocking issues.
## API
No public API change: `AVFilmGrainAOMParams` is unchanged; the
exported side data merely stops reporting the -128 bias artifact in
entries that were never coded, matching what libdav1d already reports
for the same streams.
No new FATE samples are required.
Based on master `1ade940d09`.
From fef21aedca86bbae9ad4f68b2c84675eeb54164a Mon Sep 17 00:00:00 2001
From: luoye-cpu <[email protected]>
Date: Sat, 7 Nov 2026 18:46:20 +0800
Subject: [PATCH] avcodec/av1dec: do not unbias absent film grain AR
coefficients
export_film_grain() subtracts the coding bias from the auto-regression
coefficients unconditionally: all 24 luma and all 25 chroma entries are
converted as if they had been coded. The bitstream codes them
conditionally: cbs_av1_syntax_template.c reads the luma coefficients
only when num_y_points is non-zero, and the chroma coefficients only
when chroma_scaling_from_luma is set or the respective plane has grain
points; even then only the first 2 * ar_coeff_lag * (ar_coeff_lag + 1)
(+ one luma coefficient for chroma when num_y_points is non-zero)
entries are read. Unread entries stay at zero because the parsed
structure is zero-allocated.
Subtracting the bias anyway turns those zeroes into -128, so the
exported side data carries -128 in coefficients the stream never
described. libdav1d memcpy's the parsed dav1d structure, which zeroes
unparsed fields, so for the same stream libdav1d exports 0 where av1dec
exports -128 - the same decoder-to-decoder disagreement that
2a8c6d30b9 ("avcodec/av1dec: remove the coding bias from exported
chroma grain params") set out to remove for the chroma multipliers.
AVFilmGrainAOMParams documents the effective coefficient counts and the
built-in synthesizer in aom_film_grain_template.c only consumes those
entries, so the fix is invisible to it; but the side data is a public
contract and external consumers are not required to re-derive which
entries are valid.
Guard each of the three conversions on the same conditions cbs uses and
convert only the signalled entries, leaving the rest at zero.
Signed-off-by: luoye-cpu <[email protected]>
---
libavcodec/av1dec.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
index 315828e18a4..f450281391d 100644
--- a/libavcodec/av1dec.c
+++ b/libavcodec/av1dec.c
@@ -1120,12 +1120,32 @@ static int export_film_grain(AVCodecContext *avctx,
AVFrame *frame)
aom->uv_points[1][i][1] = film_grain->point_cr_scaling[i];
}
- for (int i = 0; i < 24; i++) {
- aom->ar_coeffs_y[i] = film_grain->ar_coeffs_y_plus_128[i] - 128;
+ /* The AR coefficients are conditionally coded: cbs only reads the luma
+ * coefficients when num_y_points is non-zero, and the chroma ones only
+ * when chroma_scaling_from_luma is set or the respective plane has grain
+ * points. Only the first 2 * ar_coeff_lag * (ar_coeff_lag + 1) (plus
+ * one luma coefficient for chroma when num_y_points is non-zero)
+ * entries are signalled, the rest stay zero. Unbias only the entries
+ * which were actually coded so that absent coefficients are exported
+ * as zero, as libdav1d does for the same stream, instead of turning
+ * into the -128 bias artefact. */
+ const int num_pos_luma =
+ 2 * film_grain->ar_coeff_lag * (film_grain->ar_coeff_lag + 1);
+ const int num_pos_chroma = num_pos_luma + !!film_grain->num_y_points;
+
+ if (film_grain->num_y_points) {
+ for (int i = 0; i < num_pos_luma; i++)
+ aom->ar_coeffs_y[i] = film_grain->ar_coeffs_y_plus_128[i] - 128;
}
- for (int i = 0; i < 25; i++) {
- aom->ar_coeffs_uv[0][i] = film_grain->ar_coeffs_cb_plus_128[i] - 128;
- aom->ar_coeffs_uv[1][i] = film_grain->ar_coeffs_cr_plus_128[i] - 128;
+ if (film_grain->chroma_scaling_from_luma || film_grain->num_cb_points) {
+ for (int i = 0; i < num_pos_chroma; i++)
+ aom->ar_coeffs_uv[0][i] =
+ film_grain->ar_coeffs_cb_plus_128[i] - 128;
+ }
+ if (film_grain->chroma_scaling_from_luma || film_grain->num_cr_points) {
+ for (int i = 0; i < num_pos_chroma; i++)
+ aom->ar_coeffs_uv[1][i] =
+ film_grain->ar_coeffs_cr_plus_128[i] - 128;
}
aom->uv_mult[0] = film_grain->cb_mult - 128;
--
2.54.0
_______________________________________________
ffmpeg-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
