diff mbox series

[FFmpeg-devel] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP caps for encoders

Message ID 1585668840-30347-1-git-send-email-linjie.fu@intel.com
State Accepted
Commit 2b3206891649f317c20993411efef4bee39ae784
Headers show
Series [FFmpeg-devel] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP caps for encoders
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Fu, Linjie March 31, 2020, 3:34 p.m. UTC
ff_vaapi_encode_close() is not enough to free the resources like cbs
if initialization failure happens after codec->configure (except for
vp8/vp9).

We need to call avctx->codec->close() to deallocate, otherwise memory
leak happens.

Add FF_CODEC_CAP_INIT_CLEANUP for vaapi encoders and deallocate the
resources at free_and_end inside avcodec_open2().

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavcodec/vaapi_encode.c       | 1 -
 libavcodec/vaapi_encode_h264.c  | 1 +
 libavcodec/vaapi_encode_h265.c  | 1 +
 libavcodec/vaapi_encode_mjpeg.c | 1 +
 libavcodec/vaapi_encode_mpeg2.c | 1 +
 libavcodec/vaapi_encode_vp8.c   | 1 +
 libavcodec/vaapi_encode_vp9.c   | 1 +
 7 files changed, 6 insertions(+), 1 deletion(-)

Comments

Fu, Linjie April 23, 2020, 2:59 p.m. UTC | #1
> From: Fu, Linjie <linjie.fu@intel.com>
> Sent: Tuesday, March 31, 2020 23:34
> To: ffmpeg-devel@ffmpeg.org
> Cc: Fu, Linjie <linjie.fu@intel.com>
> Subject: [PATCH] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP
> caps for encoders
> 
> ff_vaapi_encode_close() is not enough to free the resources like cbs
> if initialization failure happens after codec->configure (except for
> vp8/vp9).
> 
> We need to call avctx->codec->close() to deallocate, otherwise memory
> leak happens.
> 
> Add FF_CODEC_CAP_INIT_CLEANUP for vaapi encoders and deallocate the
> resources at free_and_end inside avcodec_open2().
> 
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavcodec/vaapi_encode.c       | 1 -
>  libavcodec/vaapi_encode_h264.c  | 1 +
>  libavcodec/vaapi_encode_h265.c  | 1 +
>  libavcodec/vaapi_encode_mjpeg.c | 1 +
>  libavcodec/vaapi_encode_mpeg2.c | 1 +
>  libavcodec/vaapi_encode_vp8.c   | 1 +
>  libavcodec/vaapi_encode_vp9.c   | 1 +
>  7 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 8ff720e..d371033 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -2361,7 +2361,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext
> *avctx)
>      return 0;
> 
>  fail:
> -    ff_vaapi_encode_close(avctx);
>      return err;
>  }
> 
> diff --git a/libavcodec/vaapi_encode_h264.c
> b/libavcodec/vaapi_encode_h264.c
> index f4965d8..6a86905 100644
> --- a/libavcodec/vaapi_encode_h264.c
> +++ b/libavcodec/vaapi_encode_h264.c
> @@ -1356,6 +1356,7 @@ AVCodec ff_h264_vaapi_encoder = {
>      .close          = &vaapi_encode_h264_close,
>      .priv_class     = &vaapi_encode_h264_class,
>      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>      .defaults       = vaapi_encode_h264_defaults,
>      .pix_fmts = (const enum AVPixelFormat[]) {
>          AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_h265.c
> b/libavcodec/vaapi_encode_h265.c
> index 97dc5a7..4c24f7c 100644
> --- a/libavcodec/vaapi_encode_h265.c
> +++ b/libavcodec/vaapi_encode_h265.c
> @@ -1292,6 +1292,7 @@ AVCodec ff_hevc_vaapi_encoder = {
>      .close          = &vaapi_encode_h265_close,
>      .priv_class     = &vaapi_encode_h265_class,
>      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>      .defaults       = vaapi_encode_h265_defaults,
>      .pix_fmts = (const enum AVPixelFormat[]) {
>          AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_mjpeg.c
> b/libavcodec/vaapi_encode_mjpeg.c
> index bd029cc..c469e70 100644
> --- a/libavcodec/vaapi_encode_mjpeg.c
> +++ b/libavcodec/vaapi_encode_mjpeg.c
> @@ -565,6 +565,7 @@ AVCodec ff_mjpeg_vaapi_encoder = {
>      .priv_class     = &vaapi_encode_mjpeg_class,
>      .capabilities   = AV_CODEC_CAP_HARDWARE |
>                        AV_CODEC_CAP_INTRA_ONLY,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>      .defaults       = vaapi_encode_mjpeg_defaults,
>      .pix_fmts = (const enum AVPixelFormat[]) {
>          AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_mpeg2.c
> b/libavcodec/vaapi_encode_mpeg2.c
> index bac9ea1..55f9289 100644
> --- a/libavcodec/vaapi_encode_mpeg2.c
> +++ b/libavcodec/vaapi_encode_mpeg2.c
> @@ -702,6 +702,7 @@ AVCodec ff_mpeg2_vaapi_encoder = {
>      .close          = &vaapi_encode_mpeg2_close,
>      .priv_class     = &vaapi_encode_mpeg2_class,
>      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>      .defaults       = vaapi_encode_mpeg2_defaults,
>      .pix_fmts = (const enum AVPixelFormat[]) {
>          AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_vp8.c
> b/libavcodec/vaapi_encode_vp8.c
> index 6e7bf9d..d8fb031 100644
> --- a/libavcodec/vaapi_encode_vp8.c
> +++ b/libavcodec/vaapi_encode_vp8.c
> @@ -257,6 +257,7 @@ AVCodec ff_vp8_vaapi_encoder = {
>      .close          = &ff_vaapi_encode_close,
>      .priv_class     = &vaapi_encode_vp8_class,
>      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>      .defaults       = vaapi_encode_vp8_defaults,
>      .pix_fmts = (const enum AVPixelFormat[]) {
>          AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_vp9.c
> b/libavcodec/vaapi_encode_vp9.c
> index d7f415d..ea19470 100644
> --- a/libavcodec/vaapi_encode_vp9.c
> +++ b/libavcodec/vaapi_encode_vp9.c
> @@ -291,6 +291,7 @@ AVCodec ff_vp9_vaapi_encoder = {
>      .close          = &ff_vaapi_encode_close,
>      .priv_class     = &vaapi_encode_vp9_class,
>      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>      .defaults       = vaapi_encode_vp9_defaults,
>      .pix_fmts = (const enum AVPixelFormat[]) {
>          AV_PIX_FMT_VAAPI,
> --

Ping for review, this patch helps to fix the potential memory leaks.
Fu, Linjie May 4, 2020, 1:10 a.m. UTC | #2
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Fu,
> Linjie
> Sent: Thursday, April 23, 2020 22:59
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: add
> FF_CODEC_CAP_INIT_CLEANUP caps for encoders
> 
> > From: Fu, Linjie <linjie.fu@intel.com>
> > Sent: Tuesday, March 31, 2020 23:34
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Fu, Linjie <linjie.fu@intel.com>
> > Subject: [PATCH] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP
> > caps for encoders
> >
> > ff_vaapi_encode_close() is not enough to free the resources like cbs
> > if initialization failure happens after codec->configure (except for
> > vp8/vp9).
> >
> > We need to call avctx->codec->close() to deallocate, otherwise memory
> > leak happens.
> >
> > Add FF_CODEC_CAP_INIT_CLEANUP for vaapi encoders and deallocate the
> > resources at free_and_end inside avcodec_open2().
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> >  libavcodec/vaapi_encode.c       | 1 -
> >  libavcodec/vaapi_encode_h264.c  | 1 +
> >  libavcodec/vaapi_encode_h265.c  | 1 +
> >  libavcodec/vaapi_encode_mjpeg.c | 1 +
> >  libavcodec/vaapi_encode_mpeg2.c | 1 +
> >  libavcodec/vaapi_encode_vp8.c   | 1 +
> >  libavcodec/vaapi_encode_vp9.c   | 1 +
> >  7 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> > index 8ff720e..d371033 100644
> > --- a/libavcodec/vaapi_encode.c
> > +++ b/libavcodec/vaapi_encode.c
> > @@ -2361,7 +2361,6 @@ av_cold int
> ff_vaapi_encode_init(AVCodecContext
> > *avctx)
> >      return 0;
> >
> >  fail:
> > -    ff_vaapi_encode_close(avctx);
> >      return err;
> >  }
> >
> > diff --git a/libavcodec/vaapi_encode_h264.c
> > b/libavcodec/vaapi_encode_h264.c
> > index f4965d8..6a86905 100644
> > --- a/libavcodec/vaapi_encode_h264.c
> > +++ b/libavcodec/vaapi_encode_h264.c
> > @@ -1356,6 +1356,7 @@ AVCodec ff_h264_vaapi_encoder = {
> >      .close          = &vaapi_encode_h264_close,
> >      .priv_class     = &vaapi_encode_h264_class,
> >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> > +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >      .defaults       = vaapi_encode_h264_defaults,
> >      .pix_fmts = (const enum AVPixelFormat[]) {
> >          AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_h265.c
> > b/libavcodec/vaapi_encode_h265.c
> > index 97dc5a7..4c24f7c 100644
> > --- a/libavcodec/vaapi_encode_h265.c
> > +++ b/libavcodec/vaapi_encode_h265.c
> > @@ -1292,6 +1292,7 @@ AVCodec ff_hevc_vaapi_encoder = {
> >      .close          = &vaapi_encode_h265_close,
> >      .priv_class     = &vaapi_encode_h265_class,
> >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> > +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >      .defaults       = vaapi_encode_h265_defaults,
> >      .pix_fmts = (const enum AVPixelFormat[]) {
> >          AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_mjpeg.c
> > b/libavcodec/vaapi_encode_mjpeg.c
> > index bd029cc..c469e70 100644
> > --- a/libavcodec/vaapi_encode_mjpeg.c
> > +++ b/libavcodec/vaapi_encode_mjpeg.c
> > @@ -565,6 +565,7 @@ AVCodec ff_mjpeg_vaapi_encoder = {
> >      .priv_class     = &vaapi_encode_mjpeg_class,
> >      .capabilities   = AV_CODEC_CAP_HARDWARE |
> >                        AV_CODEC_CAP_INTRA_ONLY,
> > +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >      .defaults       = vaapi_encode_mjpeg_defaults,
> >      .pix_fmts = (const enum AVPixelFormat[]) {
> >          AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_mpeg2.c
> > b/libavcodec/vaapi_encode_mpeg2.c
> > index bac9ea1..55f9289 100644
> > --- a/libavcodec/vaapi_encode_mpeg2.c
> > +++ b/libavcodec/vaapi_encode_mpeg2.c
> > @@ -702,6 +702,7 @@ AVCodec ff_mpeg2_vaapi_encoder = {
> >      .close          = &vaapi_encode_mpeg2_close,
> >      .priv_class     = &vaapi_encode_mpeg2_class,
> >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> > +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >      .defaults       = vaapi_encode_mpeg2_defaults,
> >      .pix_fmts = (const enum AVPixelFormat[]) {
> >          AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_vp8.c
> > b/libavcodec/vaapi_encode_vp8.c
> > index 6e7bf9d..d8fb031 100644
> > --- a/libavcodec/vaapi_encode_vp8.c
> > +++ b/libavcodec/vaapi_encode_vp8.c
> > @@ -257,6 +257,7 @@ AVCodec ff_vp8_vaapi_encoder = {
> >      .close          = &ff_vaapi_encode_close,
> >      .priv_class     = &vaapi_encode_vp8_class,
> >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> > +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >      .defaults       = vaapi_encode_vp8_defaults,
> >      .pix_fmts = (const enum AVPixelFormat[]) {
> >          AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_vp9.c
> > b/libavcodec/vaapi_encode_vp9.c
> > index d7f415d..ea19470 100644
> > --- a/libavcodec/vaapi_encode_vp9.c
> > +++ b/libavcodec/vaapi_encode_vp9.c
> > @@ -291,6 +291,7 @@ AVCodec ff_vp9_vaapi_encoder = {
> >      .close          = &ff_vaapi_encode_close,
> >      .priv_class     = &vaapi_encode_vp9_class,
> >      .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> > +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >      .defaults       = vaapi_encode_vp9_defaults,
> >      .pix_fmts = (const enum AVPixelFormat[]) {
> >          AV_PIX_FMT_VAAPI,
> > --
> 
> Ping for review, this patch helps to fix the potential memory leaks.
> 
Ping.

- Linjie
Timo Rothenpieler May 4, 2020, 5:19 a.m. UTC | #3
On 31.03.2020 17:34, Linjie Fu wrote:
> ff_vaapi_encode_close() is not enough to free the resources like cbs
> if initialization failure happens after codec->configure (except for
> vp8/vp9).
> 
> We need to call avctx->codec->close() to deallocate, otherwise memory
> leak happens.
> 
> Add FF_CODEC_CAP_INIT_CLEANUP for vaapi encoders and deallocate the
> resources at free_and_end inside avcodec_open2().
> 
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>

Not my area of code, but the patch looks straight forward enough to my eyes.
Fu, Linjie May 4, 2020, 3:32 p.m. UTC | #4
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Timo Rothenpieler
> Sent: Monday, May 4, 2020 13:20
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: add
> FF_CODEC_CAP_INIT_CLEANUP caps for encoders
> 
> On 31.03.2020 17:34, Linjie Fu wrote:
> > ff_vaapi_encode_close() is not enough to free the resources like cbs
> > if initialization failure happens after codec->configure (except for
> > vp8/vp9).
> >
> > We need to call avctx->codec->close() to deallocate, otherwise memory
> > leak happens.
> >
> > Add FF_CODEC_CAP_INIT_CLEANUP for vaapi encoders and deallocate the
> > resources at free_and_end inside avcodec_open2().
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> 
> Not my area of code, but the patch looks straight forward enough to my eyes.

Thanks for review, and hope this could be merged soon.

- Linjie
diff mbox series

Patch

diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index 8ff720e..d371033 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -2361,7 +2361,6 @@  av_cold int ff_vaapi_encode_init(AVCodecContext *avctx)
     return 0;
 
 fail:
-    ff_vaapi_encode_close(avctx);
     return err;
 }
 
diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
index f4965d8..6a86905 100644
--- a/libavcodec/vaapi_encode_h264.c
+++ b/libavcodec/vaapi_encode_h264.c
@@ -1356,6 +1356,7 @@  AVCodec ff_h264_vaapi_encoder = {
     .close          = &vaapi_encode_h264_close,
     .priv_class     = &vaapi_encode_h264_class,
     .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
+    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
     .defaults       = vaapi_encode_h264_defaults,
     .pix_fmts = (const enum AVPixelFormat[]) {
         AV_PIX_FMT_VAAPI,
diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c
index 97dc5a7..4c24f7c 100644
--- a/libavcodec/vaapi_encode_h265.c
+++ b/libavcodec/vaapi_encode_h265.c
@@ -1292,6 +1292,7 @@  AVCodec ff_hevc_vaapi_encoder = {
     .close          = &vaapi_encode_h265_close,
     .priv_class     = &vaapi_encode_h265_class,
     .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
+    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
     .defaults       = vaapi_encode_h265_defaults,
     .pix_fmts = (const enum AVPixelFormat[]) {
         AV_PIX_FMT_VAAPI,
diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c
index bd029cc..c469e70 100644
--- a/libavcodec/vaapi_encode_mjpeg.c
+++ b/libavcodec/vaapi_encode_mjpeg.c
@@ -565,6 +565,7 @@  AVCodec ff_mjpeg_vaapi_encoder = {
     .priv_class     = &vaapi_encode_mjpeg_class,
     .capabilities   = AV_CODEC_CAP_HARDWARE |
                       AV_CODEC_CAP_INTRA_ONLY,
+    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
     .defaults       = vaapi_encode_mjpeg_defaults,
     .pix_fmts = (const enum AVPixelFormat[]) {
         AV_PIX_FMT_VAAPI,
diff --git a/libavcodec/vaapi_encode_mpeg2.c b/libavcodec/vaapi_encode_mpeg2.c
index bac9ea1..55f9289 100644
--- a/libavcodec/vaapi_encode_mpeg2.c
+++ b/libavcodec/vaapi_encode_mpeg2.c
@@ -702,6 +702,7 @@  AVCodec ff_mpeg2_vaapi_encoder = {
     .close          = &vaapi_encode_mpeg2_close,
     .priv_class     = &vaapi_encode_mpeg2_class,
     .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
+    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
     .defaults       = vaapi_encode_mpeg2_defaults,
     .pix_fmts = (const enum AVPixelFormat[]) {
         AV_PIX_FMT_VAAPI,
diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c
index 6e7bf9d..d8fb031 100644
--- a/libavcodec/vaapi_encode_vp8.c
+++ b/libavcodec/vaapi_encode_vp8.c
@@ -257,6 +257,7 @@  AVCodec ff_vp8_vaapi_encoder = {
     .close          = &ff_vaapi_encode_close,
     .priv_class     = &vaapi_encode_vp8_class,
     .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
+    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
     .defaults       = vaapi_encode_vp8_defaults,
     .pix_fmts = (const enum AVPixelFormat[]) {
         AV_PIX_FMT_VAAPI,
diff --git a/libavcodec/vaapi_encode_vp9.c b/libavcodec/vaapi_encode_vp9.c
index d7f415d..ea19470 100644
--- a/libavcodec/vaapi_encode_vp9.c
+++ b/libavcodec/vaapi_encode_vp9.c
@@ -291,6 +291,7 @@  AVCodec ff_vp9_vaapi_encoder = {
     .close          = &ff_vaapi_encode_close,
     .priv_class     = &vaapi_encode_vp9_class,
     .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
+    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
     .defaults       = vaapi_encode_vp9_defaults,
     .pix_fmts = (const enum AVPixelFormat[]) {
         AV_PIX_FMT_VAAPI,