Skip to content

Commit d4b9e11

Browse files
mkvermkver
committed
Revert "avcodec: add FF_CODEC_CAP_INIT_CLEANUP for all codecs which use ff_mpv_common_init()"
This mostly reverts commit 4b2863f. Said commit removed the freeing code from ff_mpv_common_init(), ff_mpv_common_frame_size_change() and ff_mpeg_framesize_alloc() and instead added the FF_CODEC_CAP_INIT_CLEANUP to several codecs that use ff_mpv_common_init(). This introduced several bugs: a) Several decoders using ff_mpv_common_init() in their init function were forgotten: This affected FLV, Intel H.263, RealVideo 3.0 and V4.0 as well as VC-1/WMV3. b) ff_mpv_common_init() is not only called from the init function of codecs, it is also called from AVCodec.decode functions. If an error happens after an allocation has succeeded, it can lead to memleaks; furthermore, it is now possible for the MpegEncContext to be marked as initialized even when ff_mpv_common_init() returns an error and this can lead to segfaults because decoders that call ff_mpv_common_init() when decoding a frame can mistakenly think that the MpegEncContext has been properly initialized. This can e.g. happen with H.261 or MPEG-4. c) Removing code for freeing from ff_mpeg_framesize_alloc() (which can't be called from any init function) can lead to segfaults because the check for whether it needs to allocate consists of checking whether the first of the buffers allocated there has been allocated. This part has already been fixed in 76cea1d. d) ff_mpv_common_frame_size_change() can also not be reached from any AVCodec.init function; yet the changes can e.g. lead to segfaults with decoders using ff_h263_decode_frame() upon allocation failure, because the MpegEncContext will upon return be flagged as both initialized and not in need of reinitialization (granted, the fact that ff_h263_decode_frame() clears context_reinit before the context has been reinited is a bug in itself). With the earlier version, the context would be cleaned upon failure and it would be attempted to initialize the context again in the next call to ff_h263_decode_frame(). While a) could be fixed by adding the missing FF_CODEC_CAP_INIT_CLEANUP, keeping the current approach would entail adding cleanup code to several other places because of b). Therefore ff_mpv_common_init() is again made to clean up after itself; the changes to the wmv2 decoder and the SVQ1 encoder have not been reverted: The former fixed a memleak, the latter allowed to remove cleanup code. Fixes: double free Fixes: ff_free_picture_tables.mp4 Fixes: ff_mpeg_update_thread_context.mp4 Fixes: decode_colskip.mp4 Fixes: memset.mp4 Reviewed-by: Michael Niedermayer <michael@niedermayer.cc> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
1 parent fb4da90 commit d4b9e11

File tree

7 files changed

+34
-27
lines changed

7 files changed

+34
-27
lines changed

‎libavcodec/h261dec.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -678,6 +678,6 @@ AVCodec ff_h261_decoder = {
678678
.close = h261_decode_end,
679679
.decode = h261_decode_frame,
680680
.capabilities = AV_CODEC_CAP_DR1,
681-
.caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP,
681+
.caps_internal = FF_CODEC_CAP_INIT_THREADSAFE,
682682
.max_lowres = 3,
683683
};

‎libavcodec/h263dec.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -771,7 +771,7 @@ AVCodec ff_h263_decoder = {
771771
.decode = ff_h263_decode_frame,
772772
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 |
773773
AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY,
774-
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP,
774+
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
775775
.flush = ff_mpeg_flush,
776776
.max_lowres = 3,
777777
.pix_fmts = ff_h263_hwaccel_pixfmt_list_420,
@@ -789,7 +789,7 @@ AVCodec ff_h263p_decoder = {
789789
.decode = ff_h263_decode_frame,
790790
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 |
791791
AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY,
792-
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP,
792+
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
793793
.flush = ff_mpeg_flush,
794794
.max_lowres = 3,
795795
.pix_fmts = ff_h263_hwaccel_pixfmt_list_420,

‎libavcodec/mpeg12dec.c‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2880,7 +2880,8 @@ static av_cold int mpeg_decode_end(AVCodecContext *avctx)
28802880
{
28812881
Mpeg1Context *s = avctx->priv_data;
28822882

2883-
ff_mpv_common_end(&s->mpeg_enc_ctx);
2883+
if (s->mpeg_enc_ctx_allocated)
2884+
ff_mpv_common_end(&s->mpeg_enc_ctx);
28842885
av_buffer_unref(&s->a53_buf_ref);
28852886
return 0;
28862887
}
@@ -2897,7 +2898,7 @@ AVCodec ff_mpeg1video_decoder = {
28972898
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 |
28982899
AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY |
28992900
AV_CODEC_CAP_SLICE_THREADS,
2900-
.caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP |
2901+
.caps_internal = FF_CODEC_CAP_INIT_THREADSAFE |
29012902
FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
29022903
.flush = flush,
29032904
.max_lowres = 3,
@@ -2931,7 +2932,7 @@ AVCodec ff_mpeg2video_decoder = {
29312932
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 |
29322933
AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY |
29332934
AV_CODEC_CAP_SLICE_THREADS,
2934-
.caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP |
2935+
.caps_internal = FF_CODEC_CAP_INIT_THREADSAFE |
29352936
FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
29362937
.flush = flush,
29372938
.max_lowres = 3,
@@ -2976,7 +2977,7 @@ AVCodec ff_mpegvideo_decoder = {
29762977
.close = mpeg_decode_end,
29772978
.decode = mpeg_decode_frame,
29782979
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1 | AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_SLICE_THREADS,
2979-
.caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP |
2980+
.caps_internal = FF_CODEC_CAP_INIT_THREADSAFE |
29802981
FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
29812982
.flush = flush,
29822983
.max_lowres = 3,

‎libavcodec/mpeg4videodec.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3579,8 +3579,7 @@ AVCodec ff_mpeg4_decoder = {
35793579
AV_CODEC_CAP_TRUNCATED | AV_CODEC_CAP_DELAY |
35803580
AV_CODEC_CAP_FRAME_THREADS,
35813581
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM |
3582-
FF_CODEC_CAP_ALLOCATE_PROGRESS |
3583-
FF_CODEC_CAP_INIT_CLEANUP,
3582+
FF_CODEC_CAP_ALLOCATE_PROGRESS,
35843583
.flush = ff_mpeg_flush,
35853584
.max_lowres = 3,
35863585
.pix_fmts = ff_h263_hwaccel_pixfmt_list_420,

‎libavcodec/mpegvideo.c‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -932,17 +932,17 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
932932
for (i = 0; i < MAX_PICTURE_COUNT; i++) {
933933
s->picture[i].f = av_frame_alloc();
934934
if (!s->picture[i].f)
935-
return AVERROR(ENOMEM);
935+
goto fail_nomem;
936936
}
937937

938938
if (!(s->next_picture.f = av_frame_alloc()) ||
939939
!(s->last_picture.f = av_frame_alloc()) ||
940940
!(s->current_picture.f = av_frame_alloc()) ||
941941
!(s->new_picture.f = av_frame_alloc()))
942-
return AVERROR(ENOMEM);
942+
goto fail_nomem;
943943

944944
if ((ret = init_context_frame(s)))
945-
return AVERROR(ENOMEM);
945+
goto fail;
946946

947947
s->parse_context.state = -1;
948948

@@ -956,25 +956,30 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
956956
if (i) {
957957
s->thread_context[i] = av_memdup(s, sizeof(MpegEncContext));
958958
if (!s->thread_context[i])
959-
return AVERROR(ENOMEM);
959+
goto fail_nomem;
960960
}
961961
if ((ret = init_duplicate_context(s->thread_context[i])) < 0)
962-
return ret;
962+
goto fail;
963963
s->thread_context[i]->start_mb_y =
964964
(s->mb_height * (i) + nb_slices / 2) / nb_slices;
965965
s->thread_context[i]->end_mb_y =
966966
(s->mb_height * (i + 1) + nb_slices / 2) / nb_slices;
967967
}
968968
} else {
969969
if ((ret = init_duplicate_context(s)) < 0)
970-
return ret;
970+
goto fail;
971971
s->start_mb_y = 0;
972972
s->end_mb_y = s->mb_height;
973973
}
974974
s->slice_context_count = nb_slices;
975975
// }
976976

977977
return 0;
978+
fail_nomem:
979+
ret = AVERROR(ENOMEM);
980+
fail:
981+
ff_mpv_common_end(s);
982+
return ret;
978983
}
979984

980985
/**
@@ -1067,17 +1072,17 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s)
10671072

10681073
if ((s->width || s->height) &&
10691074
(err = av_image_check_size(s->width, s->height, 0, s->avctx)) < 0)
1070-
return err;
1075+
goto fail;
10711076

10721077
/* set chroma shifts */
10731078
err = av_pix_fmt_get_chroma_sub_sample(s->avctx->pix_fmt,
10741079
&s->chroma_x_shift,
10751080
&s->chroma_y_shift);
10761081
if (err < 0)
1077-
return err;
1082+
goto fail;
10781083

10791084
if ((err = init_context_frame(s)))
1080-
return err;
1085+
goto fail;
10811086

10821087
memset(s->thread_context, 0, sizeof(s->thread_context));
10831088
s->thread_context[0] = s;
@@ -1089,11 +1094,12 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s)
10891094
if (i) {
10901095
s->thread_context[i] = av_memdup(s, sizeof(MpegEncContext));
10911096
if (!s->thread_context[i]) {
1092-
return AVERROR(ENOMEM);
1097+
err = AVERROR(ENOMEM);
1098+
goto fail;
10931099
}
10941100
}
10951101
if ((err = init_duplicate_context(s->thread_context[i])) < 0)
1096-
return err;
1102+
goto fail;
10971103
s->thread_context[i]->start_mb_y =
10981104
(s->mb_height * (i) + nb_slices / 2) / nb_slices;
10991105
s->thread_context[i]->end_mb_y =
@@ -1102,14 +1108,17 @@ int ff_mpv_common_frame_size_change(MpegEncContext *s)
11021108
} else {
11031109
err = init_duplicate_context(s);
11041110
if (err < 0)
1105-
return err;
1111+
goto fail;
11061112
s->start_mb_y = 0;
11071113
s->end_mb_y = s->mb_height;
11081114
}
11091115
s->slice_context_count = nb_slices;
11101116
}
11111117

11121118
return 0;
1119+
fail:
1120+
ff_mpv_common_end(s);
1121+
return err;
11131122
}
11141123

