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

[Frank Plowman]

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