| Message ID | 20200329210756.4865-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mpegts: use a buffer pool for PES payloads | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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.
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
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
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.
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
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".
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
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 --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);
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(-)