11151124
/* init common structure for both encoder and decoder */

‎libavcodec/msmpeg4dec.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -870,7 +870,7 @@ AVCodec ff_msmpeg4v1_decoder = {
870870
.close = ff_h263_decode_end,
871871
.decode = ff_h263_decode_frame,
872872
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1,
873-
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP,
873+
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
874874
.max_lowres = 3,
875875
.pix_fmts = (const enum AVPixelFormat[]) {
876876
AV_PIX_FMT_YUV420P,
@@ -888,7 +888,7 @@ AVCodec ff_msmpeg4v2_decoder = {
888888
.close = ff_h263_decode_end,
889889
.decode = ff_h263_decode_frame,
890890
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1,
891-
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP,
891+
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
892892
.max_lowres = 3,
893893
.pix_fmts = (const enum AVPixelFormat[]) {
894894
AV_PIX_FMT_YUV420P,
@@ -906,7 +906,7 @@ AVCodec ff_msmpeg4v3_decoder = {
906906
.close = ff_h263_decode_end,
907907
.decode = ff_h263_decode_frame,
908908
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1,
909-
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP,
909+
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
910910
.max_lowres = 3,
911911
.pix_fmts = (const enum AVPixelFormat[]) {
912912
AV_PIX_FMT_YUV420P,
@@ -924,7 +924,7 @@ AVCodec ff_wmv1_decoder = {
924924
.close = ff_h263_decode_end,
925925
.decode = ff_h263_decode_frame,
926926
.capabilities = AV_CODEC_CAP_DRAW_HORIZ_BAND | AV_CODEC_CAP_DR1,
927-
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_INIT_CLEANUP,
927+
.caps_internal = FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM,
928928
.max_lowres = 3,
929929
.pix_fmts = (const enum AVPixelFormat[]) {
930930
AV_PIX_FMT_YUV420P,

‎libavcodec/rv10.c‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -687,7 +687,6 @@ AVCodec ff_rv10_decoder = {
687687
.close = rv10_decode_end,
688688
.decode = rv10_decode_frame,
689689
.capabilities = AV_CODEC_CAP_DR1,
690-
.caps_internal = FF_CODEC_CAP_INIT_CLEANUP,
691690
.max_lowres = 3,
692691
.pix_fmts = (const enum AVPixelFormat[]) {
693692
AV_PIX_FMT_YUV420P,
@@ -705,7 +704,6 @@ AVCodec ff_rv20_decoder = {
705704
.close = rv10_decode_end,
706705
.decode = rv10_decode_frame,
707706
.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY,
708-
.caps_internal = FF_CODEC_CAP_INIT_CLEANUP,
709707
.flush = ff_mpeg_flush,
710708
.max_lowres = 3,
711709
.pix_fmts = (const enum AVPixelFormat[]) {

0 commit comments

Comments
 (0)