diff mbox series

[FFmpeg-devel] avformat/mpegts: use a buffer pool for PES payloads

Message ID 20200329210756.4865-1-jamrial@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] avformat/mpegts: use a buffer pool for PES payloads
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

James Almer March 29, 2020, 9:07 p.m. UTC
This produces a small speed up in demuxing

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/mpegts.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

James Almer April 2, 2020, 5:12 p.m. UTC | #1
On 3/29/2020 6:07 PM, James Almer wrote:
> This produces a small speed up in demuxing
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/mpegts.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index 7f56bacb2c..7746a524c4 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -157,6 +157,8 @@ struct MpegTSContext {
>      int resync_size;
>      int merge_pmt_versions;
>  
> +    AVBufferPool *pool;
> +
>      /******************************************/
>      /* private mpegts data */
>      /* scan context */
> @@ -1177,8 +1179,7 @@ static int mpegts_push_data(MpegTSFilter *filter,
>                          pes->total_size = MAX_PES_PAYLOAD;
>  
>                      /* allocate pes buffer */
> -                    pes->buffer = av_buffer_alloc(pes->total_size +
> -                                                  AV_INPUT_BUFFER_PADDING_SIZE);
> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>                      if (!pes->buffer)
>                          return AVERROR(ENOMEM);
>  
> @@ -1351,8 +1352,7 @@ skip:
>                      if (ret < 0)
>                          return ret;
>                      pes->total_size = MAX_PES_PAYLOAD;
> -                    pes->buffer = av_buffer_alloc(pes->total_size +
> -                                                  AV_INPUT_BUFFER_PADDING_SIZE);
> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>                      if (!pes->buffer)
>                          return AVERROR(ENOMEM);
>                      ts->stop_parse = 1;
> @@ -3032,6 +3032,10 @@ static int mpegts_read_header(AVFormatContext *s)
>      ts->stream     = s;
>      ts->auto_guess = 0;
>  
> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + AV_INPUT_BUFFER_PADDING_SIZE, NULL);
> +    if (!ts->pool)
> +        return AVERROR(ENOMEM);
> +
>      if (s->iformat == &ff_mpegts_demuxer) {
>          /* normal demux */
>  
> @@ -3200,6 +3204,8 @@ static void mpegts_free(MpegTSContext *ts)
>  
>      clear_programs(ts);
>  
> +    av_buffer_pool_uninit(&ts->pool);
> +
>      for (i = 0; i < NB_PID_MAX; i++)
>          if (ts->pids[i])
>              mpegts_close_filter(ts, ts->pids[i]);
> @@ -3295,6 +3301,12 @@ MpegTSContext *avpriv_mpegts_parse_open(AVFormatContext *s)
>      ts->stream = s;
>      ts->auto_guess = 1;
>  
> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + AV_INPUT_BUFFER_PADDING_SIZE, NULL);
> +    if (!ts->pool) {
> +        av_free(ts);
> +        return NULL;
> +    }
> +
>      mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
>      mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
>      mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);

Will apply soon if no one objects.
Marton Balint April 2, 2020, 6:15 p.m. UTC | #2
On Thu, 2 Apr 2020, James Almer wrote:

> On 3/29/2020 6:07 PM, James Almer wrote:
>> This produces a small speed up in demuxing
>> 
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavformat/mpegts.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>> 
>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> index 7f56bacb2c..7746a524c4 100644
>> --- a/libavformat/mpegts.c
>> +++ b/libavformat/mpegts.c
>> @@ -157,6 +157,8 @@ struct MpegTSContext {
>>      int resync_size;
>>      int merge_pmt_versions;
>> 
>> +    AVBufferPool *pool;
>> +
>>      /******************************************/
>>      /* private mpegts data */
>>      /* scan context */
>> @@ -1177,8 +1179,7 @@ static int mpegts_push_data(MpegTSFilter *filter,
>>                          pes->total_size = MAX_PES_PAYLOAD;
>>
>>                      /* allocate pes buffer */
>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>> -                                                  AV_INPUT_BUFFER_PADDING_SIZE);
>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>                      if (!pes->buffer)
>>                          return AVERROR(ENOMEM);
>> 
>> @@ -1351,8 +1352,7 @@ skip:
>>                      if (ret < 0)
>>                          return ret;
>>                      pes->total_size = MAX_PES_PAYLOAD;
>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>> -                                                  AV_INPUT_BUFFER_PADDING_SIZE);
>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>                      if (!pes->buffer)
>>                          return AVERROR(ENOMEM);
>>                      ts->stop_parse = 1;
>> @@ -3032,6 +3032,10 @@ static int mpegts_read_header(AVFormatContext *s)
>>      ts->stream     = s;
>>      ts->auto_guess = 0;
>> 
>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>> +    if (!ts->pool)
>> +        return AVERROR(ENOMEM);
>> +
>>      if (s->iformat == &ff_mpegts_demuxer) {
>>          /* normal demux */
>> 
>> @@ -3200,6 +3204,8 @@ static void mpegts_free(MpegTSContext *ts)
>>
>>      clear_programs(ts);
>> 
>> +    av_buffer_pool_uninit(&ts->pool);
>> +
>>      for (i = 0; i < NB_PID_MAX; i++)
>>          if (ts->pids[i])
>>              mpegts_close_filter(ts, ts->pids[i]);
>> @@ -3295,6 +3301,12 @@ MpegTSContext *avpriv_mpegts_parse_open(AVFormatContext *s)
>>      ts->stream = s;
>>      ts->auto_guess = 1;
>> 
>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>> +    if (!ts->pool) {
>> +        av_free(ts);
>> +        return NULL;
>> +    }
>> +
>>      mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
>>      mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
>>      mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);
>
> Will apply soon if no one objects.

