Re: [FFmpeg-devel] [PATCH 8/8] avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext
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 >> --- >> 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 8/8] avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext
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 > --- > 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 1 "Used only once"- "Some unspecified defect prevented a second use" "In good condition" - "Can be repaird by experienced expert" "As is" - "You wouldnt want it even if you were payed for it, if you knew ..." signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 8/8] avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext
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
---
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.
From f0e852857b3d218ca5e483ac47df8cb58ff2a362 Mon Sep 17 00:00:00 2001
From: James Almer
Date: Thu, 26 Jul 2018 20:42:27 -0300
Subject: [PATCH 7/8 v2] avcodec/decode: flush the internal bsfs instead of
constantly
reinitalizing them
Initialize the bsfs once when opening the codec and uninitialize them once when
closing it, instead of at every codec flush/seek.
Signed-off-by: James Almer
---
libavcodec/decode.c | 20 ++--
libavcodec/decode.h | 2 ++
libavcodec/utils.c | 7 +++
3 files changed, 19 insertions(+), 10 deletions(-)
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 = >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 = >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);
+
Re: [FFmpeg-devel] [PATCH 8/8] avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext
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 > >> --- > >> 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) -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 8/8] avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext
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 >> --- >> 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 8/8] avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext
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 > --- > 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 - libavutil 56. 18.102 / 56. 18.102 libavcodec 58. 22.100 / 58. 22.100 libavformat58. 17.101 / 58. 17.101 libavdevice58. 4.101 / 58. 4.101 libavfilter 7. 26.100 / 7. 26.100 libswscale 5. 2.100 / 5. 2.100 libswresample 3. 2.100 / 3. 2.100 libpostproc55. 2.100 / 55. 2.100 ==7203== Syscall param sendmsg(mmsg[0].msg_hdr) points to uninitialised byte(s) ==7203==at 0xF5667D9: sendmmsg (sendmmsg.c:32) ==7203==by 0x1FF88AB0: __libc_res_nsend (res_send.c:1279) ==7203==by 0x1FF85E2B: __libc_res_nquery (res_query.c:227) ==7203==by 0x1FF86862: __libc_res_nsearch (res_query.c:591) ==7203==by 0x25B07C72: _nss_dns_gethostbyname4_r (dns-host.c:315) ==7203==by 0xF537665: gaih_inet (getaddrinfo.c:858) ==7203==by 0xF53A96C: getaddrinfo (getaddrinfo.c:2414) ==7203==by 0x783CAC: tcp_open (in ffmpeg/ffmpeg_g) ==7203==by 0x6710D5: ffurl_connect (in ffmpeg/ffmpeg_g) ==7203==by 0x6717CC: ffurl_open_whitelist (in ffmpeg/ffmpeg_g) ==7203==by 0x7D8C23: ff_tls_open_underlying (in ffmpeg/ffmpeg_g) ==7203==by 0x786BBA: tls_open (in ffmpeg/ffmpeg_g) ==7203==by 0x670FBE: ffurl_connect (in ffmpeg/ffmpeg_g) ==7203==by 0x6717CC: ffurl_open_whitelist (in ffmpeg/ffmpeg_g) ==7203==by 0x6B85AC: http_open_cnx_internal (in ffmpeg/ffmpeg_g) ==7203==by 0x6B87FA: http_open_cnx (in ffmpeg/ffmpeg_g) ==7203==by 0x6B9A18: http_open (in ffmpeg/ffmpeg_g) ==7203==by 0x670FBE: ffurl_connect (in ffmpeg/ffmpeg_g) ==7203==by 0x6717CC: ffurl_open_whitelist (in ffmpeg/ffmpeg_g) ==7203==by 0x67AF2F: ffio_open_whitelist (in ffmpeg/ffmpeg_g) ==7203== Address 0x7feff7ff0 is on thread 1's stack ==7203== Uninitialised value was created by a stack allocation ==7203==at 0x1FF87E6D: __libc_res_nsend (res_send.c:364) ==7203== Input #0, apng, from 'https://upload.wikimedia.org/wikipedia/commons/1/14/Animated_PNG_example_bouncing_beach_ball.png': Duration: N/A, bitrate: N/A Stream #0:0: Video: apng, rgba(pc), 100x100, 13.33 fps, 13.33 tbr, 100k tbn, 100k tbc Stream mapping: Stream #0:0 -> #0:0 (apng (native) -> rawvideo (native)) Press [q] to stop, [?] for help ==7203== Thread 2: ==7203== Invalid read of size 4 ==7203==at 0xA43816: decode_frame_common.isra.9 (in ffmpeg/ffmpeg_g) ==7203==by 0xA46C37: decode_frame_apng (in ffmpeg/ffmpeg_g) ==7203==by 0xA544B0: frame_worker_thread (in ffmpeg/ffmpeg_g) ==7203==by 0xB8A2183: start_thread (pthread_create.c:312) ==7203==by 0xF56503C: clone (clone.S:111) ==7203== Address 0x256136c0 is 0 bytes inside a block of size 109 free'd ==7203==at 0x4C2B5D9: free (vg_replace_malloc.c:446) ==7203==by 0xB3362F: avcodec_parameters_to_context (in ffmpeg/ffmpeg_g) ==7203==by 0x829EC8: ff_decode_bsfs_init (in ffmpeg/ffmpeg_g) ==7203==by 0xB3044A: avcodec_open2 (in ffmpeg/ffmpeg_g) ==7203==by 0x4CA18C: transcode_init (in ffmpeg/ffmpeg_g) ==7203==by 0x4CC6F8: transcode (in ffmpeg/ffmpeg_g) ==7203==by 0x4AACDC: main (in ffmpeg/ffmpeg_g) ==7203== ==7203== Invalid read of size 4 ==7203==at 0xA4384A: decode_frame_common.isra.9 (in ffmpeg/ffmpeg_g) ==7203==by 0xA46C37: decode_frame_apng (in ffmpeg/ffmpeg_g) ==7203==by 0xA544B0: frame_worker_thread (in ffmpeg/ffmpeg_g) ==7203==by 0xB8A2183: start_thread (pthread_create.c:312) ==7203==by 0xF56503C: clone (clone.S:111) ==7203== Address 0x256136c4 is 4 bytes inside a block of size 109 free'd ==7203==at 0x4C2B5D9: free (vg_replace_malloc.c:446) ==7203==by 0xB3362F: avcodec_parameters_to_context (in ffmpeg/ffmpeg_g) ==7203==by 0x829EC8: ff_decode_bsfs_init (in ffmpeg/ffmpeg_g) ==7203==by 0xB3044A: avcodec_open2 (in ffmpeg/ffmpeg_g) ==7203==by 0x4CA18C: transcode_init (in ffmpeg/ffmpeg_g) ==7203==by 0x4CC6F8: transcode (in ffmpeg/ffmpeg_g) ==7203==by 0x4AACDC: main (in ffmpeg/ffmpeg_g) ==7203== ==7203== Invalid read of size 4 ==7203==at 0xA44E8E: decode_frame_common.isra.9 (in ffmpeg/ffmpeg_g) ==7203==by 0xA46C37: decode_frame_apng (in ffmpeg/ffmpeg_g) ==7203==by 0xA544B0: frame_worker_thread (in ffmpeg/ffmpeg_g) ==7203==by 0xB8A2183: start_thread (pthread_create.c:312) ==7203==by 0xF56503C: clone (clone.S:111) ==7203== Address 0x256136c8 is 8 bytes inside a block of size 109 free'd ==7203==at 0x4C2B5D9: free (vg_replace_malloc.c:446) ==7203==by 0xB3362F: avcodec_parameters_to_context (in
[FFmpeg-devel] [PATCH 8/8] avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext
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 --- Now it's not going to be called after the codec has been opened. libavcodec/decode.c | 4 1 file changed, 4 insertions(+) diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 2e82f6b506..4607e9f318 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -281,6 +281,10 @@ int ff_decode_bsfs_init(AVCodecContext *avctx) bsfs_str++; } +ret = avcodec_parameters_to_context(avctx, s->bsfs[s->nb_bsfs - 1]->par_out); +if (ret < 0) +return ret; + return 0; fail: ff_decode_bsfs_uninit(avctx); -- 2.18.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
