[FFmpeg-devel] [PATCH 31/57] avcodec/mpegpicture: Split MPVPicture into WorkPicture and ordinary Pic

[Andreas Rheinhardt]

Michael Niedermayer:
> On Mon, Apr 29, 2024 at 11:14:12PM +0200, Andreas Rheinhardt wrote:
>> There are two types of MPVPictures: Three (cur_pic, last_pic, next_pic)
>> that are directly part of MpegEncContext and an array of MPVPictures
>> that are separately allocated and are mostly accessed via pointers
>> (cur|last|next)_pic_ptr; they are also used to store AVFrames in the
>> encoder (necessary due to B-frames). As the name implies, each of the
>> former is directly associated with one of the _ptr pointers:
>> They actually share the same underlying buffers, but the ones
>> that are part of the context can have their data pointers offset
>> and their linesize doubled for field pictures.
>>
>> Up until now, each of these had their own references; in particular,
>> there was an underlying av_frame_ref() to sync cur_pic and cur_pic_ptr
>> etc. This is wasteful.
>>
>> This commit changes this relationship: cur_pic, last_pic and next_pic
>> now become MPVWorkPictures; this structure does not have an AVFrame
>> at all any more, but only the cached values of data and linesize.
>> It also contains a pointer to the corresponding MPVPicture, establishing
>> a more natural relationsship between the two.
>> This already means that creating the context-pictures from the pointers
>> can no longer fail.
>>
>> What has not been changed is the fact that the MPVPicture* pointers
>> are not ownership pointers and that the MPVPictures are part of an
>> array of MPVPictures that is owned by a single AVCodecContext.
>> Doing so will be done in a latter commit.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> 
> segfaults
> 
> [vc1 @ 0xa0c81c0] Failed to open codec in avformat_find_stream_info
> ==840294== Invalid read of size 4
> ==840294==    at 0xDB91F4: ff_vc1_mc_4mv_chroma4 (vc1_mc.c:893)
> ==840294==    by 0xDA4FD8: vc1_decode_p_mb_intfr (vc1_block.c:1661)
> ==840294==    by 0xDAD9CD: vc1_decode_p_blocks (vc1_block.c:2831)
> ==840294==    by 0xDAE6A4: ff_vc1_decode_blocks (vc1_block.c:2996)
> ==840294==    by 0xDC5EFB: vc1_decode_frame (vc1dec.c:1304)
> ==840294==    by 0x99B6D2: decode_simple_internal (decode.c:412)
> ==840294==    by 0x99BBFB: decode_simple_receive_frame (decode.c:582)
> ==840294==    by 0x99BD7F: decode_receive_frame_internal (decode.c:611)
> ==840294==    by 0x99C135: avcodec_send_packet (decode.c:702)
> ==840294==    by 0x66BE65: try_decode_frame (demux.c:2156)
> ==840294==    by 0x66EB84: avformat_find_stream_info (demux.c:2840)
> ==840294==    by 0x24F991: ifile_open (ffmpeg_demux.c:1707)
> ==840294==    by 0x278387: open_files (ffmpeg_opt.c:1206)
> ==840294==    by 0x27854A: ffmpeg_parse_options (ffmpeg_opt.c:1246)
> ==840294==    by 0x28CBF7: main (ffmpeg.c:941)
> ==840294==  Address 0x9c is not stack'd, malloc'd or (recently) free'd
> 
> [...]

When I ported this code, I presumed that these frames need to exist (all
the time, not only for valid input) because otherwise the code would use
data of a non-existent frame. But I overlooked the check below that you
added in 6c4516d0413ea9b2a9b48fb83d0ba0ef7bc84f92. (Presumably you
tested with this sample?)
Thanks for testing. I will fix this.

- Andreas