On Sat, Jan 4, 2025 at 9:51 PM Nuo Mi <nuomi2...@gmail.com> wrote:
>
>
> On Fri, Jan 3, 2025 at 2:01 AM Frank Plowman <p...@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 <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>
>> >> ---
>> >> 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.
>
Changes have been made and pushed.
Please note that section 8.3.2, Decoding process for reference picture
lists construction, requires the subpicture size to be the same for both
the reference and current frames.
Therefore, I further simplified the patch.
Thank you for the patch.
>
> 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@ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>>
>>
>>
>>
_______________________________________________
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".
