[FFmpeg-devel] [PATCH] lavc/vvc: Fix race condition for MVs cropped to subpic
Nuo Mi
nuomi2021 at gmail.com
Mon Dec 30 15:58:39 EET 2024
On Sun, Dec 29, 2024 at 6:24 PM Frank Plowman <post at 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 <post at frankplowman.com
> > <mailto:post at 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 <post at frankplowman.com
> >> <mailto:post at 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
>
>
More information about the ffmpeg-devel
mailing list