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".