| Message ID | 20240206225735.12251-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 7f92014acaadc739660c2cf35bde8e1c7e7aee36 |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/nvdec: don't free NVDECContext->bitstream | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 06.02.2024 23:57, James Almer wrote: > Ensure all hwaccels that allocate a buffer use NVDECContext->bitstream_internal. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/nvdec.c | 2 +- > libavcodec/nvdec_h264.c | 4 ++-- > libavcodec/nvdec_hevc.c | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c > index 27be644356..d13b790632 100644 > --- a/libavcodec/nvdec.c > +++ b/libavcodec/nvdec.c > @@ -259,8 +259,8 @@ int ff_nvdec_decode_uninit(AVCodecContext *avctx) > { > NVDECContext *ctx = avctx->internal->hwaccel_priv_data; > > - av_freep(&ctx->bitstream); > av_freep(&ctx->bitstream_internal); > + ctx->bitstream = NULL; > ctx->bitstream_len = 0; > ctx->bitstream_allocated = 0; > > diff --git a/libavcodec/nvdec_h264.c b/libavcodec/nvdec_h264.c > index f022619b64..8c72d5f4f7 100644 > --- a/libavcodec/nvdec_h264.c > +++ b/libavcodec/nvdec_h264.c > @@ -138,11 +138,11 @@ static int nvdec_h264_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, > const H264SliceContext *sl = &h->slice_ctx[0]; > void *tmp; > > - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, > + tmp = av_fast_realloc(ctx->bitstream_internal, &ctx->bitstream_allocated, > ctx->bitstream_len + size + 3); > if (!tmp) > return AVERROR(ENOMEM); > - ctx->bitstream = tmp; > + ctx->bitstream = ctx->bitstream_internal = tmp; > > tmp = av_fast_realloc(ctx->slice_offsets, &ctx->slice_offsets_allocated, > (ctx->nb_slices + 1) * sizeof(*ctx->slice_offsets)); > diff --git a/libavcodec/nvdec_hevc.c b/libavcodec/nvdec_hevc.c > index b83d5edcf9..25319a1328 100644 > --- a/libavcodec/nvdec_hevc.c > +++ b/libavcodec/nvdec_hevc.c > @@ -274,11 +274,11 @@ static int nvdec_hevc_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, > NVDECContext *ctx = avctx->internal->hwaccel_priv_data; > void *tmp; > > - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, > + tmp = av_fast_realloc(ctx->bitstream_internal, &ctx->bitstream_allocated, > ctx->bitstream_len + size + 3); > if (!tmp) > return AVERROR(ENOMEM); > - ctx->bitstream = tmp; > + ctx->bitstream = ctx->bitstream_internal = tmp; > > tmp = av_fast_realloc(ctx->slice_offsets, &ctx->slice_offsets_allocated, > (ctx->nb_slices + 1) * sizeof(*ctx->slice_offsets)); bitstream_internal was added for the av1 slice reconstructions. Probably for the exact reason to avoid this confusion. Looking at this some more... With this patch in place, there should never be a situation in which we want to free/freep ctx->bitstream? So the freep() on it should probably be removed alongside, and replace with just setting it to NULL.
On 07.02.2024 00:00, Timo Rothenpieler wrote: > On 06.02.2024 23:57, James Almer wrote: >> Ensure all hwaccels that allocate a buffer use >> NVDECContext->bitstream_internal. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/nvdec.c | 2 +- >> libavcodec/nvdec_h264.c | 4 ++-- >> libavcodec/nvdec_hevc.c | 4 ++-- >> 3 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c >> index 27be644356..d13b790632 100644 >> --- a/libavcodec/nvdec.c >> +++ b/libavcodec/nvdec.c >> @@ -259,8 +259,8 @@ int ff_nvdec_decode_uninit(AVCodecContext *avctx) >> { >> NVDECContext *ctx = avctx->internal->hwaccel_priv_data; >> - av_freep(&ctx->bitstream); >> av_freep(&ctx->bitstream_internal); >> + ctx->bitstream = NULL; >> ctx->bitstream_len = 0; >> ctx->bitstream_allocated = 0; >> diff --git a/libavcodec/nvdec_h264.c b/libavcodec/nvdec_h264.c >> index f022619b64..8c72d5f4f7 100644 >> --- a/libavcodec/nvdec_h264.c >> +++ b/libavcodec/nvdec_h264.c >> @@ -138,11 +138,11 @@ static int >> nvdec_h264_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, >> const H264SliceContext *sl = &h->slice_ctx[0]; >> void *tmp; >> - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, >> + tmp = av_fast_realloc(ctx->bitstream_internal, >> &ctx->bitstream_allocated, >> ctx->bitstream_len + size + 3); >> if (!tmp) >> return AVERROR(ENOMEM); >> - ctx->bitstream = tmp; >> + ctx->bitstream = ctx->bitstream_internal = tmp; >> tmp = av_fast_realloc(ctx->slice_offsets, >> &ctx->slice_offsets_allocated, >> (ctx->nb_slices + 1) * >> sizeof(*ctx->slice_offsets)); >> diff --git a/libavcodec/nvdec_hevc.c b/libavcodec/nvdec_hevc.c >> index b83d5edcf9..25319a1328 100644 >> --- a/libavcodec/nvdec_hevc.c >> +++ b/libavcodec/nvdec_hevc.c >> @@ -274,11 +274,11 @@ static int >> nvdec_hevc_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, >> NVDECContext *ctx = avctx->internal->hwaccel_priv_data; >> void *tmp; >> - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, >> + tmp = av_fast_realloc(ctx->bitstream_internal, >> &ctx->bitstream_allocated, >> ctx->bitstream_len + size + 3); >> if (!tmp) >> return AVERROR(ENOMEM); >> - ctx->bitstream = tmp; >> + ctx->bitstream = ctx->bitstream_internal = tmp; >> tmp = av_fast_realloc(ctx->slice_offsets, >> &ctx->slice_offsets_allocated, >> (ctx->nb_slices + 1) * >> sizeof(*ctx->slice_offsets)); > > bitstream_internal was added for the av1 slice reconstructions. > Probably for the exact reason to avoid this confusion. > > Looking at this some more... With this patch in place, there should > never be a situation in which we want to free/freep ctx->bitstream? > So the freep() on it should probably be removed alongside, and replace > with just setting it to NULL. Likewise, the entire need for the _simple_end_frame function is gone. NULL'ing the bitstream can just be done in the normal one, and all decoders can use it.
On 2/6/2024 8:00 PM, Timo Rothenpieler wrote: > On 06.02.2024 23:57, James Almer wrote: >> Ensure all hwaccels that allocate a buffer use >> NVDECContext->bitstream_internal. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/nvdec.c | 2 +- >> libavcodec/nvdec_h264.c | 4 ++-- >> libavcodec/nvdec_hevc.c | 4 ++-- >> 3 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c >> index 27be644356..d13b790632 100644 >> --- a/libavcodec/nvdec.c >> +++ b/libavcodec/nvdec.c >> @@ -259,8 +259,8 @@ int ff_nvdec_decode_uninit(AVCodecContext *avctx) >> { >> NVDECContext *ctx = avctx->internal->hwaccel_priv_data; >> - av_freep(&ctx->bitstream); >> av_freep(&ctx->bitstream_internal); >> + ctx->bitstream = NULL; >> ctx->bitstream_len = 0; >> ctx->bitstream_allocated = 0; >> diff --git a/libavcodec/nvdec_h264.c b/libavcodec/nvdec_h264.c >> index f022619b64..8c72d5f4f7 100644 >> --- a/libavcodec/nvdec_h264.c >> +++ b/libavcodec/nvdec_h264.c >> @@ -138,11 +138,11 @@ static int >> nvdec_h264_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, >> const H264SliceContext *sl = &h->slice_ctx[0]; >> void *tmp; >> - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, >> + tmp = av_fast_realloc(ctx->bitstream_internal, >> &ctx->bitstream_allocated, >> ctx->bitstream_len + size + 3); >> if (!tmp) >> return AVERROR(ENOMEM); >> - ctx->bitstream = tmp; >> + ctx->bitstream = ctx->bitstream_internal = tmp; >> tmp = av_fast_realloc(ctx->slice_offsets, >> &ctx->slice_offsets_allocated, >> (ctx->nb_slices + 1) * >> sizeof(*ctx->slice_offsets)); >> diff --git a/libavcodec/nvdec_hevc.c b/libavcodec/nvdec_hevc.c >> index b83d5edcf9..25319a1328 100644 >> --- a/libavcodec/nvdec_hevc.c >> +++ b/libavcodec/nvdec_hevc.c >> @@ -274,11 +274,11 @@ static int >> nvdec_hevc_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, >> NVDECContext *ctx = avctx->internal->hwaccel_priv_data; >> void *tmp; >> - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, >> + tmp = av_fast_realloc(ctx->bitstream_internal, >> &ctx->bitstream_allocated, >> ctx->bitstream_len + size + 3); >> if (!tmp) >> return AVERROR(ENOMEM); >> - ctx->bitstream = tmp; >> + ctx->bitstream = ctx->bitstream_internal = tmp; >> tmp = av_fast_realloc(ctx->slice_offsets, >> &ctx->slice_offsets_allocated, >> (ctx->nb_slices + 1) * >> sizeof(*ctx->slice_offsets)); > > bitstream_internal was added for the av1 slice reconstructions. > Probably for the exact reason to avoid this confusion. > > Looking at this some more... With this patch in place, there should > never be a situation in which we want to free/freep ctx->bitstream? > So the freep() on it should probably be removed alongside, and replace > with just setting it to NULL. That's what i did above, in the libavcodec/nvdec.c chunk.
On 07.02.2024 00:05, James Almer wrote: > On 2/6/2024 8:00 PM, Timo Rothenpieler wrote: >> On 06.02.2024 23:57, James Almer wrote: >>> Ensure all hwaccels that allocate a buffer use >>> NVDECContext->bitstream_internal. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/nvdec.c | 2 +- >>> libavcodec/nvdec_h264.c | 4 ++-- >>> libavcodec/nvdec_hevc.c | 4 ++-- >>> 3 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c >>> index 27be644356..d13b790632 100644 >>> --- a/libavcodec/nvdec.c >>> +++ b/libavcodec/nvdec.c >>> @@ -259,8 +259,8 @@ int ff_nvdec_decode_uninit(AVCodecContext *avctx) >>> { >>> NVDECContext *ctx = avctx->internal->hwaccel_priv_data; >>> - av_freep(&ctx->bitstream); >>> av_freep(&ctx->bitstream_internal); >>> + ctx->bitstream = NULL; >>> ctx->bitstream_len = 0; >>> ctx->bitstream_allocated = 0; >>> diff --git a/libavcodec/nvdec_h264.c b/libavcodec/nvdec_h264.c >>> index f022619b64..8c72d5f4f7 100644 >>> --- a/libavcodec/nvdec_h264.c >>> +++ b/libavcodec/nvdec_h264.c >>> @@ -138,11 +138,11 @@ static int >>> nvdec_h264_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, >>> const H264SliceContext *sl = &h->slice_ctx[0]; >>> void *tmp; >>> - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, >>> + tmp = av_fast_realloc(ctx->bitstream_internal, >>> &ctx->bitstream_allocated, >>> ctx->bitstream_len + size + 3); >>> if (!tmp) >>> return AVERROR(ENOMEM); >>> - ctx->bitstream = tmp; >>> + ctx->bitstream = ctx->bitstream_internal = tmp; >>> tmp = av_fast_realloc(ctx->slice_offsets, >>> &ctx->slice_offsets_allocated, >>> (ctx->nb_slices + 1) * >>> sizeof(*ctx->slice_offsets)); >>> diff --git a/libavcodec/nvdec_hevc.c b/libavcodec/nvdec_hevc.c >>> index b83d5edcf9..25319a1328 100644 >>> --- a/libavcodec/nvdec_hevc.c >>> +++ b/libavcodec/nvdec_hevc.c >>> @@ -274,11 +274,11 @@ static int >>> nvdec_hevc_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, >>> NVDECContext *ctx = avctx->internal->hwaccel_priv_data; >>> void *tmp; >>> - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, >>> + tmp = av_fast_realloc(ctx->bitstream_internal, >>> &ctx->bitstream_allocated, >>> ctx->bitstream_len + size + 3); >>> if (!tmp) >>> return AVERROR(ENOMEM); >>> - ctx->bitstream = tmp; >>> + ctx->bitstream = ctx->bitstream_internal = tmp; >>> tmp = av_fast_realloc(ctx->slice_offsets, >>> &ctx->slice_offsets_allocated, >>> (ctx->nb_slices + 1) * >>> sizeof(*ctx->slice_offsets)); >> >> bitstream_internal was added for the av1 slice reconstructions. >> Probably for the exact reason to avoid this confusion. >> >> Looking at this some more... With this patch in place, there should >> never be a situation in which we want to free/freep ctx->bitstream? >> So the freep() on it should probably be removed alongside, and replace >> with just setting it to NULL. > > That's what i did above, in the libavcodec/nvdec.c chunk. Ah, I somehow missed that!
diff --git a/libavcodec/nvdec.c b/libavcodec/nvdec.c index 27be644356..d13b790632 100644 --- a/libavcodec/nvdec.c +++ b/libavcodec/nvdec.c @@ -259,8 +259,8 @@ int ff_nvdec_decode_uninit(AVCodecContext *avctx) { NVDECContext *ctx = avctx->internal->hwaccel_priv_data; - av_freep(&ctx->bitstream); av_freep(&ctx->bitstream_internal); + ctx->bitstream = NULL; ctx->bitstream_len = 0; ctx->bitstream_allocated = 0; diff --git a/libavcodec/nvdec_h264.c b/libavcodec/nvdec_h264.c index f022619b64..8c72d5f4f7 100644 --- a/libavcodec/nvdec_h264.c +++ b/libavcodec/nvdec_h264.c @@ -138,11 +138,11 @@ static int nvdec_h264_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, const H264SliceContext *sl = &h->slice_ctx[0]; void *tmp; - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, + tmp = av_fast_realloc(ctx->bitstream_internal, &ctx->bitstream_allocated, ctx->bitstream_len + size + 3); if (!tmp) return AVERROR(ENOMEM); - ctx->bitstream = tmp; + ctx->bitstream = ctx->bitstream_internal = tmp; tmp = av_fast_realloc(ctx->slice_offsets, &ctx->slice_offsets_allocated, (ctx->nb_slices + 1) * sizeof(*ctx->slice_offsets)); diff --git a/libavcodec/nvdec_hevc.c b/libavcodec/nvdec_hevc.c index b83d5edcf9..25319a1328 100644 --- a/libavcodec/nvdec_hevc.c +++ b/libavcodec/nvdec_hevc.c @@ -274,11 +274,11 @@ static int nvdec_hevc_decode_slice(AVCodecContext *avctx, const uint8_t *buffer, NVDECContext *ctx = avctx->internal->hwaccel_priv_data; void *tmp; - tmp = av_fast_realloc(ctx->bitstream, &ctx->bitstream_allocated, + tmp = av_fast_realloc(ctx->bitstream_internal, &ctx->bitstream_allocated, ctx->bitstream_len + size + 3); if (!tmp) return AVERROR(ENOMEM); - ctx->bitstream = tmp; + ctx->bitstream = ctx->bitstream_internal = tmp; tmp = av_fast_realloc(ctx->slice_offsets, &ctx->slice_offsets_allocated, (ctx->nb_slices + 1) * sizeof(*ctx->slice_offsets));
Ensure all hwaccels that allocate a buffer use NVDECContext->bitstream_internal. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/nvdec.c | 2 +- libavcodec/nvdec_h264.c | 4 ++-- libavcodec/nvdec_hevc.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)