[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