| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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?
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
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.
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 --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
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(+)