| Message ID | AS8P250MB0744E573A29753776E0FA3578FC9A@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/13] avcodec/mpegvideo_enc: Fix abort on allocation errors | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
On Fri, Oct 06, 2023 at 04:46:29AM +0200, Andreas Rheinhardt wrote: > Only entries 0..max_b_frames are ever used. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/mpegvideo_enc.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > index 1e0aed8db9..c06fdd08fe 100644 > --- a/libavcodec/mpegvideo_enc.c > +++ b/libavcodec/mpegvideo_enc.c > @@ -819,8 +819,8 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) > !FF_ALLOCZ_TYPED_ARRAY(s->q_intra_matrix16, 32) || > !FF_ALLOCZ_TYPED_ARRAY(s->q_chroma_intra_matrix16, 32) || > !FF_ALLOCZ_TYPED_ARRAY(s->q_inter_matrix16, 32) || > - !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_PICTURE_COUNT) || > - !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_PICTURE_COUNT)) > + !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_B_FRAMES + 1) || > + !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_B_FRAMES + 1)) > return AVERROR(ENOMEM); > > /* Allocate MV tables; the MV and MB tables will be copied > @@ -1231,7 +1231,7 @@ static int load_input_picture(MpegEncContext *s, const AVFrame *pic_arg) > } > > /* shift buffer entries */ > - for (i = flush_offset; i < MAX_PICTURE_COUNT /*s->encoding_delay + 1*/; i++) > + for (int i = flush_offset; i <= MAX_B_FRAMES; i++) > s->input_picture[i - flush_offset] = s->input_picture[i]; > > s->input_picture[encoding_delay] = pic; > @@ -1450,9 +1450,9 @@ static int select_input_picture(MpegEncContext *s) > { > int i, ret; > > - for (i = 1; i < MAX_PICTURE_COUNT; i++) > + for (int i = 1; i <= MAX_B_FRAMES; i++) > s->reordered_input_picture[i - 1] = s->reordered_input_picture[i]; I see the addition of "int" and that seems neither needed nor explained why in the commit message thx [...]
Michael Niedermayer: > On Fri, Oct 06, 2023 at 04:46:29AM +0200, Andreas Rheinhardt wrote: >> Only entries 0..max_b_frames are ever used. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/mpegvideo_enc.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c >> index 1e0aed8db9..c06fdd08fe 100644 >> --- a/libavcodec/mpegvideo_enc.c >> +++ b/libavcodec/mpegvideo_enc.c >> @@ -819,8 +819,8 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) >> !FF_ALLOCZ_TYPED_ARRAY(s->q_intra_matrix16, 32) || >> !FF_ALLOCZ_TYPED_ARRAY(s->q_chroma_intra_matrix16, 32) || >> !FF_ALLOCZ_TYPED_ARRAY(s->q_inter_matrix16, 32) || >> - !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_PICTURE_COUNT) || >> - !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_PICTURE_COUNT)) >> + !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_B_FRAMES + 1) || >> + !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_B_FRAMES + 1)) >> return AVERROR(ENOMEM); >> >> /* Allocate MV tables; the MV and MB tables will be copied >> @@ -1231,7 +1231,7 @@ static int load_input_picture(MpegEncContext *s, const AVFrame *pic_arg) >> } >> >> /* shift buffer entries */ >> - for (i = flush_offset; i < MAX_PICTURE_COUNT /*s->encoding_delay + 1*/; i++) >> + for (int i = flush_offset; i <= MAX_B_FRAMES; i++) >> s->input_picture[i - flush_offset] = s->input_picture[i]; >> >> s->input_picture[encoding_delay] = pic; >> @@ -1450,9 +1450,9 @@ static int select_input_picture(MpegEncContext *s) >> { >> int i, ret; >> >> - for (i = 1; i < MAX_PICTURE_COUNT; i++) >> + for (int i = 1; i <= MAX_B_FRAMES; i++) >> s->reordered_input_picture[i - 1] = s->reordered_input_picture[i]; > > I see the addition of "int" and that seems neither needed nor > explained why in the commit message > It's part of the general switch to the loop-based iterators wherever possible (it is better because it automatically indicates that the value at the end of the loop doesn't matter and it also allows to more easily move blocks of code around). I always use them when I touch a loop. If it matters: That i in the outer scope survives this patchset, but it won't survive for long. - Andreas
Andreas Rheinhardt: > Michael Niedermayer: >> On Fri, Oct 06, 2023 at 04:46:29AM +0200, Andreas Rheinhardt wrote: >>> Only entries 0..max_b_frames are ever used. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> libavcodec/mpegvideo_enc.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c >>> index 1e0aed8db9..c06fdd08fe 100644 >>> --- a/libavcodec/mpegvideo_enc.c >>> +++ b/libavcodec/mpegvideo_enc.c >>> @@ -819,8 +819,8 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) >>> !FF_ALLOCZ_TYPED_ARRAY(s->q_intra_matrix16, 32) || >>> !FF_ALLOCZ_TYPED_ARRAY(s->q_chroma_intra_matrix16, 32) || >>> !FF_ALLOCZ_TYPED_ARRAY(s->q_inter_matrix16, 32) || >>> - !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_PICTURE_COUNT) || >>> - !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_PICTURE_COUNT)) >>> + !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_B_FRAMES + 1) || >>> + !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_B_FRAMES + 1)) >>> return AVERROR(ENOMEM); >>> >>> /* Allocate MV tables; the MV and MB tables will be copied >>> @@ -1231,7 +1231,7 @@ static int load_input_picture(MpegEncContext *s, const AVFrame *pic_arg) >>> } >>> >>> /* shift buffer entries */ >>> - for (i = flush_offset; i < MAX_PICTURE_COUNT /*s->encoding_delay + 1*/; i++) >>> + for (int i = flush_offset; i <= MAX_B_FRAMES; i++) >>> s->input_picture[i - flush_offset] = s->input_picture[i]; >>> >>> s->input_picture[encoding_delay] = pic; >>> @@ -1450,9 +1450,9 @@ static int select_input_picture(MpegEncContext *s) >>> { >>> int i, ret; >>> >>> - for (i = 1; i < MAX_PICTURE_COUNT; i++) >>> + for (int i = 1; i <= MAX_B_FRAMES; i++) >>> s->reordered_input_picture[i - 1] = s->reordered_input_picture[i]; >> >> I see the addition of "int" and that seems neither needed nor >> explained why in the commit message >> > > It's part of the general switch to the loop-based iterators wherever > possible (it is better because it automatically indicates that the value > at the end of the loop doesn't matter and it also allows to more easily > move blocks of code around). I always use them when I touch a loop. > > If it matters: That i in the outer scope survives this patchset, but it > won't survive for long. > Are you ok with this patch and the rest of the patchset? I'd like to apply it tomorrow unless there are objections. - Andreas
On Mon, Oct 09, 2023 at 02:30:21PM +0200, Andreas Rheinhardt wrote: > Andreas Rheinhardt: > > Michael Niedermayer: > >> On Fri, Oct 06, 2023 at 04:46:29AM +0200, Andreas Rheinhardt wrote: > >>> Only entries 0..max_b_frames are ever used. > >>> > >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >>> --- > >>> libavcodec/mpegvideo_enc.c | 10 +++++----- > >>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > >>> index 1e0aed8db9..c06fdd08fe 100644 > >>> --- a/libavcodec/mpegvideo_enc.c > >>> +++ b/libavcodec/mpegvideo_enc.c > >>> @@ -819,8 +819,8 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) > >>> !FF_ALLOCZ_TYPED_ARRAY(s->q_intra_matrix16, 32) || > >>> !FF_ALLOCZ_TYPED_ARRAY(s->q_chroma_intra_matrix16, 32) || > >>> !FF_ALLOCZ_TYPED_ARRAY(s->q_inter_matrix16, 32) || > >>> - !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_PICTURE_COUNT) || > >>> - !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_PICTURE_COUNT)) > >>> + !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_B_FRAMES + 1) || > >>> + !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_B_FRAMES + 1)) > >>> return AVERROR(ENOMEM); > >>> > >>> /* Allocate MV tables; the MV and MB tables will be copied > >>> @@ -1231,7 +1231,7 @@ static int load_input_picture(MpegEncContext *s, const AVFrame *pic_arg) > >>> } > >>> > >>> /* shift buffer entries */ > >>> - for (i = flush_offset; i < MAX_PICTURE_COUNT /*s->encoding_delay + 1*/; i++) > >>> + for (int i = flush_offset; i <= MAX_B_FRAMES; i++) > >>> s->input_picture[i - flush_offset] = s->input_picture[i]; > >>> > >>> s->input_picture[encoding_delay] = pic; > >>> @@ -1450,9 +1450,9 @@ static int select_input_picture(MpegEncContext *s) > >>> { > >>> int i, ret; > >>> > >>> - for (i = 1; i < MAX_PICTURE_COUNT; i++) > >>> + for (int i = 1; i <= MAX_B_FRAMES; i++) > >>> s->reordered_input_picture[i - 1] = s->reordered_input_picture[i]; > >> > >> I see the addition of "int" and that seems neither needed nor > >> explained why in the commit message > >> > > > > It's part of the general switch to the loop-based iterators wherever > > possible (it is better because it automatically indicates that the value > > at the end of the loop doesn't matter and it also allows to more easily > > move blocks of code around). I always use them when I touch a loop. > > > > If it matters: That i in the outer scope survives this patchset, but it > > won't survive for long. > > > > Are you ok with this patch ok > and the rest of the patchset? I'd like to > apply it tomorrow unless there are objections. i have no objections thx [...]
diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c index 1e0aed8db9..c06fdd08fe 100644 --- a/libavcodec/mpegvideo_enc.c +++ b/libavcodec/mpegvideo_enc.c @@ -819,8 +819,8 @@ av_cold int ff_mpv_encode_init(AVCodecContext *avctx) !FF_ALLOCZ_TYPED_ARRAY(s->q_intra_matrix16, 32) || !FF_ALLOCZ_TYPED_ARRAY(s->q_chroma_intra_matrix16, 32) || !FF_ALLOCZ_TYPED_ARRAY(s->q_inter_matrix16, 32) || - !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_PICTURE_COUNT) || - !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_PICTURE_COUNT)) + !FF_ALLOCZ_TYPED_ARRAY(s->input_picture, MAX_B_FRAMES + 1) || + !FF_ALLOCZ_TYPED_ARRAY(s->reordered_input_picture, MAX_B_FRAMES + 1)) return AVERROR(ENOMEM); /* Allocate MV tables; the MV and MB tables will be copied @@ -1231,7 +1231,7 @@ static int load_input_picture(MpegEncContext *s, const AVFrame *pic_arg) } /* shift buffer entries */ - for (i = flush_offset; i < MAX_PICTURE_COUNT /*s->encoding_delay + 1*/; i++) + for (int i = flush_offset; i <= MAX_B_FRAMES; i++) s->input_picture[i - flush_offset] = s->input_picture[i]; s->input_picture[encoding_delay] = pic; @@ -1450,9 +1450,9 @@ static int select_input_picture(MpegEncContext *s) { int i, ret; - for (i = 1; i < MAX_PICTURE_COUNT; i++) + for (int i = 1; i <= MAX_B_FRAMES; i++) s->reordered_input_picture[i - 1] = s->reordered_input_picture[i]; - s->reordered_input_picture[MAX_PICTURE_COUNT - 1] = NULL; + s->reordered_input_picture[MAX_B_FRAMES] = NULL; /* set next picture type & ordering */ if (!s->reordered_input_picture[0] && s->input_picture[0]) {
Only entries 0..max_b_frames are ever used. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/mpegvideo_enc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)