[FFmpeg-devel] [PATCH 4/6] lavc/vvc: Fix scaling matrix DC coef derivation

[Nuo Mi]

Hi Frank,
Thank you for the detail
Applied.

On Sat, Nov 30, 2024 at 6:11 PM Frank Plowman <post at frankplowman.com> wrote:

> Hi,
>
> Thank you very much for the review.  Responses inline.
>
> On 30/11/2024 06:39, Nuo Mi wrote:
> > Hi Frank,
> > Thank you for the patch set; I will apply it except for this one
> >
> > On Fri, Nov 29, 2024 at 6:19 AM Frank Plowman <post at frankplowman.com>
> wrote:
> >
> >> In 7.4.3.20 of H.266 (V3), there are two similarly-named variables:
> >> scalingMatrixDcPred and ScalingMatrixDcRec.  The old code set
> >> ScalingMatrixDcRec, rather than scalingMatrixDcPred, in the first two
> >> branches of the conditions on scaling_list_copy_mode_flag[id] and
> >> aps->scaling_list_pred_mode_flag[id].  This could lead to decode
> >> mismatches in sequences with explicitly-signalled scaling lists.
> >>
> > dc is scaling_list_dc_coef, and scalingMatrixDcPred is 8, 16,
> > scaling_matrix_dc_rec, or scaling_matrix_rec.
> > Then we use (dc + scalingMatrixDcPred) & 255 to get ScalingMatrixDcRec
> > The original logic looks correct to me. Did I miss something? Could you
> > send me the clip?
>
> In the code before the patch, we don't add together scalingMatrixDcPred
> and scaling_list_dc_coef in the first two cases.  Indeed, dc (i.e.
> scaling_list_dc_coef) is entirely unused if
> aps->scaling_list_pred_id_delta[id] is zero.
>
> Before we hit this if (id >= SL_START_16x16) block, dc is equal to
> scaling_list_dc_coef.  Then, one of three things can happen:
> * If scaling_list_copy_mode_flag[id] and scaling_list_pred_mode_flag[id]
>   are both equal to zero:
>     * Before the patch, scaling_matrix_dc_rec is set equal to 8, i.e.
>       scalingMatrixDcPred.  Then, scaling_matrix_dc_rec is set equal to
>       dc & 255, i.e. scalingMatrixDcPred & 255.  Note the missing
>       scaling_list_dc_coef term.
>     * After the patch, 8, i.e. scalingMatrixDcPred is *added* to dc.
>       After this, the value of dc is
>       scaling_matrix_dc_rec + scalingMatrixDcPred.  Then,
>       scaling_matrix_dc_rec is set to dc & 255, i.e.
>       (scalingMatrixDcPred + scaling_matrix_dc_rec) & 255.
> * Otherwise, if scaling_list_pred_id_delta[id] is equal to 0, the case
>   proceeds in a similar fashion as for the first case, but with
>   scalingMatrixDcPred equal to 16 instead of 8.
> * Otherwise, before before and after the patch, dependent on the value
>   of refId, either ScalingMatrixDcRec or scalingMatrixPred is added to
>   dc, hence the value of dc is equal to
>   scalingMatrixDcPred + scaling_list_dc_coef.  We then set
>   scaling_matrix_dc_rec equal to dc & 255.
>
> This final case is fine both before and after the patch, but the first
> two cases are incorrect before the patch as dc (i.e.
> scaling_list_dc_coef) is unused.
>
> I observed this behaviour in the bitstream vvc_frames_with_ltr.vvc,
> available here:
>
> https://chromium.googlesource.com/chromium/src/+/lkgr/media/test/data/vvc_frames_with_ltr.vvc
> .
>  Note to download, the txt button at the button of the page gives the
> file encoded in base64 format.  I think this issue first appears in the
> CU with top-left corner (232, 216) on the frame with POC 0.
>
> >
> >>
> >> Signed-off-by: Frank Plowman <post at frankplowman.com>
> >> ---
> >>  libavcodec/vvc/ps.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/libavcodec/vvc/ps.c b/libavcodec/vvc/ps.c
> >> index f32f1cc5a1..9bd2d01776 100644
> >> --- a/libavcodec/vvc/ps.c
> >> +++ b/libavcodec/vvc/ps.c
> >> @@ -1107,17 +1107,17 @@ static void scaling_derive(VVCScalingList *sl,
> >> const H266RawAPS *aps)
> >>          //dc
> >>          if (id >= SL_START_16x16) {
> >>              if (!aps->scaling_list_copy_mode_flag[id] &&
> >> !aps->scaling_list_pred_mode_flag[id]) {
> >> -                sl->scaling_matrix_dc_rec[id - SL_START_16x16] = 8;
> >> +                dc += 8;
> >>              } else if (!aps->scaling_list_pred_id_delta[id]) {
> >> -                sl->scaling_matrix_dc_rec[id - SL_START_16x16] = 16;
> >> +                dc += 16;
> >>              } else {
> >>                  const int ref_id = id -
> >> aps->scaling_list_pred_id_delta[id];
> >>                  if (ref_id >= SL_START_16x16)
> >>                      dc += sl->scaling_matrix_dc_rec[ref_id -
> >> SL_START_16x16];
> >>                  else
> >>                      dc += sl->scaling_matrix_rec[ref_id][0];
> >>
> > This should be  sl->scaling_matrix_rec[0][0];
> > Is the issue related to this?
>
> This might be another issue, I'm not sure.  I tried making this change
> and didn't observe any difference on vvc_frames_with_ltr.yuv or any of
> the conformance bitstreams.  I tried this both with and without my patch
> applied also, and still changing this to [0][0] made no difference.
>
> >
> > -                sl->scaling_matrix_dc_rec[id - SL_START_16x16] = dc &
> 255;
> >>              }
> >> +            sl->scaling_matrix_dc_rec[id - SL_START_16x16] = dc & 255;
> >>          }
> >>
> >>          //ac
> >> --
> >> 2.47.0
> >>
>
> --
> Frank
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>