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

Frank Plowman post at frankplowman.com
Sun Dec 29 12:24:09 EET 2024


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.

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.

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

>
>>      }
>>
>>     -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