LGTM.

Thanks,
Marton
Marton Balint April 2, 2020, 6:27 p.m. UTC | #3
On Thu, 2 Apr 2020, Marton Balint wrote:

>
>
> On Thu, 2 Apr 2020, James Almer wrote:
>
>> On 3/29/2020 6:07 PM, James Almer wrote:
>>> This produces a small speed up in demuxing
>>> 
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  libavformat/mpegts.c | 20 ++++++++++++++++----
>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>>> index 7f56bacb2c..7746a524c4 100644
>>> --- a/libavformat/mpegts.c
>>> +++ b/libavformat/mpegts.c
>>> @@ -157,6 +157,8 @@ struct MpegTSContext {
>>>      int resync_size;
>>>      int merge_pmt_versions;
>>> 
>>> +    AVBufferPool *pool;
>>> +
>>>      /******************************************/
>>>      /* private mpegts data */
>>>      /* scan context */
>>> @@ -1177,8 +1179,7 @@ static int mpegts_push_data(MpegTSFilter *filter,
>>>                          pes->total_size = MAX_PES_PAYLOAD;
>>>
>>>                      /* allocate pes buffer */
>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>> - 
> AV_INPUT_BUFFER_PADDING_SIZE);
>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>                      if (!pes->buffer)
>>>                          return AVERROR(ENOMEM);
>>> 
>>> @@ -1351,8 +1352,7 @@ skip:
>>>                      if (ret < 0)
>>>                          return ret;
>>>                      pes->total_size = MAX_PES_PAYLOAD;
>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>> - 
> AV_INPUT_BUFFER_PADDING_SIZE);
>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>                      if (!pes->buffer)
>>>                          return AVERROR(ENOMEM);
>>>                      ts->stop_parse = 1;
>>> @@ -3032,6 +3032,10 @@ static int mpegts_read_header(AVFormatContext *s)
>>>      ts->stream     = s;
>>>      ts->auto_guess = 0;
>>> 
>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>> +    if (!ts->pool)
>>> +        return AVERROR(ENOMEM);
>>> +
>>>      if (s->iformat == &ff_mpegts_demuxer) {
>>>          /* normal demux */
>>> 
>>> @@ -3200,6 +3204,8 @@ static void mpegts_free(MpegTSContext *ts)
>>>
>>>      clear_programs(ts);
>>> 
>>> +    av_buffer_pool_uninit(&ts->pool);
>>> +
>>>      for (i = 0; i < NB_PID_MAX; i++)
>>>          if (ts->pids[i])
>>>              mpegts_close_filter(ts, ts->pids[i]);
>>> @@ -3295,6 +3301,12 @@ MpegTSContext 
> *avpriv_mpegts_parse_open(AVFormatContext *s)
>>>      ts->stream = s;
>>>      ts->auto_guess = 1;
>>> 
>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>> +    if (!ts->pool) {
>>> +        av_free(ts);
>>> +        return NULL;
>>> +    }
>>> +
>>>      mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
>>>      mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
>>>      mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);
>>
>> Will apply soon if no one objects.
>
> LGTM.

On second thought aint this going to waste too much memory on small 
packets? If packets are queued somewhere it can make a huge difference if 
every buffer is 200kb...

Does the speedup goes away if only unbound packets make use of the buffer 
pool?

