| Message ID | 20210808225612.11600-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/h264_picture: split copying some H264Picture fields into a separate function | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
James Almer: > Will remove unnecessary allocations when both src and dst picture contain > references to the same buffers. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/h264_picture.c | 45 +++++++++++++++++++++++++++++++++++++++ > libavcodec/h264dec.h | 1 + > 2 files changed, 46 insertions(+) > > diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c > index 89aef37edd..1073d9e7e0 100644 > --- a/libavcodec/h264_picture.c > +++ b/libavcodec/h264_picture.c > @@ -142,6 +142,51 @@ fail: > return ret; > } > > +int ff_h264_replace_picture(H264Context *h, H264Picture *dst, H264Picture *src) Is there something that hinders you from making src const? If so, I don't see it. > +{ > + int ret, i; > + > + if (!src->f || !src->f->buf[0]) { > + ff_h264_unref_picture(h, dst); > + return 0; > + } > + > + av_assert0(src->tf.f == src->f); > + > + dst->tf.f = dst->f; > + ff_thread_release_buffer(h->avctx, &dst->tf); > + ret = ff_thread_ref_frame(&dst->tf, &src->tf); > + if (ret < 0) > + goto fail; > + > + ret = av_buffer_replace(&dst->qscale_table_buf, src->qscale_table_buf); > + ret |= av_buffer_replace(&dst->mb_type_buf, src->mb_type_buf); > + ret |= av_buffer_replace(&dst->pps_buf, src->pps_buf); > + if (ret < 0) > + goto fail; > + > + for (i = 0; i < 2; i++) { > + ret = av_buffer_replace(&dst->motion_val_buf[i], src->motion_val_buf[i]); > + ret |= av_buffer_replace(&dst->ref_index_buf[i], src->ref_index_buf[i]); > + if (ret < 0) > + goto fail; > + } > + > + if (src->hwaccel_picture_private) { dst in this function is allowed to be used/dirty; so I don't see anything that precludes dst->hwaccel_picture_private/hwaccel_priv_buf to be set while the same is not true for src. But then this check means that dst is not equivalent to src. > + ret = av_buffer_replace(&dst->hwaccel_priv_buf, src->hwaccel_priv_buf); > + if (ret < 0) > + goto fail; > + dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; This is the only thing that needs to be under if. - Andreas
On 8/8/2021 8:16 PM, Andreas Rheinhardt wrote: > James Almer: >> Will remove unnecessary allocations when both src and dst picture contain >> references to the same buffers. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/h264_picture.c | 45 +++++++++++++++++++++++++++++++++++++++ >> libavcodec/h264dec.h | 1 + >> 2 files changed, 46 insertions(+) >> >> diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c >> index 89aef37edd..1073d9e7e0 100644 >> --- a/libavcodec/h264_picture.c >> +++ b/libavcodec/h264_picture.c >> @@ -142,6 +142,51 @@ fail: >> return ret; >> } >> >> +int ff_h264_replace_picture(H264Context *h, H264Picture *dst, H264Picture *src) > > Is there something that hinders you from making src const? If so, I > don't see it. Amended locally (Also h264_copy_picture_params() in patch 1/3). > > >> +{ >> + int ret, i; >> + >> + if (!src->f || !src->f->buf[0]) { >> + ff_h264_unref_picture(h, dst); >> + return 0; >> + } >> + >> + av_assert0(src->tf.f == src->f); >> + >> + dst->tf.f = dst->f; >> + ff_thread_release_buffer(h->avctx, &dst->tf); >> + ret = ff_thread_ref_frame(&dst->tf, &src->tf); >> + if (ret < 0) >> + goto fail; >> + >> + ret = av_buffer_replace(&dst->qscale_table_buf, src->qscale_table_buf); >> + ret |= av_buffer_replace(&dst->mb_type_buf, src->mb_type_buf); >> + ret |= av_buffer_replace(&dst->pps_buf, src->pps_buf); >> + if (ret < 0) >> + goto fail; >> + >> + for (i = 0; i < 2; i++) { >> + ret = av_buffer_replace(&dst->motion_val_buf[i], src->motion_val_buf[i]); >> + ret |= av_buffer_replace(&dst->ref_index_buf[i], src->ref_index_buf[i]); >> + if (ret < 0) >> + goto fail; >> + } >> + >> + if (src->hwaccel_picture_private) { > > dst in this function is allowed to be used/dirty; so I don't see > anything that precludes dst->hwaccel_picture_private/hwaccel_priv_buf to > be set while the same is not true for src. But then this check means > that dst is not equivalent to src. > >> + ret = av_buffer_replace(&dst->hwaccel_priv_buf, src->hwaccel_priv_buf); >> + if (ret < 0) >> + goto fail; >> + dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; > > This is the only thing that needs to be under if. Amended locally into: > ret = av_buffer_replace(&dst->hwaccel_priv_buf, src->hwaccel_priv_buf); > if (ret < 0) > goto fail; > > dst->hwaccel_picture_private = NULL; > if (src->hwaccel_picture_private) > dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; > > - Andreas > _______________________________________________ > 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". >
James Almer: > On 8/8/2021 8:16 PM, Andreas Rheinhardt wrote: >> James Almer: >>> Will remove unnecessary allocations when both src and dst picture >>> contain >>> references to the same buffers. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/h264_picture.c | 45 +++++++++++++++++++++++++++++++++++++++ >>> libavcodec/h264dec.h | 1 + >>> 2 files changed, 46 insertions(+) >>> >>> diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c >>> index 89aef37edd..1073d9e7e0 100644 >>> --- a/libavcodec/h264_picture.c >>> +++ b/libavcodec/h264_picture.c >>> @@ -142,6 +142,51 @@ fail: >>> return ret; >>> } >>> +int ff_h264_replace_picture(H264Context *h, H264Picture *dst, >>> H264Picture *src) >> >> Is there something that hinders you from making src const? If so, I >> don't see it. > > Amended locally (Also h264_copy_picture_params() in patch 1/3). > >> >> >>> +{ >>> + int ret, i; >>> + >>> + if (!src->f || !src->f->buf[0]) { >>> + ff_h264_unref_picture(h, dst); >>> + return 0; >>> + } >>> + >>> + av_assert0(src->tf.f == src->f); >>> + >>> + dst->tf.f = dst->f; >>> + ff_thread_release_buffer(h->avctx, &dst->tf); >>> + ret = ff_thread_ref_frame(&dst->tf, &src->tf); >>> + if (ret < 0) >>> + goto fail; >>> + >>> + ret = av_buffer_replace(&dst->qscale_table_buf, >>> src->qscale_table_buf); >>> + ret |= av_buffer_replace(&dst->mb_type_buf, src->mb_type_buf); >>> + ret |= av_buffer_replace(&dst->pps_buf, src->pps_buf); >>> + if (ret < 0) >>> + goto fail; >>> + >>> + for (i = 0; i < 2; i++) { >>> + ret = av_buffer_replace(&dst->motion_val_buf[i], >>> src->motion_val_buf[i]); >>> + ret |= av_buffer_replace(&dst->ref_index_buf[i], >>> src->ref_index_buf[i]); >>> + if (ret < 0) >>> + goto fail; >>> + } >>> + >>> + if (src->hwaccel_picture_private) { >> >> dst in this function is allowed to be used/dirty; so I don't see >> anything that precludes dst->hwaccel_picture_private/hwaccel_priv_buf to >> be set while the same is not true for src. But then this check means >> that dst is not equivalent to src. >> >>> + ret = av_buffer_replace(&dst->hwaccel_priv_buf, >>> src->hwaccel_priv_buf); >>> + if (ret < 0) >>> + goto fail; >>> + dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; >> >> This is the only thing that needs to be under if. > > Amended locally into: > >> ret = av_buffer_replace(&dst->hwaccel_priv_buf, >> src->hwaccel_priv_buf); >> if (ret < 0) >> goto fail; >> >> dst->hwaccel_picture_private = NULL; >> if (src->hwaccel_picture_private) >> dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; On second look wouldn't dst->hwaccel_picture_private = src->hwaccel_picture_private be the same thing, but without a branch? (And even if it could happen (I doubt it) that src->hwaccel_picture_private and src->hwaccel_priv_buf->data differ then shouldn't we set the hwaccel_picture_private from src's hwaccel_picture_private to make src and dst equivalent?) - Andreas
On 8/8/2021 9:34 PM, Andreas Rheinhardt wrote: > James Almer: >> On 8/8/2021 8:16 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> Will remove unnecessary allocations when both src and dst picture >>>> contain >>>> references to the same buffers. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/h264_picture.c | 45 +++++++++++++++++++++++++++++++++++++++ >>>> libavcodec/h264dec.h | 1 + >>>> 2 files changed, 46 insertions(+) >>>> >>>> diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c >>>> index 89aef37edd..1073d9e7e0 100644 >>>> --- a/libavcodec/h264_picture.c >>>> +++ b/libavcodec/h264_picture.c >>>> @@ -142,6 +142,51 @@ fail: >>>> return ret; >>>> } >>>> +int ff_h264_replace_picture(H264Context *h, H264Picture *dst, >>>> H264Picture *src) >>> >>> Is there something that hinders you from making src const? If so, I >>> don't see it. >> >> Amended locally (Also h264_copy_picture_params() in patch 1/3). >> >>> >>> >>>> +{ >>>> + int ret, i; >>>> + >>>> + if (!src->f || !src->f->buf[0]) { >>>> + ff_h264_unref_picture(h, dst); >>>> + return 0; >>>> + } >>>> + >>>> + av_assert0(src->tf.f == src->f); >>>> + >>>> + dst->tf.f = dst->f; >>>> + ff_thread_release_buffer(h->avctx, &dst->tf); >>>> + ret = ff_thread_ref_frame(&dst->tf, &src->tf); >>>> + if (ret < 0) >>>> + goto fail; >>>> + >>>> + ret = av_buffer_replace(&dst->qscale_table_buf, >>>> src->qscale_table_buf); >>>> + ret |= av_buffer_replace(&dst->mb_type_buf, src->mb_type_buf); >>>> + ret |= av_buffer_replace(&dst->pps_buf, src->pps_buf); >>>> + if (ret < 0) >>>> + goto fail; >>>> + >>>> + for (i = 0; i < 2; i++) { >>>> + ret = av_buffer_replace(&dst->motion_val_buf[i], >>>> src->motion_val_buf[i]); >>>> + ret |= av_buffer_replace(&dst->ref_index_buf[i], >>>> src->ref_index_buf[i]); >>>> + if (ret < 0) >>>> + goto fail; >>>> + } >>>> + >>>> + if (src->hwaccel_picture_private) { >>> >>> dst in this function is allowed to be used/dirty; so I don't see >>> anything that precludes dst->hwaccel_picture_private/hwaccel_priv_buf to >>> be set while the same is not true for src. But then this check means >>> that dst is not equivalent to src. >>> >>>> + ret = av_buffer_replace(&dst->hwaccel_priv_buf, >>>> src->hwaccel_priv_buf); >>>> + if (ret < 0) >>>> + goto fail; >>>> + dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; >>> >>> This is the only thing that needs to be under if. >> >> Amended locally into: >> >>> ret = av_buffer_replace(&dst->hwaccel_priv_buf, >>> src->hwaccel_priv_buf); >>> if (ret < 0) >>> goto fail; >>> >>> dst->hwaccel_picture_private = NULL; >>> if (src->hwaccel_picture_private) >>> dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; > > On second look wouldn't dst->hwaccel_picture_private = > src->hwaccel_picture_private be the same thing, but without a branch? Yeah, I'll remove the branch and just copy the field. > (And even if it could happen (I doubt it) that > src->hwaccel_picture_private and src->hwaccel_priv_buf->data differ then > shouldn't we set the hwaccel_picture_private from src's > hwaccel_picture_private to make src and dst equivalent?) If they could differ, then ff_h264_ref_picture() would be broken. So yeah, like i said above I'll just copy the field. > > - Andreas > _______________________________________________ > 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/libavcodec/h264_picture.c b/libavcodec/h264_picture.c index 89aef37edd..1073d9e7e0 100644 --- a/libavcodec/h264_picture.c +++ b/libavcodec/h264_picture.c @@ -142,6 +142,51 @@ fail: return ret; } +int ff_h264_replace_picture(H264Context *h, H264Picture *dst, H264Picture *src) +{ + int ret, i; + + if (!src->f || !src->f->buf[0]) { + ff_h264_unref_picture(h, dst); + return 0; + } + + av_assert0(src->tf.f == src->f); + + dst->tf.f = dst->f; + ff_thread_release_buffer(h->avctx, &dst->tf); + ret = ff_thread_ref_frame(&dst->tf, &src->tf); + if (ret < 0) + goto fail; + + ret = av_buffer_replace(&dst->qscale_table_buf, src->qscale_table_buf); + ret |= av_buffer_replace(&dst->mb_type_buf, src->mb_type_buf); + ret |= av_buffer_replace(&dst->pps_buf, src->pps_buf); + if (ret < 0) + goto fail; + + for (i = 0; i < 2; i++) { + ret = av_buffer_replace(&dst->motion_val_buf[i], src->motion_val_buf[i]); + ret |= av_buffer_replace(&dst->ref_index_buf[i], src->ref_index_buf[i]); + if (ret < 0) + goto fail; + } + + if (src->hwaccel_picture_private) { + ret = av_buffer_replace(&dst->hwaccel_priv_buf, src->hwaccel_priv_buf); + if (ret < 0) + goto fail; + dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; + } + + h264_copy_picture_params(dst, src); + + return 0; +fail: + ff_h264_unref_picture(h, dst); + return ret; +} + void ff_h264_set_erpic(ERPicture *dst, H264Picture *src) { #if CONFIG_ERROR_RESILIENCE diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h index 8954b74795..655b8f0b9a 100644 --- a/libavcodec/h264dec.h +++ b/libavcodec/h264dec.h @@ -833,6 +833,7 @@ static inline int find_start_code(const uint8_t *buf, int buf_size, int ff_h264_field_end(H264Context *h, H264SliceContext *sl, int in_setup); int ff_h264_ref_picture(H264Context *h, H264Picture *dst, H264Picture *src); +int ff_h264_replace_picture(H264Context *h, H264Picture *dst, H264Picture *src); void ff_h264_unref_picture(H264Context *h, H264Picture *pic); int ff_h264_slice_context_init(H264Context *h, H264SliceContext *sl);
Will remove unnecessary allocations when both src and dst picture contain references to the same buffers. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/h264_picture.c | 45 +++++++++++++++++++++++++++++++++++++++ libavcodec/h264dec.h | 1 + 2 files changed, 46 insertions(+)