Hi Frank, Thank you for the detail Applied. On Sat, Nov 30, 2024 at 6:11 PM Frank Plowman <p...@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 <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". > _______________________________________________ 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".
