[FFmpeg-devel] [PATCH v2] avcodec/h264_parser: Try to avoid (un)referencing

[Anton Khirnov]

Quoting Andreas Rheinhardt (2020-06-01 12:21:15)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2020-05-29 18:31:57)
> >> When a slice is encountered, the H.264 parser up until now always
> >> unreferenced and reset the currently active PPS; immediately
> >> afterwards, the currently active PPS is set again which includes
> >> referencing it. Given that it is typical for the active parameter
> >> sets to change only seldomly, most of the time the new active PPS will
> >> be the old one. Therefore this commit checks for this and only
> >> unreferences the PPS if it changed.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> >> ---
> >> New and much simplified version of [1]. This has been made possible by
> >> 5e316096fa9ba4493d9dbb48847ad8e0b0e188c3.
> >>
> >> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-August/248374.html
> >>
> >>  libavcodec/h264_parser.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> > 
> > I've been considering a utility function along the lines of:
> > 
> > int av_buffer_update(AVBufferRef **dst, AVBufferRef *src)
> > {
> >     if ((*dst)->buffer == src->buffer)
> 
> Obviously missing a check for *dst == NULL.
> 
> >         return 0;
> 
> What if *dst and src point to different parts of the same underlying
> AVBuffer? You'd need to overwrite **dst with *src in this case.
> 
> >     av_buffer_unref(dst);
> >     *dst = av_buffer_ref(src);
> >     return 1;
> 
> Missing check for av_buffer_ref failure.

This was supposed to be a quick example, not complete code ready to be
pushed.

> 
> > }
> > 
> > which would help avoid unnecessary unrefs+refs in such cases. Thoughts?
> > 
> I also pondered such a helper function, but then opted for the simple
> approach instead. But if you want such a function and if it resides in
> libavutil/buffer.c, then one could improve this even further by not
> calling av_buffer_unref(dst) which throws an AVBufferRef away, but by
> just decrementing the refcount of the underlying AVBuffer of *dst (and
> freeing it if necessary), incrementing the refcount of the AVBuffer of
> src and overwriting **dst with *src.

How is that an improvement? It seems like a lot of complexity to save a
small malloc+free.

-- 
Anton Khirnov