[FFmpeg-devel] [RFC] Avoid av_read_frame memory copy in implementation

[Cyril Russo]

Le 26/05/2010 20:10, Michael Niedermayer a ?crit :
> On Wed, May 26, 2010 at 04:45:16PM +0200, Jean-Daniel Dupas wrote:
>    
>> Le 26 mai 2010 ? 16:39, Ronald S. Bultje a ?crit :
>>
>>      
>>> Hi,
>>>
>>> On Wed, May 26, 2010 at 7:06 AM, Cyril Russo
>>> <stage.nexvision at laposte.net>  wrote:
>>>        
>>>> Currently, one must allocate another buffer to store the returned packet's
>>>> data and copy the data, because initial memory is reused when calling
>>>> av_read_frame again.
>>>>          
> wrong
>    
My tests shows that it's the case (at least for AAC + H264 in mp4).
The pseudo code is:

AVPacket packet[10];
foreach packet in array: av_read_frame(packet[i])
[... later ...]
avcodec_decode_[...](packet[i]) => corruption / crash in avcodec_decode

And the documentation says so.

>
>    
>>> Huh? That sounds like a bug, demuxers freshly allocate memory for each packet.
>>>        
> most do, after parsers it can be reused memory though, thats a complicated
> issue as one demuxer packet can end up being used for several parsed packets
> or a "static" buffer could contain data from several demuxer packets.
> its theoreticall possibly to reduce the coping outside libavformat for these
> if we could keep track of all the packets refering to packets, but that would
> then also require thread sync and become somewhat complex.
>    
Which demuxer use threads currently ? Is is absolute requirement ?
If not, then the complexity seems to be in the number of demuxer to 
changes, am I wrong ?

> Either way a patch that improves the performance is certainly welcome
> if reasonable clean ...
>
> also a patch making the memory allocation for packets user overrideable is
> not unwelcome if it speed things up.
>    
While thinking about it, I don't think it'll speed things up at first.
It'll move the malloc + memcpy into FFmpeg library, but in the long run, 
demuxer could catch up to avoid this useless memcpy (and thus allow this 
possible optimization).
Also that would make user code easier to write and maintain.


>
>    
>>>        
>> It's not what the doc of av_read_frame says:
>>
>> ?The returned packet is valid until the next av_read_frame() or until av_close_input_file() and must be freed with av_free_packet.?
>>      
> correct
>
> also see av_dup_packet()
> which makes sure you have a lasting packet
>
>    
av_dup_packet is exactly what we(users) are using currently, but it's 
doing malloc + memcpy (which I would like to avoid).
On a 20MBits/s stream, it's would be a significant saving anyway.

Best regards,
Cyril Russo