[FFmpeg-devel] [PATCH v2] lavc/vvc: Fix race condition for MVs cropped to subpic

Nuo Mi nuomi2021 at gmail.com
Sat Jan 4 15:51:48 EET 2025


On Fri, Jan 3, 2025 at 2:01 AM Frank Plowman <post at frankplowman.com> wrote:

> Thank you for your review.
>
> On 01/01/2025 04:30, Nuo Mi wrote:
> > 👍
> >
> > On Tue, Dec 31, 2024 at 2:02 AM Frank Plowman <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>
> >> ---
> >> Changes since v1:
> >> * Also clip max_y to the subpic_bottom, in order to prevent adding
> >>   unecessary dependencies.  max_y is also clipped to the picture bottom
> >>   where it wasn't before.  This doesn't make any difference to the
> >>   function of the code as is, but this was only previously unnecessary
> >>   due to a detail of how y-coordinates are mapped to CTU rows.  To aid
> >>   comprehensibility, the MV is now clipped to both the top and bottom
> edges
> >>   of the picture.
> >> * Rename subpic_t to subpic_top to avoid clashing with conventions for
> >>   type names.
> >> ---
> >>  libavcodec/vvc/ctu.c | 34 +++++++++++++++++++++++-----------
> >>  1 file changed, 23 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/libavcodec/vvc/ctu.c b/libavcodec/vvc/ctu.c
> >> index f80bce637c..b82dece0dd 100644
> >> --- a/libavcodec/vvc/ctu.c
> >> +++ b/libavcodec/vvc/ctu.c
> >> @@ -2393,23 +2393,34 @@ 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 pic_height = pps->height;
> >> +    const int subpic_idx = lc->sc->sh.r->curr_subpic_idx;
> >> +    const int subpic_top = pps->subpic_y[subpic_idx];
> >> +    const int subpic_bottom = subpic_top +
> pps->subpic_height[subpic_idx]
> >> - 1;
> >>
> > -1 is not needed.
>
> Are you sure?  Would subpic_top + subpic_height not give the
>
Not entirely sure, but based on the previous code logic, -1 doesn’t seem
necessary.
We can submit another patch if we later decide that -1 isn’t harmful.

y-coordinate of the top edge of the subpic below the current one?  And
> av_clip(a, amin, amax) returns aclip in the range amin <= aclip <= amax,
> so this would allow for y-coordinates in the lower subpic and so next
> CTU row.
>


>
> >
> >> +    const int subpic_treated_as_pic =
> >> sps->r->sps_subpic_treated_as_pic_flag[subpic_idx];
> >> +    const int lower_bound = subpic_treated_as_pic ? subpic_top : 0;
> >> +    const int upper_bound = subpic_treated_as_pic ? subpic_bottom :
> >> pic_height;
> >> +    return av_clip(y0 + (mv->y >> 4) + height, lower_bound,
> upper_bound);
> >>  }
> >>
> > This can be further simplified as
> >
> > static int pred_get_y(const VVCLocalContext *lc, const int y0, const Mv
> *mv,
> > const int height, const VVCFrame *src_frame)
> > {
> >     const VVCPPS *pps = src_frame->pps;
> >     const int subpic_idx = lc->sc->sh.r->curr_subpic_idx;
> >     const int top = pps->subpic_y[subpic_idx];
> >     const int bottom = top + pps->subpic_height[subpic_idx];
> >
> >     return av_clip(y0 + (mv->y >> 4) + height, top, bottom);
> > }
> >
> > since subpic_y and subpic_height have valid values even if we have no
> > subpic.
> > see static void pps_subpic(VVCPPS *pps, const VVCSPS *sps)
>
> Ah, I didn't realise this.  Yes I agree this is cleaner.  Would you like
> me to send a v3 or can you just use your local changes?
>
Sure. No need v3, I will make the change.

thank you

>
> >
> >>
> >> -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 +2435,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 +2464,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
> >>
> >>
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
>
>
>


More information about the ffmpeg-devel mailing list