Thanks,
Marton
James Almer April 2, 2020, 6:37 p.m. UTC | #4
On 4/2/2020 3:27 PM, Marton Balint wrote:
> 
> 
> On Thu, 2 Apr 2020, Marton Balint wrote:
> 
>>
>>
>> On Thu, 2 Apr 2020, James Almer wrote:
>>
>>> On 3/29/2020 6:07 PM, James Almer wrote:
>>>> This produces a small speed up in demuxing
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>  libavformat/mpegts.c | 20 ++++++++++++++++----
>>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>>>> index 7f56bacb2c..7746a524c4 100644
>>>> --- a/libavformat/mpegts.c
>>>> +++ b/libavformat/mpegts.c
>>>> @@ -157,6 +157,8 @@ struct MpegTSContext {
>>>>      int resync_size;
>>>>      int merge_pmt_versions;
>>>>
>>>> +    AVBufferPool *pool;
>>>> +
>>>>      /******************************************/
>>>>      /* private mpegts data */
>>>>      /* scan context */
>>>> @@ -1177,8 +1179,7 @@ static int mpegts_push_data(MpegTSFilter *filter,
>>>>                          pes->total_size = MAX_PES_PAYLOAD;
>>>>
>>>>                      /* allocate pes buffer */
>>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>>> - 
>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>                      if (!pes->buffer)
>>>>                          return AVERROR(ENOMEM);
>>>>
>>>> @@ -1351,8 +1352,7 @@ skip:
>>>>                      if (ret < 0)
>>>>                          return ret;
>>>>                      pes->total_size = MAX_PES_PAYLOAD;
>>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>>> - 
>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>                      if (!pes->buffer)
>>>>                          return AVERROR(ENOMEM);
>>>>                      ts->stop_parse = 1;
>>>> @@ -3032,6 +3032,10 @@ static int mpegts_read_header(AVFormatContext
>>>> *s)
>>>>      ts->stream     = s;
>>>>      ts->auto_guess = 0;
>>>>
>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>> +    if (!ts->pool)
>>>> +        return AVERROR(ENOMEM);
>>>> +
>>>>      if (s->iformat == &ff_mpegts_demuxer) {
>>>>          /* normal demux */
>>>>
>>>> @@ -3200,6 +3204,8 @@ static void mpegts_free(MpegTSContext *ts)
>>>>
>>>>      clear_programs(ts);
>>>>
>>>> +    av_buffer_pool_uninit(&ts->pool);
>>>> +
>>>>      for (i = 0; i < NB_PID_MAX; i++)
>>>>          if (ts->pids[i])
>>>>              mpegts_close_filter(ts, ts->pids[i]);
>>>> @@ -3295,6 +3301,12 @@ MpegTSContext 
>> *avpriv_mpegts_parse_open(AVFormatContext *s)
>>>>      ts->stream = s;
>>>>      ts->auto_guess = 1;
>>>>
>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>> +    if (!ts->pool) {
>>>> +        av_free(ts);
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>>      mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
>>>>      mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
>>>>      mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);
>>>
>>> Will apply soon if no one objects.
>>
>> LGTM.
> 
> On second thought aint this going to waste too much memory on small
> packets? If packets are queued somewhere it can make a huge difference
> if every buffer is 200kb...
> 
> Does the speedup goes away if only unbound packets make use of the
> buffer pool?
> 
> Thanks,
> Marton

I tried these two LG 4k HDR sample videos, and the pool was never bigger
than 3 to 5 buffers for the entire demuxing process.

I'll bench again with your suggestion, but i don't think it will be a
problem even as is.
Marton Balint April 2, 2020, 6:49 p.m. UTC | #5
On Thu, 2 Apr 2020, James Almer wrote:

> On 4/2/2020 3:27 PM, Marton Balint wrote:
>> 
>> 
>> On Thu, 2 Apr 2020, Marton Balint wrote:
>> 
>>>
>>>
>>> On Thu, 2 Apr 2020, James Almer wrote:
>>>
>>>> On 3/29/2020 6:07 PM, James Almer wrote:
>>>>> This produces a small speed up in demuxing
>>>>>
>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>> ---
>>>>>  libavformat/mpegts.c | 20 ++++++++++++++++----
>>>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>>>>> index 7f56bacb2c..7746a524c4 100644
>>>>> --- a/libavformat/mpegts.c
>>>>> +++ b/libavformat/mpegts.c
>>>>> @@ -157,6 +157,8 @@ struct MpegTSContext {
>>>>>      int resync_size;
>>>>>      int merge_pmt_versions;
>>>>>
>>>>> +    AVBufferPool *pool;
>>>>> +
>>>>>      /******************************************/
>>>>>      /* private mpegts data */
>>>>>      /* scan context */
>>>>> @@ -1177,8 +1179,7 @@ static int mpegts_push_data(MpegTSFilter *filter,
>>>>>                          pes->total_size = MAX_PES_PAYLOAD;
>>>>>
>>>>>                      /* allocate pes buffer */
>>>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>>>> - 
>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>>                      if (!pes->buffer)
>>>>>                          return AVERROR(ENOMEM);
>>>>>
>>>>> @@ -1351,8 +1352,7 @@ skip:
>>>>>                      if (ret < 0)
>>>>>                          return ret;
>>>>>                      pes->total_size = MAX_PES_PAYLOAD;
>>>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>>>> - 
>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>>                      if (!pes->buffer)
>>>>>                          return AVERROR(ENOMEM);
>>>>>                      ts->stop_parse = 1;
>>>>> @@ -3032,6 +3032,10 @@ static int mpegts_read_header(AVFormatContext
>>>>> *s)
>>>>>      ts->stream     = s;
>>>>>      ts->auto_guess = 0;
>>>>>
>>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>>> +    if (!ts->pool)
>>>>> +        return AVERROR(ENOMEM);
>>>>> +
>>>>>      if (s->iformat == &ff_mpegts_demuxer) {
>>>>>          /* normal demux */
>>>>>
>>>>> @@ -3200,6 +3204,8 @@ static void mpegts_free(MpegTSContext *ts)
>>>>>
>>>>>      clear_programs(ts);
>>>>>
>>>>> +    av_buffer_pool_uninit(&ts->pool);
>>>>> +
>>>>>      for (i = 0; i < NB_PID_MAX; i++)
>>>>>          if (ts->pids[i])
>>>>>              mpegts_close_filter(ts, ts->pids[i]);
>>>>> @@ -3295,6 +3301,12 @@ MpegTSContext 
>>> *avpriv_mpegts_parse_open(AVFormatContext *s)
>>>>>      ts->stream = s;
>>>>>      ts->auto_guess = 1;
>>>>>
>>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>>> +    if (!ts->pool) {
>>>>> +        av_free(ts);
>>>>> +        return NULL;
>>>>> +    }
>>>>> +
>>>>>      mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
>>>>>      mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
>>>>>      mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);
>>>>
>>>> Will apply soon if no one objects.
>>>
>>> LGTM.
>> 
>> On second thought aint this going to waste too much memory on small
>> packets? If packets are queued somewhere it can make a huge difference
>> if every buffer is 200kb...
>> 
>> Does the speedup goes away if only unbound packets make use of the
>> buffer pool?
>> 
>> Thanks,
>> Marton
>
> I tried these two LG 4k HDR sample videos, and the pool was never bigger
> than 3 to 5 buffers for the entire demuxing process.

That is the case if there is no packet buffering, like in ffmpeg using a 
single input. But in ffplay or a lot of other apps packets are 
pre-buffered to remove the latency of the source media. An app could have 
assumed that if it buffers 1 MB of packets it will not consume a lot more 
than 1 MB of memory.

>
> I'll bench again with your suggestion, but i don't think it will be a
> problem even as is.

Maybe we should create log2(MAX_PACKET_SIZE) number of buffer pools 
instead? In that case the memory wasted is at most 100%... Still not very 
good, but a lot more acceptable.

Regards,
Marton
James Almer April 2, 2020, 7:10 p.m. UTC | #6
On 4/2/2020 3:49 PM, Marton Balint wrote:
> 
> 
> On Thu, 2 Apr 2020, James Almer wrote:
> 
>> On 4/2/2020 3:27 PM, Marton Balint wrote:
>>>
>>>
>>> On Thu, 2 Apr 2020, Marton Balint wrote:
>>>
>>>>
>>>>
>>>> On Thu, 2 Apr 2020, James Almer wrote:
>>>>
>>>>> On 3/29/2020 6:07 PM, James Almer wrote:
>>>>>> This produces a small speed up in demuxing
>>>>>>
>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>> ---
>>>>>>  libavformat/mpegts.c | 20 ++++++++++++++++----
>>>>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>>>>>> index 7f56bacb2c..7746a524c4 100644
>>>>>> --- a/libavformat/mpegts.c
>>>>>> +++ b/libavformat/mpegts.c
>>>>>> @@ -157,6 +157,8 @@ struct MpegTSContext {
>>>>>>      int resync_size;
>>>>>>      int merge_pmt_versions;
>>>>>>
>>>>>> +    AVBufferPool *pool;
>>>>>> +
>>>>>>      /******************************************/
>>>>>>      /* private mpegts data */
>>>>>>      /* scan context */
>>>>>> @@ -1177,8 +1179,7 @@ static int mpegts_push_data(MpegTSFilter
>>>>>> *filter,
>>>>>>                          pes->total_size = MAX_PES_PAYLOAD;
>>>>>>
>>>>>>                      /* allocate pes buffer */
>>>>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>>>>> - 
>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>>>                      if (!pes->buffer)
>>>>>>                          return AVERROR(ENOMEM);
>>>>>>
>>>>>> @@ -1351,8 +1352,7 @@ skip:
>>>>>>                      if (ret < 0)
>>>>>>                          return ret;
>>>>>>                      pes->total_size = MAX_PES_PAYLOAD;
>>>>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>>>>> - 
>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>>>                      if (!pes->buffer)
>>>>>>                          return AVERROR(ENOMEM);
>>>>>>                      ts->stop_parse = 1;
>>>>>> @@ -3032,6 +3032,10 @@ static int mpegts_read_header(AVFormatContext
>>>>>> *s)
>>>>>>      ts->stream     = s;
>>>>>>      ts->auto_guess = 0;
>>>>>>
>>>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>>>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>>>> +    if (!ts->pool)
>>>>>> +        return AVERROR(ENOMEM);
>>>>>> +
>>>>>>      if (s->iformat == &ff_mpegts_demuxer) {
>>>>>>          /* normal demux */
>>>>>>
>>>>>> @@ -3200,6 +3204,8 @@ static void mpegts_free(MpegTSContext *ts)
>>>>>>
>>>>>>      clear_programs(ts);
>>>>>>
>>>>>> +    av_buffer_pool_uninit(&ts->pool);
>>>>>> +
>>>>>>      for (i = 0; i < NB_PID_MAX; i++)
>>>>>>          if (ts->pids[i])
>>>>>>              mpegts_close_filter(ts, ts->pids[i]);
>>>>>> @@ -3295,6 +3301,12 @@ MpegTSContext 
>>>> *avpriv_mpegts_parse_open(AVFormatContext *s)
>>>>>>      ts->stream = s;
>>>>>>      ts->auto_guess = 1;
>>>>>>
>>>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>>>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>>>> +    if (!ts->pool) {
>>>>>> +        av_free(ts);
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +
>>>>>>      mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
>>>>>>      mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
>>>>>>      mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);
>>>>>
>>>>> Will apply soon if no one objects.
>>>>
>>>> LGTM.
>>>
>>> On second thought aint this going to waste too much memory on small
>>> packets? If packets are queued somewhere it can make a huge difference
>>> if every buffer is 200kb...
>>>
>>> Does the speedup goes away if only unbound packets make use of the
>>> buffer pool?
>>>
>>> Thanks,
>>> Marton
>>
>> I tried these two LG 4k HDR sample videos, and the pool was never bigger
>> than 3 to 5 buffers for the entire demuxing process.
> 
> That is the case if there is no packet buffering, like in ffmpeg using a
> single input. But in ffplay or a lot of other apps packets are
> pre-buffered to remove the latency of the source media. An app could
> have assumed that if it buffers 1 MB of packets it will not consume a
> lot more than 1 MB of memory.
> 
>>
>> I'll bench again with your suggestion, but i don't think it will be a
>> problem even as is.
> 
> Maybe we should create log2(MAX_PACKET_SIZE) number of buffer pools
> instead? In that case the memory wasted is at most 100%... Still not
> very good, but a lot more acceptable.

I don't think that's possible, or worth trying. There's currently no way
to query the amount of buffers in a given pool, and definitely no way to
know what buffers will be returned to a pool once all their references
are freed, other than keeping track of them within the muxer.

I'll try limiting the usage of the pool to unbound packets only, but i
suspect it will not make much of a difference compared to no pool usage
at all.

> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint April 2, 2020, 7:54 p.m. UTC | #7
On Thu, 2 Apr 2020, James Almer wrote:

> On 4/2/2020 3:49 PM, Marton Balint wrote:
>> 
>> 
>> On Thu, 2 Apr 2020, James Almer wrote:
>> 
>>> On 4/2/2020 3:27 PM, Marton Balint wrote:
>>>>
>>>>
>>>> On Thu, 2 Apr 2020, Marton Balint wrote:
>>>>
>>>>>
>>>>>
>>>>> On Thu, 2 Apr 2020, James Almer wrote:
>>>>>
>>>>>> On 3/29/2020 6:07 PM, James Almer wrote:
>>>>>>> This produces a small speed up in demuxing
>>>>>>>
>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>> ---
>>>>>>>  libavformat/mpegts.c | 20 ++++++++++++++++----
>>>>>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>>>>>>> index 7f56bacb2c..7746a524c4 100644
>>>>>>> --- a/libavformat/mpegts.c
>>>>>>> +++ b/libavformat/mpegts.c
>>>>>>> @@ -157,6 +157,8 @@ struct MpegTSContext {
>>>>>>>      int resync_size;
>>>>>>>      int merge_pmt_versions;
>>>>>>>
>>>>>>> +    AVBufferPool *pool;
>>>>>>> +
>>>>>>>      /******************************************/
>>>>>>>      /* private mpegts data */
>>>>>>>      /* scan context */
>>>>>>> @@ -1177,8 +1179,7 @@ static int mpegts_push_data(MpegTSFilter
>>>>>>> *filter,
>>>>>>>                          pes->total_size = MAX_PES_PAYLOAD;
>>>>>>>
>>>>>>>                      /* allocate pes buffer */
>>>>>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>>>>>> - 
>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>>>>                      if (!pes->buffer)
>>>>>>>                          return AVERROR(ENOMEM);
>>>>>>>
>>>>>>> @@ -1351,8 +1352,7 @@ skip:
>>>>>>>                      if (ret < 0)
>>>>>>>                          return ret;
>>>>>>>                      pes->total_size = MAX_PES_PAYLOAD;
>>>>>>> -                    pes->buffer = av_buffer_alloc(pes->total_size +
>>>>>>> - 
>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>>>>                      if (!pes->buffer)
>>>>>>>                          return AVERROR(ENOMEM);
>>>>>>>                      ts->stop_parse = 1;
>>>>>>> @@ -3032,6 +3032,10 @@ static int mpegts_read_header(AVFormatContext
>>>>>>> *s)
>>>>>>>      ts->stream     = s;
>>>>>>>      ts->auto_guess = 0;
>>>>>>>
>>>>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>>>>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>>>>> +    if (!ts->pool)
>>>>>>> +        return AVERROR(ENOMEM);
>>>>>>> +
>>>>>>>      if (s->iformat == &ff_mpegts_demuxer) {
>>>>>>>          /* normal demux */
>>>>>>>
>>>>>>> @@ -3200,6 +3204,8 @@ static void mpegts_free(MpegTSContext *ts)
>>>>>>>
>>>>>>>      clear_programs(ts);
>>>>>>>
>>>>>>> +    av_buffer_pool_uninit(&ts->pool);
>>>>>>> +
>>>>>>>      for (i = 0; i < NB_PID_MAX; i++)
>>>>>>>          if (ts->pids[i])
>>>>>>>              mpegts_close_filter(ts, ts->pids[i]);
>>>>>>> @@ -3295,6 +3301,12 @@ MpegTSContext 
>>>>> *avpriv_mpegts_parse_open(AVFormatContext *s)
>>>>>>>      ts->stream = s;
>>>>>>>      ts->auto_guess = 1;
>>>>>>>
>>>>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>>>>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>>>>> +    if (!ts->pool) {
>>>>>>> +        av_free(ts);
>>>>>>> +        return NULL;
>>>>>>> +    }
>>>>>>> +
>>>>>>>      mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
>>>>>>>      mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
>>>>>>>      mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);
>>>>>>
>>>>>> Will apply soon if no one objects.
>>>>>
>>>>> LGTM.
>>>>
>>>> On second thought aint this going to waste too much memory on small
>>>> packets? If packets are queued somewhere it can make a huge difference
>>>> if every buffer is 200kb...
>>>>
>>>> Does the speedup goes away if only unbound packets make use of the
>>>> buffer pool?
>>>>
>>>> Thanks,
>>>> Marton
>>>
>>> I tried these two LG 4k HDR sample videos, and the pool was never bigger
>>> than 3 to 5 buffers for the entire demuxing process.
>> 
>> That is the case if there is no packet buffering, like in ffmpeg using a
>> single input. But in ffplay or a lot of other apps packets are
>> pre-buffered to remove the latency of the source media. An app could
>> have assumed that if it buffers 1 MB of packets it will not consume a
>> lot more than 1 MB of memory.
>> 
>>>
>>> I'll bench again with your suggestion, but i don't think it will be a
>>> problem even as is.
>> 
>> Maybe we should create log2(MAX_PACKET_SIZE) number of buffer pools
>> instead? In that case the memory wasted is at most 100%... Still not
>> very good, but a lot more acceptable.
>
> I don't think that's possible, or worth trying. There's currently no way
> to query the amount of buffers in a given pool, and definitely no way to
> know what buffers will be returned to a pool once all their references
> are freed, other than keeping track of them within the muxer.

I meant something like this:

required_size = pes->total_size + AV_INPUT_BUFFER_PADDING;
index = av_log2(requred_size);
if (!ts->pools[index]) {
    pool_size = FFMIN(MAX_PES_PAYLOAD + AV_INPUT_BUFFER_PADDING, 2 << index);
    ts->pools[index] = av_buffer_pool_init(pool_size);
}
pes->buffer = av_buffer_pool_get(ts->pool[index]);


>
> I'll try limiting the usage of the pool to unbound packets only, but i
> suspect it will not make much of a difference compared to no pool usage
> at all.

Yeah, I expect this outcome as well.

Regards,
Marton
James Almer April 3, 2020, 5:46 p.m. UTC | #8
On 4/2/2020 4:54 PM, Marton Balint wrote:
> 
> 
> On Thu, 2 Apr 2020, James Almer wrote:
> 
>> On 4/2/2020 3:49 PM, Marton Balint wrote:
>>>
>>>
>>> On Thu, 2 Apr 2020, James Almer wrote:
>>>
>>>> On 4/2/2020 3:27 PM, Marton Balint wrote:
>>>>>
>>>>>
>>>>> On Thu, 2 Apr 2020, Marton Balint wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, 2 Apr 2020, James Almer wrote:
>>>>>>
>>>>>>> On 3/29/2020 6:07 PM, James Almer wrote:
>>>>>>>> This produces a small speed up in demuxing
>>>>>>>>
>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>> ---
>>>>>>>>  libavformat/mpegts.c | 20 ++++++++++++++++----
>>>>>>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>>>>>>>> index 7f56bacb2c..7746a524c4 100644
>>>>>>>> --- a/libavformat/mpegts.c
>>>>>>>> +++ b/libavformat/mpegts.c
>>>>>>>> @@ -157,6 +157,8 @@ struct MpegTSContext {
>>>>>>>>      int resync_size;
>>>>>>>>      int merge_pmt_versions;
>>>>>>>>
>>>>>>>> +    AVBufferPool *pool;
>>>>>>>> +
>>>>>>>>      /******************************************/
>>>>>>>>      /* private mpegts data */
>>>>>>>>      /* scan context */
>>>>>>>> @@ -1177,8 +1179,7 @@ static int mpegts_push_data(MpegTSFilter
>>>>>>>> *filter,
>>>>>>>>                          pes->total_size = MAX_PES_PAYLOAD;
>>>>>>>>
>>>>>>>>                      /* allocate pes buffer */
>>>>>>>> -                    pes->buffer =
>>>>>>>> av_buffer_alloc(pes->total_size +
>>>>>>>> - 
>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>>>>>                      if (!pes->buffer)
>>>>>>>>                          return AVERROR(ENOMEM);
>>>>>>>>
>>>>>>>> @@ -1351,8 +1352,7 @@ skip:
>>>>>>>>                      if (ret < 0)
>>>>>>>>                          return ret;
>>>>>>>>                      pes->total_size = MAX_PES_PAYLOAD;
>>>>>>>> -                    pes->buffer =
>>>>>>>> av_buffer_alloc(pes->total_size +
>>>>>>>> - 
>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>> +                    pes->buffer = av_buffer_pool_get(ts->pool);
>>>>>>>>                      if (!pes->buffer)
>>>>>>>>                          return AVERROR(ENOMEM);
>>>>>>>>                      ts->stop_parse = 1;
>>>>>>>> @@ -3032,6 +3032,10 @@ static int
>>>>>>>> mpegts_read_header(AVFormatContext
>>>>>>>> *s)
>>>>>>>>      ts->stream     = s;
>>>>>>>>      ts->auto_guess = 0;
>>>>>>>>
>>>>>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>>>>>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>>>>>> +    if (!ts->pool)
>>>>>>>> +        return AVERROR(ENOMEM);
>>>>>>>> +
>>>>>>>>      if (s->iformat == &ff_mpegts_demuxer) {
>>>>>>>>          /* normal demux */
>>>>>>>>
>>>>>>>> @@ -3200,6 +3204,8 @@ static void mpegts_free(MpegTSContext *ts)
>>>>>>>>
>>>>>>>>      clear_programs(ts);
>>>>>>>>
>>>>>>>> +    av_buffer_pool_uninit(&ts->pool);
>>>>>>>> +
>>>>>>>>      for (i = 0; i < NB_PID_MAX; i++)
>>>>>>>>          if (ts->pids[i])
>>>>>>>>              mpegts_close_filter(ts, ts->pids[i]);
>>>>>>>> @@ -3295,6 +3301,12 @@ MpegTSContext 
>>>>>> *avpriv_mpegts_parse_open(AVFormatContext *s)
>>>>>>>>      ts->stream = s;
>>>>>>>>      ts->auto_guess = 1;
>>>>>>>>
>>>>>>>> +    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + 
>>>>>> AV_INPUT_BUFFER_PADDING_SIZE, NULL);
>>>>>>>> +    if (!ts->pool) {
>>>>>>>> +        av_free(ts);
>>>>>>>> +        return NULL;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>>      mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
>>>>>>>>      mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
>>>>>>>>      mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);
>>>>>>>
>>>>>>> Will apply soon if no one objects.
>>>>>>
>>>>>> LGTM.
>>>>>
>>>>> On second thought aint this going to waste too much memory on small
>>>>> packets? If packets are queued somewhere it can make a huge difference
>>>>> if every buffer is 200kb...
>>>>>
>>>>> Does the speedup goes away if only unbound packets make use of the
>>>>> buffer pool?
>>>>>
>>>>> Thanks,
>>>>> Marton
>>>>
>>>> I tried these two LG 4k HDR sample videos, and the pool was never
>>>> bigger
>>>> than 3 to 5 buffers for the entire demuxing process.
>>>
>>> That is the case if there is no packet buffering, like in ffmpeg using a
>>> single input. But in ffplay or a lot of other apps packets are
>>> pre-buffered to remove the latency of the source media. An app could
>>> have assumed that if it buffers 1 MB of packets it will not consume a
>>> lot more than 1 MB of memory.
>>>
>>>>
>>>> I'll bench again with your suggestion, but i don't think it will be a
>>>> problem even as is.
>>>
>>> Maybe we should create log2(MAX_PACKET_SIZE) number of buffer pools
>>> instead? In that case the memory wasted is at most 100%... Still not
>>> very good, but a lot more acceptable.
>>
>> I don't think that's possible, or worth trying. There's currently no way
>> to query the amount of buffers in a given pool, and definitely no way to
>> know what buffers will be returned to a pool once all their references
>> are freed, other than keeping track of them within the muxer.
> 
> I meant something like this:
> 
> required_size = pes->total_size + AV_INPUT_BUFFER_PADDING;
> index = av_log2(requred_size);
> if (!ts->pools[index]) {
>    pool_size = FFMIN(MAX_PES_PAYLOAD + AV_INPUT_BUFFER_PADDING, 2 <<
> index);
>    ts->pools[index] = av_buffer_pool_init(pool_size);
> }
> pes->buffer = av_buffer_pool_get(ts->pool[index]);

Ah, you meant several pools. Seems a bit too convoluted for what
ultimately is a relatively small gain...

> 
> 
>>
>> I'll try limiting the usage of the pool to unbound packets only, but i
>> suspect it will not make much of a difference compared to no pool usage
>> at all.
> 
> Yeah, I expect this outcome as well.

So in the end it did make a difference, at least for these samples. Not
sure how often unbound PES payloads happen in mpegts, since i don't have
other samples at hand to test and bench, so i'm inclined to either do it
this way, or just drop the patch.

> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index 7f56bacb2c..7746a524c4 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -157,6 +157,8 @@  struct MpegTSContext {
     int resync_size;
     int merge_pmt_versions;
 
+    AVBufferPool *pool;
+
     /******************************************/
     /* private mpegts data */
     /* scan context */
@@ -1177,8 +1179,7 @@  static int mpegts_push_data(MpegTSFilter *filter,
                         pes->total_size = MAX_PES_PAYLOAD;
 
                     /* allocate pes buffer */
-                    pes->buffer = av_buffer_alloc(pes->total_size +
-                                                  AV_INPUT_BUFFER_PADDING_SIZE);
+                    pes->buffer = av_buffer_pool_get(ts->pool);
                     if (!pes->buffer)
                         return AVERROR(ENOMEM);
 
@@ -1351,8 +1352,7 @@  skip:
                     if (ret < 0)
                         return ret;
                     pes->total_size = MAX_PES_PAYLOAD;
-                    pes->buffer = av_buffer_alloc(pes->total_size +
-                                                  AV_INPUT_BUFFER_PADDING_SIZE);
+                    pes->buffer = av_buffer_pool_get(ts->pool);
                     if (!pes->buffer)
                         return AVERROR(ENOMEM);
                     ts->stop_parse = 1;
@@ -3032,6 +3032,10 @@  static int mpegts_read_header(AVFormatContext *s)
     ts->stream     = s;
     ts->auto_guess = 0;
 
+    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + AV_INPUT_BUFFER_PADDING_SIZE, NULL);
+    if (!ts->pool)
+        return AVERROR(ENOMEM);
+
     if (s->iformat == &ff_mpegts_demuxer) {
         /* normal demux */
 
@@ -3200,6 +3204,8 @@  static void mpegts_free(MpegTSContext *ts)
 
     clear_programs(ts);
 
+    av_buffer_pool_uninit(&ts->pool);
+
     for (i = 0; i < NB_PID_MAX; i++)
         if (ts->pids[i])
             mpegts_close_filter(ts, ts->pids[i]);
@@ -3295,6 +3301,12 @@  MpegTSContext *avpriv_mpegts_parse_open(AVFormatContext *s)
     ts->stream = s;
     ts->auto_guess = 1;
 
+    ts->pool = av_buffer_pool_init(MAX_PES_PAYLOAD + AV_INPUT_BUFFER_PADDING_SIZE, NULL);
+    if (!ts->pool) {
+        av_free(ts);
+        return NULL;
+    }
+
     mpegts_open_section_filter(ts, SDT_PID, sdt_cb, ts, 1);
     mpegts_open_section_filter(ts, PAT_PID, pat_cb, ts, 1);
     mpegts_open_section_filter(ts, EIT_PID, eit_cb, ts, 1);