Message ID | b7206251-2fb4-93da-ead5-df55145f5fc2@gmail.com |
---|---|
State | Accepted |
Commit | 94fe138de0ba5892a7051f5b47c191a41b78805a |
Headers | show |
On Sat, Jul 28, 2018 at 01:25:36PM -0300, James Almer wrote: > On 7/28/2018 4:09 AM, Michael Niedermayer wrote: > > On Fri, Jul 27, 2018 at 11:11:47PM -0300, James Almer wrote: > >> On 7/27/2018 10:58 PM, Michael Niedermayer wrote: > >>> On Fri, Jul 27, 2018 at 11:57:49AM -0300, James Almer wrote: > >>>> Certain AVCodecParameters, like the contents of the extradata, may be changed > >>>> by the init() function of any of the bitstream filters in the chain. > >>>> > >>>> Signed-off-by: James Almer <jamrial@gmail.com> > >>>> --- > >>>> Now it's not going to be called after the codec has been opened. > >>>> > >>>> libavcodec/decode.c | 4 ++++ > >>>> 1 file changed, 4 insertions(+) > >>> > >>> This breaks: > >>> ffmpeg -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -bitexact -pix_fmt rgba -f framecrc - > >> > >> Is any other decoder failing the same way? Because the apng decoder > >> threading code may be faulty otherwise. Plenty of avctx fields are > >> changed after ff_thread_init() is called within avcodec_open2(). There > >> should not be a race at this point. > > > > I found a failure with mpeg4 (with bframes) decoding + pcm_alaw from mkv but it > > does not want to reproduce. The slightest change i do makes this not happen > > even just duplicating a command line parameter (which should have no effect) > > simply adding the -threads parameter to it independant of value makes it go away > > too > > > > > > in the png case > > this hits teh issue: > > -threads 2 -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -f framecrc - > > > > this does not: > > -threads 1 -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -f framecrc - > > > > also odly the bitexact flag made a differnce in how it fails outside valgrind > > last i tried. (doesnt make a difference in valgrind it seems) > > A solution may be moving the ff_decode_bsfs_init call in patch 7/8 right > above the call to ff_thread_init (See attachment), hopefully preventing > this race once this patch is applied afterwards, but it will result in > the bsfs initialized before the decoder, and some of the avctx fields > that are changed later in avcodec_open2 like channels and bit_rate not > being reflected during said bsfs initialization. > I can't say if the former is correct or ideal, but for now the latter > would not be an issue. I don't know what may happen if we were to > autoinsert a filter that does care about such fields in the future, though. > > If the above change doesn't solve it, or is not ideal, then this patch > 8/8 can be dropped or postponed, and the rest of the set pushed without it. with this patch, the pachset seems not to trigger these errors anymore thanks [...]
On 7/28/2018 6:28 PM, Michael Niedermayer wrote: > On Sat, Jul 28, 2018 at 01:25:36PM -0300, James Almer wrote: >> On 7/28/2018 4:09 AM, Michael Niedermayer wrote: >>> On Fri, Jul 27, 2018 at 11:11:47PM -0300, James Almer wrote: >>>> On 7/27/2018 10:58 PM, Michael Niedermayer wrote: >>>>> On Fri, Jul 27, 2018 at 11:57:49AM -0300, James Almer wrote: >>>>>> Certain AVCodecParameters, like the contents of the extradata, may be changed >>>>>> by the init() function of any of the bitstream filters in the chain. >>>>>> >>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>> --- >>>>>> Now it's not going to be called after the codec has been opened. >>>>>> >>>>>> libavcodec/decode.c | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> This breaks: >>>>> ffmpeg -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -bitexact -pix_fmt rgba -f framecrc - >>>> >>>> Is any other decoder failing the same way? Because the apng decoder >>>> threading code may be faulty otherwise. Plenty of avctx fields are >>>> changed after ff_thread_init() is called within avcodec_open2(). There >>>> should not be a race at this point. >>> >>> I found a failure with mpeg4 (with bframes) decoding + pcm_alaw from mkv but it >>> does not want to reproduce. The slightest change i do makes this not happen >>> even just duplicating a command line parameter (which should have no effect) >>> simply adding the -threads parameter to it independant of value makes it go away >>> too >>> >>> >>> in the png case >>> this hits teh issue: >>> -threads 2 -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -f framecrc - >>> >>> this does not: >>> -threads 1 -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -f framecrc - >>> >>> also odly the bitexact flag made a differnce in how it fails outside valgrind >>> last i tried. (doesnt make a difference in valgrind it seems) >> >> A solution may be moving the ff_decode_bsfs_init call in patch 7/8 right >> above the call to ff_thread_init (See attachment), hopefully preventing >> this race once this patch is applied afterwards, but it will result in >> the bsfs initialized before the decoder, and some of the avctx fields >> that are changed later in avcodec_open2 like channels and bit_rate not >> being reflected during said bsfs initialization. >> I can't say if the former is correct or ideal, but for now the latter >> would not be an issue. I don't know what may happen if we were to >> autoinsert a filter that does care about such fields in the future, though. >> >> If the above change doesn't solve it, or is not ideal, then this patch >> 8/8 can be dropped or postponed, and the rest of the set pushed without it. > > with this patch, the pachset seems not to trigger these errors anymore > > thanks Set pushed. Thanks.
On Fri, Aug 17, 2018 at 12:07:56AM -0300, James Almer wrote: > On 7/28/2018 6:28 PM, Michael Niedermayer wrote: > > On Sat, Jul 28, 2018 at 01:25:36PM -0300, James Almer wrote: > >> On 7/28/2018 4:09 AM, Michael Niedermayer wrote: > >>> On Fri, Jul 27, 2018 at 11:11:47PM -0300, James Almer wrote: > >>>> On 7/27/2018 10:58 PM, Michael Niedermayer wrote: > >>>>> On Fri, Jul 27, 2018 at 11:57:49AM -0300, James Almer wrote: > >>>>>> Certain AVCodecParameters, like the contents of the extradata, may be changed > >>>>>> by the init() function of any of the bitstream filters in the chain. > >>>>>> > >>>>>> Signed-off-by: James Almer <jamrial@gmail.com> > >>>>>> --- > >>>>>> Now it's not going to be called after the codec has been opened. > >>>>>> > >>>>>> libavcodec/decode.c | 4 ++++ > >>>>>> 1 file changed, 4 insertions(+) > >>>>> > >>>>> This breaks: > >>>>> ffmpeg -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -bitexact -pix_fmt rgba -f framecrc - > >>>> > >>>> Is any other decoder failing the same way? Because the apng decoder > >>>> threading code may be faulty otherwise. Plenty of avctx fields are > >>>> changed after ff_thread_init() is called within avcodec_open2(). There > >>>> should not be a race at this point. > >>> > >>> I found a failure with mpeg4 (with bframes) decoding + pcm_alaw from mkv but it > >>> does not want to reproduce. The slightest change i do makes this not happen > >>> even just duplicating a command line parameter (which should have no effect) > >>> simply adding the -threads parameter to it independant of value makes it go away > >>> too > >>> > >>> > >>> in the png case > >>> this hits teh issue: > >>> -threads 2 -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -f framecrc - > >>> > >>> this does not: > >>> -threads 1 -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -f framecrc - > >>> > >>> also odly the bitexact flag made a differnce in how it fails outside valgrind > >>> last i tried. (doesnt make a difference in valgrind it seems) > >> > >> A solution may be moving the ff_decode_bsfs_init call in patch 7/8 right > >> above the call to ff_thread_init (See attachment), hopefully preventing > >> this race once this patch is applied afterwards, but it will result in > >> the bsfs initialized before the decoder, and some of the avctx fields > >> that are changed later in avcodec_open2 like channels and bit_rate not > >> being reflected during said bsfs initialization. > >> I can't say if the former is correct or ideal, but for now the latter > >> would not be an issue. I don't know what may happen if we were to > >> autoinsert a filter that does care about such fields in the future, though. > >> > >> If the above change doesn't solve it, or is not ideal, then this patch > >> 8/8 can be dropped or postponed, and the rest of the set pushed without it. > > > > with this patch, the pachset seems not to trigger these errors anymore > > > > thanks > > Set pushed. Thanks. > Just so you know, f631c328e680a3dd491936b92f69970c20cdcfc7 causes Firefox's media playback thread to crash. Just try playing any local or remote mp4 file. The crash is reproducible. Tested and bisected with FF Developer Edition (64-bit linux).
On 8/23/2018 2:34 PM, Mohammad_AlSaleh wrote: > On Fri, Aug 17, 2018 at 12:07:56AM -0300, James Almer wrote: >> On 7/28/2018 6:28 PM, Michael Niedermayer wrote: >>> On Sat, Jul 28, 2018 at 01:25:36PM -0300, James Almer wrote: >>>> On 7/28/2018 4:09 AM, Michael Niedermayer wrote: >>>>> On Fri, Jul 27, 2018 at 11:11:47PM -0300, James Almer wrote: >>>>>> On 7/27/2018 10:58 PM, Michael Niedermayer wrote: >>>>>>> On Fri, Jul 27, 2018 at 11:57:49AM -0300, James Almer wrote: >>>>>>>> Certain AVCodecParameters, like the contents of the extradata, may be changed >>>>>>>> by the init() function of any of the bitstream filters in the chain. >>>>>>>> >>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>>>> --- >>>>>>>> Now it's not going to be called after the codec has been opened. >>>>>>>> >>>>>>>> libavcodec/decode.c | 4 ++++ >>>>>>>> 1 file changed, 4 insertions(+) >>>>>>> >>>>>>> This breaks: >>>>>>> ffmpeg -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -bitexact -pix_fmt rgba -f framecrc - >>>>>> >>>>>> Is any other decoder failing the same way? Because the apng decoder >>>>>> threading code may be faulty otherwise. Plenty of avctx fields are >>>>>> changed after ff_thread_init() is called within avcodec_open2(). There >>>>>> should not be a race at this point. >>>>> >>>>> I found a failure with mpeg4 (with bframes) decoding + pcm_alaw from mkv but it >>>>> does not want to reproduce. The slightest change i do makes this not happen >>>>> even just duplicating a command line parameter (which should have no effect) >>>>> simply adding the -threads parameter to it independant of value makes it go away >>>>> too >>>>> >>>>> >>>>> in the png case >>>>> this hits teh issue: >>>>> -threads 2 -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -f framecrc - >>>>> >>>>> this does not: >>>>> -threads 1 -i https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png -f framecrc - >>>>> >>>>> also odly the bitexact flag made a differnce in how it fails outside valgrind >>>>> last i tried. (doesnt make a difference in valgrind it seems) >>>> >>>> A solution may be moving the ff_decode_bsfs_init call in patch 7/8 right >>>> above the call to ff_thread_init (See attachment), hopefully preventing >>>> this race once this patch is applied afterwards, but it will result in >>>> the bsfs initialized before the decoder, and some of the avctx fields >>>> that are changed later in avcodec_open2 like channels and bit_rate not >>>> being reflected during said bsfs initialization. >>>> I can't say if the former is correct or ideal, but for now the latter >>>> would not be an issue. I don't know what may happen if we were to >>>> autoinsert a filter that does care about such fields in the future, though. >>>> >>>> If the above change doesn't solve it, or is not ideal, then this patch >>>> 8/8 can be dropped or postponed, and the rest of the set pushed without it. >>> >>> with this patch, the pachset seems not to trigger these errors anymore >>> >>> thanks >> >> Set pushed. Thanks. >> > > Just so you know, f631c328e680a3dd491936b92f69970c20cdcfc7 > causes Firefox's media playback thread to crash. > > Just try playing any local or remote mp4 file. The crash is > reproducible. > > Tested and bisected with FF Developer Edition (64-bit linux). Can you give us some more details, and a gdb output? And please open a ticket in http://trac.ffmpeg.org/ for this.
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index db364ca700..2e82f6b506 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -182,7 +182,7 @@ static int unrefcount_frame(AVCodecInternal *avci, AVFrame *frame) return 0; } -static int bsfs_init(AVCodecContext *avctx) +int ff_decode_bsfs_init(AVCodecContext *avctx) { AVCodecInternal *avci = avctx->internal; DecodeFilterContext *s = &avci->filter; @@ -688,10 +688,6 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke if (avpkt && !avpkt->size && avpkt->data) return AVERROR(EINVAL); - ret = bsfs_init(avctx); - if (ret < 0) - return ret; - av_packet_unref(avci->buffer_pkt); if (avpkt && (avpkt->data || avpkt->side_data_elems)) { ret = av_packet_ref(avci->buffer_pkt, avpkt); @@ -751,10 +747,6 @@ int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *fr if (!avcodec_is_open(avctx) || !av_codec_is_decoder(avctx->codec)) return AVERROR(EINVAL); - ret = bsfs_init(avctx); - if (ret < 0) - return ret; - if (avci->buffer_frame->buf[0]) { av_frame_move_ref(frame, avci->buffer_frame); } else { @@ -1978,6 +1970,14 @@ int ff_reget_buffer(AVCodecContext *avctx, AVFrame *frame) return ret; } +static void bsfs_flush(AVCodecContext *avctx) +{ + DecodeFilterContext *s = &avctx->internal->filter; + + for (int i = 0; i < s->nb_bsfs; i++) + av_bsf_flush(s->bsfs[i]); +} + void avcodec_flush_buffers(AVCodecContext *avctx) { avctx->internal->draining = 0; @@ -1998,7 +1998,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx) avctx->pts_correction_last_pts = avctx->pts_correction_last_dts = INT64_MIN; - ff_decode_bsfs_uninit(avctx); + bsfs_flush(avctx); if (!avctx->refcounted_frames) av_frame_unref(avctx->internal->to_free); diff --git a/libavcodec/decode.h b/libavcodec/decode.h index 15271c529a..c3e0e82f4c 100644 --- a/libavcodec/decode.h +++ b/libavcodec/decode.h @@ -64,6 +64,8 @@ typedef struct FrameDecodeData { */ int ff_decode_get_packet(AVCodecContext *avctx, AVPacket *pkt); +int ff_decode_bsfs_init(AVCodecContext *avctx); + void ff_decode_bsfs_uninit(AVCodecContext *avctx); /** diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 4f9a2b76ef..285bfdbc63 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -727,6 +727,12 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code goto free_and_end; } + if (av_codec_is_decoder(avctx->codec)) { + ret = ff_decode_bsfs_init(avctx); + if (ret < 0) + goto free_and_end; + } + if (HAVE_THREADS && !(avctx->internal->frame_thread_encoder && (avctx->active_thread_type&FF_THREAD_FRAME))) { ret = ff_thread_init(avctx); @@ -1032,6 +1038,7 @@ FF_ENABLE_DEPRECATION_WARNINGS av_packet_free(&avctx->internal->last_pkt_props); av_packet_free(&avctx->internal->ds.in_pkt); + ff_decode_bsfs_uninit(avctx); av_freep(&avctx->internal->pool); }