diff mbox series

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

Message ID 20200529163157.22588-1-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avcodec/h264_parser: Try to avoid (un)referencing
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt May 29, 2020, 4:31 p.m. UTC
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@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(+)

Comments

Anton Khirnov June 1, 2020, 9:49 a.m. UTC | #1
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@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)
        return 0;
    av_buffer_unref(dst);
    *dst = av_buffer_ref(src);
    return 1;
}

which would help avoid unnecessary unrefs+refs in such cases. Thoughts?
Andreas Rheinhardt June 1, 2020, 10:21 a.m. UTC | #2
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@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.

> }
> 
> 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.

- Andreas
Anton Khirnov June 1, 2020, 1:07 p.m. UTC | #3
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@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.
James Almer June 1, 2020, 6:57 p.m. UTC | #4
On 6/1/2020 6:49 AM, Anton Khirnov wrote:
> 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@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)
>         return 0;
>     av_buffer_unref(dst);
>     *dst = av_buffer_ref(src);
>     return 1;
> }
> 
> which would help avoid unnecessary unrefs+refs in such cases. Thoughts?

I'd call it av_buffer_replace() instead, and i think you could reuse the
buffer_replace() function already in buffer.c for this.
diff mbox series

Patch

diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index d988484660..c6fd049f46 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -363,6 +363,7 @@  static inline int parse_nal_units(AVCodecParserContext *s,
                 goto fail;
             }
 
+            if (p->ps.pps != (const PPS*)p->ps.pps_list[pps_id]->data) {
             av_buffer_unref(&p->ps.pps_ref);
             p->ps.pps = NULL;
             p->ps.sps = NULL;
@@ -371,6 +372,7 @@  static inline int parse_nal_units(AVCodecParserContext *s,
                 goto fail;
             p->ps.pps = (const PPS*)p->ps.pps_ref->data;
             p->ps.sps = p->ps.pps->sps;
+            }
             sps       = p->ps.sps;
 
             // heuristic to detect non marked keyframes