On Sun, Dec 29, 2024 at 6:24 PM Frank Plowman <p...@frankplowman.com> wrote:
> Hi, > > Thank you for your review. > > It seems your mail did not reach the ML for whatever reason. I have > ensured your comments are all left verbatim below so that others may > read them. > Thank you. > > On 29/12/2024 03:44, Nuo Mi wrote: > > Hi Frank, > > Thank you for the patch. > > > > On Sun, Dec 22, 2024 at 9:40 PM Frank Plowman <p...@frankplowman.com > > <mailto:p...@frankplowman.com>> wrote: > >> > >> When the current subpicture has sps_subpic_treated_as_pic_flag > >> equal to > >> 1, motion vectors are cropped such that they cannot point to > >> other > >> subpictures. This was accounted for in the prediction logic, but > >> not > >> in pred_get_y, which is used by the scheduling logic to determine > >> which > >> parts of the reference pictures must have been reconstructed > >> before > >> inter prediction of a subsequent frame may begin. Consequently, > >> where a > >> motion vector pointed to a location significantly above the > >> current > >> subpicture, there was the possibility of a race condition. Patch > >> fixes > >> this by cropping the motion vector to the current subpicture in > >> pred_get_y. > >> > >> Signed-off-by: Frank Plowman <p...@frankplowman.com > >> <mailto:p...@frankplowman.com>> > >> --- > >> You can reproduce this issue with the bitstream available here: > >> https://chromium.googlesource.com/chromium/src/+/lkgr/media/test/ > >> data/vvc_frames_with_ltr.vvc?format=TEXT <https:// > >> chromium.googlesource.com/chromium/src/+/lkgr/media/test/data/ > >> vvc_frames_with_ltr.vvc?format=TEXT> > >> Note this link downloads the file in base 64 format rather than > >> binary. > >> You can use base64 -d <BASE 64 FILE> > <BINARY FILE> to convert > >> it to > >> binary. The issue does not reproduce very reliably with a normal > >> number > >> of threads. For better reproducibility, run ffmpeg with 100+ > >> threads. > >> --- > >> libavcodec/vvc/ctu.c | 30 +++++++++++++++++++----------- > >> 1 file changed, 19 insertions(+), 11 deletions(-) > >> > >> diff --git a/libavcodec/vvc/ctu.c b/libavcodec/vvc/ctu.c > >> index f80bce637c..6857c79f2b 100644 > >> --- a/libavcodec/vvc/ctu.c > >> +++ b/libavcodec/vvc/ctu.c > >> @@ -2393,23 +2393,30 @@ static int has_inter_luma(const > >> CodingUnit *cu) > >> return cu->pred_mode != MODE_INTRA && cu->pred_mode != > >> MODE_PLT > >> && cu->tree_type != DUAL_TREE_CHROMA; > >> } > >> > >> -static int pred_get_y(const int y0, const Mv *mv, const int > >> height) > >> +static int pred_get_y(const VVCLocalContext *lc, const int y0, > >> const Mv *mv, const int height, const VVCFrame *src_frame) > >> { > >> - return FFMAX(0, y0 + (mv->y >> 4) + height); > >> + const VVCSPS *sps = src_frame->sps; > >> + const VVCPPS *pps = src_frame->pps; > >> + const int subpic_idx = lc->sc->sh.r->curr_subpic_idx; > >> + const int subpic_t = pps->subpic_y[subpic_idx]; > > > > _t reserved by types, line int8_t > > Good catch! Could you rename this to subpic_top locally when you commit > to avoid sending a v2? Alternatively, I have fixed it locally in the > case a v2 is needed for other reasons. > sure. > > > > >> + const int subpic_treated_as_pic = sps->r- > >> >sps_subpic_treated_as_pic_flag[subpic_idx]; > >> + return FFMAX(subpic_treated_as_pic ? subpic_t : 0, y0 + (mv->y > >> >> 4) + height); > > > > Clipping to the subpicture bottom might be better, as y + (mv->y >> 4) + > > height could be much smaller than the bottom boundary. > > The pathological case only occurs when the clipping causes the > y-coordinate of the referenced area to increase. The MV is only clipped > to the bottom boundary (specifically to subpic_bottom - height) in the case > > y + (mv->y >> 4) + height > subpic_bottom > > hence the clipped y is less than the unclipped y and the issue does not > occur. Clipping to the subpicture bottom would result in reduced > performance as it would add unnecessary dependencies, particularly for > single-subpicture sequences which make up the majority of sequences. > We can apply the clipping only when sps_subpic_treated_as_pic_flag is set to true. In the patch, if sps_subpic_treated_as_pic_flag is true and y + (mv->y >> 4) + height > subpic_bottom, we are still waiting for y + (mv->y >> 4) + height. However, this is unnecessary because the reference line is constrained within the current subpicture (ie subpic_bottom) > > > > >> } > >> > >> -static void cu_get_max_y(const CodingUnit *cu, int max_y[2] > >> [VVC_MAX_REF_ENTRIES], const VVCFrameContext *fc) > >> +static void cu_get_max_y(const VVCLocalContext *lc, const > >> CodingUnit *cu, int max_y[2][VVC_MAX_REF_ENTRIES]) > >> { > >> + const VVCFrameContext *fc = lc->fc; > >> const PredictionUnit *pu = &cu->pu; > >> > >> if (pu->merge_gpm_flag) { > >> for (int i = 0; i < FF_ARRAY_ELEMS(pu->gpm_mv); i++) { > >> - const MvField *mvf = pu->gpm_mv + i; > >> - const int lx = mvf->pred_flag - PF_L0; > >> - const int idx = mvf->ref_idx[lx]; > >> - const int y = pred_get_y(cu->y0, mvf->mv + > >> lx, > >> cu->cb_height); > >> + const MvField *mvf = pu->gpm_mv + i; > >> + const int lx = mvf->pred_flag - PF_L0; > >> + const int idx = mvf->ref_idx[lx]; > >> + const VVCRefPic *refp = lc->sc->rpl[lx].refs + idx; > >> + const int y = pred_get_y(lc, cu->y0, > mvf->> >mv > >> + lx, cu->cb_height, refp->ref); > >> > >> - max_y[lx][idx] = FFMAX(max_y[lx][idx], y); > >> + max_y[lx][idx] = FFMAX(max_y[lx][idx], y); > >> > >> } > >> } else { > >> const MotionInfo *mi = &pu->mi; > >> @@ -2424,8 +2431,9 @@ static void cu_get_max_y(const CodingUnit > >> *cu, > >> int max_y[2][VVC_MAX_REF_ENTRIES] > >> for (int lx = 0; lx < 2; lx++) { > >> const PredFlag mask = 1 << lx; > >> if (mvf->pred_flag & mask) { > >> - const int idx = mvf->ref_idx[lx]; > >> - const int y = pred_get_y(y0, mvf->mv > >> + > >> lx, sbh); > >> + const int idx = mvf- > >> >ref_idx[lx]; > >> + const VVCRefPic *refp = lc->sc- > >> >rpl[lx].refs + idx; > >> + int y = pred_get_y(lc, > >> y0, > >> mvf->mv + lx, sbh, refp->ref); > >> > >> max_y[lx][idx] = FFMAX(max_y[lx][idx], > >> y + > >> max_dmvr_off); > >> } > >> @@ -2452,7 +2460,7 @@ static void ctu_get_pred(VVCLocalContext > >> *lc, > >> const int rs) > >> > >> while (cu) { > >> if (has_inter_luma(cu)) { > >> - cu_get_max_y(cu, ctu->max_y, fc); > >> + cu_get_max_y(lc, cu, ctu->max_y); > >> ctu->has_dmvr |= cu->pu.dmvr_flag; > >> } > >> cu = cu->next; > >> -- > >> 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".
