[FFmpeg-devel] [PATCH] avcodec/decode: copy the output parameters from the last bsf in the chain back to the AVCodecContext

[James Almer]

On 7/21/2018 2:34 PM, Michael Niedermayer wrote:
> On Fri, Jul 20, 2018 at 08:30:19PM -0300, James Almer wrote:
>> On 7/20/2018 8:23 PM, Michael Niedermayer wrote:
>>> On Thu, Jul 19, 2018 at 10:54:23PM -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 at gmail.com>
>>>> ---
>>>> Things worked fine without this until now by pure chance. vp9_superframe_split
>>>> doesn't use extradata, and those hardware decoders inserting h264_mp4toannexb
>>>> and hevc_mp4toannexb passed the extradata to functions that could handle it
>>>> unfiltered.
>>>>
>>>>  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 -
>>> ...
>>> [apng @ 0x2d431c0] chunk too big
>>> [apng @ 0x2d44380] chunk too big
>>> [apng @ 0x2d45b40] chunk too big
>>> [apng @ 0x2d472c0] chunk too big
>>> [apng @ 0x2d48a40] chunk too big
>>> [apng @ 0x2d4a1c0] chunk too big
>>> [apng @ 0x2d4b940] chunk too big
>>> [apng @ 0x2d4d0c0] chunk too big
>>> [apng @ 0x2d4e840] chunk too big
>>> [apng @ 0x2d4ffc0] chunk too big
>>> [apng @ 0x2d5fc00] chunk too big
>>> [apng @ 0x2d612c0] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>> [apng @ 0x2d62900] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>> [apng @ 0x2d431c0] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>> [apng @ 0x2d44380] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>> [apng @ 0x2d45b40] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>> [apng @ 0x2d472c0] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>> [apng @ 0x2d48a40] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>> [apng @ 0x2d4a1c0] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>> [apng @ 0x2d4b940] chunk too big
>>> Error while decoding stream #0:0: Invalid data found when processing input
>>
>> I can't reproduce this. Are you sure it was this patch and not some
>> other change in your tree?
> 
> this patch seems to trigger the crash and or some anomalies
> valgrind output below.
> The exact error differs between valgrind and no valgrind
> 
> 
> ==3661== Syscall param sendmsg(mmsg[0].msg_hdr) points to uninitialised byte(s)
> ==3661==    at 0xC7647D9: sendmmsg (sendmmsg.c:32)
> ==3661==    by 0x113FFAB0: __libc_res_nsend (res_send.c:1279)
> ==3661==    by 0x113FCE2B: __libc_res_nquery (res_query.c:227)
> ==3661==    by 0x113FD862: __libc_res_nsearch (res_query.c:591)
> ==3661==    by 0x11C1CC72: _nss_dns_gethostbyname4_r (dns-host.c:315)
> ==3661==    by 0xC735665: gaih_inet (getaddrinfo.c:858)
> ==3661==    by 0xC73896C: getaddrinfo (getaddrinfo.c:2414)
> ==3661==    by 0x7D29C5: tcp_open (tcp.c:116)
> ==3661==    by 0x67A36C: ffurl_connect (avio.c:209)
> ==3661==    by 0x67AA81: ffurl_open_whitelist (avio.c:345)
> ==3661==    by 0x83F149: ff_tls_open_underlying (tls.c:107)
> ==3661==    by 0x7D7D78: tls_open (tls_openssl.c:239)
> ==3661==    by 0x67A346: ffurl_connect (avio.c:209)
> ==3661==    by 0x67AA81: ffurl_open_whitelist (avio.c:345)
> ==3661==    by 0x6C957B: http_open_cnx_internal (http.c:235)
> ==3661==    by 0x6C9680: http_open_cnx (http.c:262)
> ==3661==    by 0x6CA560: http_open (http.c:557)
> ==3661==    by 0x67A346: ffurl_connect (avio.c:209)
> ==3661==    by 0x67AA81: ffurl_open_whitelist (avio.c:345)
> ==3661==    by 0x67E88D: ffio_open_whitelist (aviobuf.c:1167)
> ==3661==  Address 0x7feff9260 is on thread 1's stack
> ==3661==  Uninitialised value was created by a stack allocation
> ==3661==    at 0x113FEE6D: __libc_res_nsend (res_send.c:364)
> ==3661== 
> 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
> ==3661== Thread 2:
> ==3661== Invalid read of size 4
> ==3661==    at 0xB1081F: bytestream_get_be32 (bytestream.h:92)
> ==3661==    by 0xB10841: bytestream2_get_be32u (bytestream.h:92)
> ==3661==    by 0xB1088F: bytestream2_get_be32 (bytestream.h:92)
> ==3661==    by 0xB14FA0: decode_frame_common (pngdec.c:1196)
> ==3661==    by 0xB1600B: decode_frame_apng (pngdec.c:1490)
> ==3661==    by 0xB25DA8: frame_worker_thread (pthread_frame.c:201)
> ==3661==    by 0xC44F183: start_thread (pthread_create.c:312)
> ==3661==    by 0xC76303C: clone (clone.S:111)
> ==3661==  Address 0x1171d6c0 is 0 bytes inside a block of size 109 free'd
> ==3661==    at 0x4C2B5D9: free (vg_replace_malloc.c:446)
> ==3661==    by 0x1218592: av_free (mem.c:223)
> ==3661==    by 0x12185CA: av_freep (mem.c:233)
> ==3661==    by 0xC004FE: avcodec_parameters_to_context (utils.c:2126)
> ==3661==    by 0x8AD34E: bsfs_init (decode.c:284)
> ==3661==    by 0x8AE729: avcodec_send_packet (decode.c:695)
> ==3661==    by 0x43383E: decode (ffmpeg.c:2238)
> ==3661==    by 0x434098: decode_video (ffmpeg.c:2382)
> ==3661==    by 0x4350E0: process_input_packet (ffmpeg.c:2623)
> ==3661==    by 0x43C536: process_input (ffmpeg.c:4505)
> ==3661==    by 0x43CADF: transcode_step (ffmpeg.c:4625)
> ==3661==    by 0x43CC5B: transcode (ffmpeg.c:4679)
> ==3661==    by 0x43D4C8: main (ffmpeg.c:4886)

Data race, it seems. Looking at bsfs_init() again, i see it's called
from avcodec_send_packet(), which is long after the codec is
initialized. So i guess copying the filtered parameters back to the
AVCodecContext is not an option. Or at least not unless the decoder sets
AV_CODEC_CAP_PARAM_CHANGE.

This is a problem, or will be once a decoder needs to autoinsert a bsf
that makes a change that needs to be reflected in the codec context.
It may be a good idea (or even necessary) to see how feasible it is to
call this function from avcodec_open2() instead.

Patch dropped in any case.