| Message ID | 20180120214929.2480-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Jan 20, 2018 at 06:49:29PM -0300, James Almer wrote: > Signed-off-by: James Almer <jamrial@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 is this intended? has someone checked this to be safe ? [...] > --- a/libavcodec/h264dec.c > +++ b/libavcodec/h264dec.c > @@ -326,6 +326,9 @@ static int h264_init_context(AVCodecContext *avctx, H264Context *h) > > ff_h264_sei_uninit(&h->sei); > > + if (ff_h264_ps_init(&h->ps) < 0) > + return AVERROR(ENOMEM); This probably should forward the returned error code also a memcmp on the data might be worth looking into to avoid re-parsing of unchanged PS (theres already a memcmp in libavcodec/h264_ps.c) [...]
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@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. > is this intended? Not really. My intention is to still get a zeroed buffer as it always did. > has someone checked this to be safe ? > > > [...] >> --- a/libavcodec/h264dec.c >> +++ b/libavcodec/h264dec.c >> @@ -326,6 +326,9 @@ static int h264_init_context(AVCodecContext *avctx, H264Context *h) >> >> ff_h264_sei_uninit(&h->sei); >> >> + if (ff_h264_ps_init(&h->ps) < 0) >> + return AVERROR(ENOMEM); > > This probably should forward the returned error code Changed locally. > > also a memcmp on the data might be worth looking into to avoid re-parsing of > unchanged PS > (theres already a memcmp in libavcodec/h264_ps.c) > > [...] > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Sun, 21 Jan 2018 03:33:08 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > also a memcmp on the data might be worth looking into to avoid re-parsing of > unchanged PS > (theres already a memcmp in libavcodec/h264_ps.c) That sounds like it'd help much more than a buffer pool. Anyway, did anyone run benchmarks on this?
On 1/21/2018 9:12 AM, wm4 wrote: > On Sun, 21 Jan 2018 03:33:08 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > >> also a memcmp on the data might be worth looking into to avoid re-parsing of >> unchanged PS >> (theres already a memcmp in libavcodec/h264_ps.c) > > That sounds like it'd help much more than a buffer pool. > > Anyway, did anyone run benchmarks on this? Do you have a raw h264/hevc stream with preferably more than one pps before every keyframe? Otherwise i don't think there will be much of a difference in performance.
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@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 > > > is this intended? > > Not really. My intention is to still get a zeroed buffer as it always did. > [...]
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@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.
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); if (!pps_buf) return AVERROR(ENOMEM); pps = (PPS*)pps_buf->data; diff --git a/libavcodec/h264_ps.h b/libavcodec/h264_ps.h index 51b6694b5f..65730153e7 100644 --- a/libavcodec/h264_ps.h +++ b/libavcodec/h264_ps.h @@ -135,6 +135,9 @@ typedef struct PPS { } PPS; typedef struct H264ParamSets { + AVBufferPool *sps_pool; + AVBufferPool *pps_pool; + AVBufferRef *sps_list[MAX_SPS_COUNT]; AVBufferRef *pps_list[MAX_PPS_COUNT]; @@ -162,4 +165,6 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct */ void ff_h264_ps_uninit(H264ParamSets *ps); +int ff_h264_ps_init(H264ParamSets *ps); + #endif /* AVCODEC_H264_PS_H */ diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c index 8c9c6d9f3b..ef79387326 100644 --- a/libavcodec/h264dec.c +++ b/libavcodec/h264dec.c @@ -326,6 +326,9 @@ static int h264_init_context(AVCodecContext *avctx, H264Context *h) ff_h264_sei_uninit(&h->sei); + if (ff_h264_ps_init(&h->ps) < 0) + return AVERROR(ENOMEM); + avctx->chroma_sample_location = AVCHROMA_LOC_LEFT; h->nb_slice_ctx = (avctx->active_thread_type & FF_THREAD_SLICE) ? avctx->thread_count : 1; diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c index 6c5d3ddd79..f623147741 100644 --- a/libavcodec/mediacodecdec.c +++ b/libavcodec/mediacodecdec.c @@ -131,6 +131,10 @@ static int h264_set_extradata(AVCodecContext *avctx, FFAMediaFormat *format) memset(&ps, 0, sizeof(ps)); + ret = ff_h264_ps_init(&ps); + if (ret < 0) + goto done; + ret = ff_h264_decode_extradata(avctx->extradata, avctx->extradata_size, &ps, &is_avc, &nal_length_size, 0, avctx); if (ret < 0) {
Signed-off-by: James Almer <jamrial@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(-)