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 <p...@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 <p...@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@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
