[FFmpeg-devel] [PATCH] Use AVBufferPool in MOV

[James Almer]

On 11/24/2024 2:31 PM, James Almer wrote:
> On 11/22/2024 2:46 PM, Arnaud Masserann wrote:
>> Most demuxers allocate a new buffer for each packet.
>> For MOV, this can be problematic, because of some high-bitrate
>> codecs like ProRes/HAPQ/NotchLC.
>>
>> Use pools of buffer instead.
>>
>> For some test media, demuxing goes from 20ms/frame to 11ms/frame,
>> close to the perf of ReadFile (10ms/frame), making it possible
>> to read the media at 60Hz.
>>
>> Signed-off-by: Arnaud Masserann <Arnaud1602 at gmail.com>
>> ---
>>   libavformat/avformat.h | 12 ++++++++++++
>>   libavformat/isom.h     |  2 ++
>>   libavformat/mov.c      | 20 ++++++++++++++++++--
>>   libavformat/utils.c    | 29 +++++++++++++++++++++++++++++
>>   4 files changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 56c1c80289..25f2e59b22 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -440,6 +440,18 @@ int av_get_packet(AVIOContext *s, AVPacket *pkt, 
>> int size);
>>    */
>>   int av_append_packet(AVIOContext *s, AVPacket *pkt, int size);
>> +
>> +/**
>> + * Like av_get_packet, but allocates the AVPacket's buffer from a pool.
>> + *
>> + * @param s    associated IO context
>> + * @param pool the pool of buffers; must be initialized with a 
>> sufficient size
>> + * @param pkt packet
>> + * @param size desired payload size
>> + * @return >0 (read size) if OK, AVERROR_xxx otherwise
>> + */
>> +int av_get_pooled_packet(AVIOContext *s, AVBufferPool* pool, AVPacket 
>> *pkt, int size);
> 
> This doesn't need to be public. It can be internal to lavf just fine.
> 
>> +
>>   /*************************************************/
>>   /* input/output formats */
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index 4723397048..c608f30e04 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -273,6 +273,8 @@ typedef struct MOVStreamContext {
>>       } cenc;
>>       struct IAMFDemuxContext *iamf;
>> +
>> +    AVBufferPool* pools[32];
> 
> Like you argued, this can be in MovContext instead, since the pools are 
> fixed size and can be used by any stream.
> 
>>   } MOVStreamContext;
>>   typedef struct HEIFItem {
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index a2333ac1fd..2668bdf78f 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -9583,6 +9583,7 @@ static void 
>> mov_free_encryption_index(MOVEncryptionIndex **index) {
>>   static void mov_free_stream_context(AVFormatContext *s, AVStream *st)
>>   {
>>       MOVStreamContext *sc = st->priv_data;
>> +    int i;
>>       if (!sc || --sc->refcount) {
>>           st->priv_data = NULL;
>> @@ -9639,6 +9640,9 @@ static void 
>> mov_free_stream_context(AVFormatContext *s, AVStream *st)
>>           ff_iamf_read_deinit(sc->iamf);
>>   #endif
>>       av_freep(&sc->iamf);
>> +
>> +    for (i = 0; i < FF_ARRAY_ELEMS(sc->pools); i++)
>> +        av_buffer_pool_uninit(&sc->pools[i]);
>>   }
>>   static int mov_read_close(AVFormatContext *s)
>> @@ -10544,6 +10548,16 @@ static int 
>> mov_finalize_packet(AVFormatContext *s, AVStream *st, AVIndexEntry *s
>>       return 0;
>>   }
>> +static AVBufferPool *mov_pool_get(AVBufferPool* pools[32], int size)
>> +{
>> +    int index = av_log2(size + AV_INPUT_BUFFER_PADDING_SIZE);
>> +    if (!pools[index]) {
>> +        int pool_size = 2 << index;
>> +        pools[index] = av_buffer_pool_init(pool_size, NULL);
>> +    }
>> +    return pools[index];
>> +}
>> +
>>   static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
>>   {
>>       MOVContext *mov = s->priv_data;
>> @@ -10617,8 +10631,10 @@ static int mov_read_packet(AVFormatContext 
>> *s, AVPacket *pkt)
>>                   return FFERROR_REDO;
>>           }
>>   #endif
>> -        else
>> -            ret = av_get_packet(sc->pb, pkt, sample->size);
>> +        else{
>> +            AVBufferPool* pool = mov_pool_get(sc->pools, sample->size);
>> +            ret = av_get_pooled_packet(sc->pb, pool, pkt, sample->size);
>> +        }
>>           if (ret < 0) {
>>               if (should_retry(sc->pb, ret)) {
>>                   mov_current_sample_dec(sc);
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index e9ded627ad..869e97c089 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -104,6 +104,35 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>       return append_packet_chunked(s, pkt, size);
>>   }
>> +int av_get_pooled_packet(AVIOContext *s, AVBufferPool* pool, AVPacket 
>> *pkt, int size)
>> +{
>> +#if FF_API_INIT_PACKET
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +    av_init_packet(pkt);
>> +    pkt->data = NULL;
>> +    pkt->size = 0;
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#else
>> +    av_packet_unref(pkt);
>> +#endif
>> +    pkt->pos  = avio_tell(s);
>> +
>> +    if(!pool)
>> +        return AVERROR(ENOMEM);
>> +
>> +    pkt->buf = av_buffer_pool_get(pool);;
> 
> Extra ;
> 
>> +    if(!pkt->buf)
>> +        return AVERROR(ENOMEM);
>> +
>> +    if(pkt->buf->size < size){
>> +        av_packet_unref(pkt); // TODO test this
>> +        return AVERROR(ENOMEM);
> 
> Maybe EINVAL instead.
> 
> Also, you're not setting pkt->data to pkt->buf->data.

Actually, disregard this part. It's fine as is.

> 
>> +    }
>> +
>> +    return append_packet_chunked(s, pkt, size);
>> +}
>> +
>> +
>>   int av_append_packet(AVIOContext *s, AVPacket *pkt, int size)
>>   {
>>       if (!pkt->size)
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241124/0edb1380/attachment.sig>