[FFmpeg-devel] [PATCH] avcodec/h264_ps: use the AVBufferPool API to allocate parameter set buffers

[James Almer]

On 1/21/2018 7:06 PM, Michael Niedermayer wrote:
> On Sat, Jan 20, 2018 at 11:53:25PM -0300, James Almer wrote:
>> On 1/20/2018 11:33 PM, Michael Niedermayer wrote:
>>> On Sat, Jan 20, 2018 at 06:49:29PM -0300, James Almer wrote:
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>> Similar rationale as hevc. With up to 32 sps and 256 pps, this may
>>>> come in handy when parsing raw streams.
>>>>
>>>>  libavcodec/h264_parser.c   |  3 ++-
>>>>  libavcodec/h264_ps.c       | 22 ++++++++++++++++++++--
>>>>  libavcodec/h264_ps.h       |  5 +++++
>>>>  libavcodec/h264dec.c       |  3 +++
>>>>  libavcodec/mediacodecdec.c |  4 ++++
>>>>  5 files changed, 34 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
>>>> index 65d9d44b50..fa6777fc05 100644
>>>> --- a/libavcodec/h264_parser.c
>>>> +++ b/libavcodec/h264_parser.c
>>>> @@ -697,7 +697,8 @@ static av_cold int init(AVCodecParserContext *s)
>>>>      p->reference_dts = AV_NOPTS_VALUE;
>>>>      p->last_frame_num = INT_MAX;
>>>>      ff_h264dsp_init(&p->h264dsp, 8, 1);
>>>> -    return 0;
>>>> +
>>>> +    return ff_h264_ps_init(&p->ps);
>>>>  }
>>>>  
>>>>  AVCodecParser ff_h264_parser = {
>>>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
>>>> index 8d1ef831fa..50dbabdb8b 100644
>>>> --- a/libavcodec/h264_ps.c
>>>> +++ b/libavcodec/h264_ps.c
>>>> @@ -314,6 +314,21 @@ static int decode_scaling_matrices(GetBitContext *gb, const SPS *sps,
>>>>      return ret;
>>>>  }
>>>>  
>>>> +int ff_h264_ps_init(H264ParamSets *ps)
>>>> +{
>>>> +    ps->sps_pool = av_buffer_pool_init(sizeof(*ps->sps), av_buffer_allocz);
>>>> +    ps->pps_pool = av_buffer_pool_init(sizeof(*ps->pps), av_buffer_allocz);
>>>> +
>>>> +    if (!ps->sps_pool || !ps->pps_pool) {
>>>> +        av_buffer_pool_uninit(&ps->sps_pool);
>>>> +        av_buffer_pool_uninit(&ps->pps_pool);
>>>> +
>>>> +        return AVERROR(ENOMEM);
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  void ff_h264_ps_uninit(H264ParamSets *ps)
>>>>  {
>>>>      int i;
>>>> @@ -327,6 +342,9 @@ void ff_h264_ps_uninit(H264ParamSets *ps)
>>>>      av_buffer_unref(&ps->sps_ref);
>>>>      av_buffer_unref(&ps->pps_ref);
>>>>  
>>>> +    av_buffer_pool_uninit(&ps->sps_pool);
>>>> +    av_buffer_pool_uninit(&ps->pps_pool);
>>>> +
>>>>      ps->pps = NULL;
>>>>      ps->sps = NULL;
>>>>  }
>>>> @@ -341,7 +359,7 @@ int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
>>>>      SPS *sps;
>>>>      int ret;
>>>>  
>>>> -    sps_buf = av_buffer_allocz(sizeof(*sps));
>>>> +    sps_buf = av_buffer_pool_get(ps->sps_pool);
>>>>      if (!sps_buf)
>>>>          return AVERROR(ENOMEM);
>>>>      sps = (SPS*)sps_buf->data;
>>>> @@ -738,7 +756,7 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct
>>>>          return AVERROR_INVALIDDATA;
>>>>      }
>>>>  
>>>> -    pps_buf = av_buffer_allocz(sizeof(*pps));
>>>> +    pps_buf = av_buffer_pool_get(ps->pps_pool);
>>>
>>> this seems to remove the memset(0) unless iam missing something
>>
>> Isn't it enough using av_buffer_allocz() as the alloc function when
>> initializing the pool?
>> If the buffers are not cleared when returned to the pool to be reused
>> then I could change the alloc function to av_buffer_alloc() and call
>> memset(0) here.
> 
> Iam not sure but i dont see what would clear them

Apparently it will fill it with garbage if you configure with memory
poisoning, but aside from that it will not touch its contents. That
kinda makes passing av_buffer_allocz() to av_buffer_pool_init()
absolutely pointless beyond the first time a buffer is returned.

Anyway, just benched it a bit and this was not faster in my tests.
A file with one sps and three pps every keyframe and performance was
within the error margin. Sometimes even slower.
It was a fun exercise with the buffer pool API, but I'll probably drop
this and the h264 patch unless someone has favorable benchmarks for both.