[FFmpeg-devel] About avpicture_alloc (deprecate?)

[Cyril Russo]

Le 02/05/2010 19:45, Reimar D?ffinger a ?crit :
> On Sun, May 02, 2010 at 07:31:44PM +0200, Cyril Russo wrote:
>    
>>      
>> As soon as you use swscale, you need it.
>> avcodec_alloc_frame allocate the frame structure but not the "data"
>> buffer, while avpicture_alloc does.
>>
>> So unless you want people starting to allocate data[0] et al, by
>> themselves (and doing bad thing with alignment since it's very hard
>> to find out the required stride for decoder, see issue1909)
>>      
> The get_buffer functions are for allocating the data, and their
> documentation also explains how to get proper values _if_ you want
> to implement them yourselves (which is a good idea if performance is
> important).
>    
The fact is when you're using swscale, you need an allocated picture 
that is likely not in the same format of the decoded picture.
You can't call codecContext.get_buffer as it'll allocate (or reuse, the 
doc doesn't say) a picture in the format expected by the codec.

Let's take an example then, you want to convert YUV420P to RGB24.
How do you allocate the RGB24 picture without avpicture_alloc (and make 
sure about the alignement requirement of swscale's algorithms) ?



>    
>> That is:
>>      AVFrame frame;
>>      avcodec_decode_video2(context,&frame, .... // Might crash, it's
>> confusing since the doc might say that it allocates frame
>>      
> What do you mean by "doc might say"?
> The documentation says
> "sizeof(AVFrame) must not be used outside libav*."
>    
Please refer to the other discussion about adding a message in 
avcodec_decode_video about allocation.
The issue here is, as you've said later, stack alignment (and there is 
no documentation written about the required stack alignment).


> I can understand that this maybe is easily overlooked, and maybe we
> need a "common but often overlooked design considerations" that
> mention this and padding requirements and some other things, but it
> is documented in principle.
> Btw. it only ever breaks when compiling against a different libavcodec
> version than the version that is actually used in the end.
>
>    
Or using a different compiler with the same version. Not everyone use 
GCC to link with GCC compiled libavcodec.

>> But this leaks:
>>      AVFrame frame;
>>      avpicture_alloc((AVPicture*)&frame, pix_fmt, width, height);
>>      avcodec_decode_video2(context,&frame, .... // Leaks
>>      
> Well, the cast there might be a hint something's not right with this.
> Worse, it still has exactly the same issues as the previous code,
> it just leaks memory in addition.
> Actually that is the only different that the avpicture_alloc makes (though
> by chance it might "fix" things by changing the stack layout).
>    
I agree. That's the point of the other discussion about the 
documentation stating "don't do that".
>    
>> For me, the option would probably to render picture&  frame not
>> binary compatible, so casting from any other is not possible. That
>> way, when you deal with a AVFrame, you call x_frame functions, and
>> when you deal with picture, you call x_picture functions.
>>      
> I fail to see the point of it. Particularly since I doubt whether many
> if any of the imgconvert.c functions should still be there (non-deprecated).
>    
There is no picture allocation function in swscale. So if you deprecate 
this, please create equivalent functions in swscale.
IMHO, it would be a large regression if we had to do the pic.data[] 
malloc by ourselves like it's done in api-example.c

Best regards,
Cyril