diff mbox series

[FFmpeg-devel,2/5] avcodec/libsvtav1: remove unneeded svt_av1_enc_deinit_handle()

Message ID 1596280963-15526-2-git-send-email-lance.lmwang@gmail.com
State Accepted
Commit 392a2d0790336369185b7f2e0435844a368c1281
Headers show
Series [FFmpeg-devel,1/5] avcodec/libsvtav1: fix copy and paste error | expand

Checks

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

Commit Message

Lance Wang Aug. 1, 2020, 11:22 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/libsvtav1.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Mark Thompson Aug. 1, 2020, 12:55 p.m. UTC | #1 
On 01/08/2020 12:22, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>   libavcodec/libsvtav1.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index d34c6b3..eb6043b 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -273,23 +273,17 @@ static av_cold int eb_enc_init(AVCodecContext *avctx)
>   
>       ret = config_enc_params(&svt_enc->enc_params, avctx);
>       if (ret < 0) {
> -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> -        svt_enc->svt_handle = NULL;
>           av_log(avctx, AV_LOG_ERROR, "Error configuring encoder parameters\n");
>           return ret;
>       }
>   
>       svt_ret = svt_av1_enc_set_parameter(svt_enc->svt_handle, &svt_enc->enc_params);
>       if (svt_ret != EB_ErrorNone) {
> -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> -        svt_enc->svt_handle = NULL;
>           return svt_print_error(avctx, svt_ret, "Error setting encoder parameters");
>       }
>   
>       svt_ret = svt_av1_enc_init(svt_enc->svt_handle);
>       if (svt_ret != EB_ErrorNone) {
> -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> -        svt_enc->svt_handle = NULL;
>           return svt_print_error(avctx, svt_ret, "Error initializing encoder");
>       }
>   

Maybe mention in the commit message why this is ok, because it's not immediately obvious.  (Because of INIT_CLEANUP.)

LGTM.

Thanks,

- Mark
James Almer Aug. 1, 2020, 1:09 p.m. UTC | #2 
On 8/1/2020 8:22 AM, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavcodec/libsvtav1.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index d34c6b3..eb6043b 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -273,23 +273,17 @@ static av_cold int eb_enc_init(AVCodecContext *avctx)
>  
>      ret = config_enc_params(&svt_enc->enc_params, avctx);
>      if (ret < 0) {
> -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> -        svt_enc->svt_handle = NULL;
>          av_log(avctx, AV_LOG_ERROR, "Error configuring encoder parameters\n");
>          return ret;
>      }
>  
>      svt_ret = svt_av1_enc_set_parameter(svt_enc->svt_handle, &svt_enc->enc_params);
>      if (svt_ret != EB_ErrorNone) {
> -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> -        svt_enc->svt_handle = NULL;
>          return svt_print_error(avctx, svt_ret, "Error setting encoder parameters");
>      }
>  
>      svt_ret = svt_av1_enc_init(svt_enc->svt_handle);
>      if (svt_ret != EB_ErrorNone) {
> -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> -        svt_enc->svt_handle = NULL;
>          return svt_print_error(avctx, svt_ret, "Error initializing encoder");
>      }

Did you try to trigger one of these paths without the code you're
removing? Afaik these were added because otherwise the library would
segfault during close(), because svt_av1_enc_deinit() was called on it
on an scenario where svt_av1_enc_init() hadn't ever been called.

A proper change would be to ensure the above doesn't happen.
Lance Wang Aug. 1, 2020, 3:13 p.m. UTC | #3 
On Sat, Aug 01, 2020 at 10:09:20AM -0300, James Almer wrote:
> On 8/1/2020 8:22 AM, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavcodec/libsvtav1.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> > index d34c6b3..eb6043b 100644
> > --- a/libavcodec/libsvtav1.c
> > +++ b/libavcodec/libsvtav1.c
> > @@ -273,23 +273,17 @@ static av_cold int eb_enc_init(AVCodecContext *avctx)
> >  
> >      ret = config_enc_params(&svt_enc->enc_params, avctx);
> >      if (ret < 0) {
> > -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> > -        svt_enc->svt_handle = NULL;
> >          av_log(avctx, AV_LOG_ERROR, "Error configuring encoder parameters\n");
> >          return ret;
> >      }
> >  
> >      svt_ret = svt_av1_enc_set_parameter(svt_enc->svt_handle, &svt_enc->enc_params);
> >      if (svt_ret != EB_ErrorNone) {
> > -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> > -        svt_enc->svt_handle = NULL;
> >          return svt_print_error(avctx, svt_ret, "Error setting encoder parameters");
> >      }
> >  
> >      svt_ret = svt_av1_enc_init(svt_enc->svt_handle);
> >      if (svt_ret != EB_ErrorNone) {
> > -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> > -        svt_enc->svt_handle = NULL;
> >          return svt_print_error(avctx, svt_ret, "Error initializing encoder");
> >      }
> 
> Did you try to trigger one of these paths without the code you're
> removing? Afaik these were added because otherwise the library would
> segfault during close(), because svt_av1_enc_deinit() was called on it
> on an scenario where svt_av1_enc_init() hadn't ever been called.
> 
> A proper change would be to ensure the above doesn't happen.

Yes, I'm change the ret/svt_ret to -1 to trigger the code return with error and haven't got
segfault, how to reproduce the issue? If you check the code of the library,  svt_av1_enc_init_handle()
will call svt_av1_enc_deinit() also before it'll return errors, in fact,  svt_av1_enc_init() haven't 
been called also. So I think if have any segfault, we should fixed in the libary as svt_av1_enc_init()
may failed in the middle of its code also. 

> _______________________________________________
> 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".
Lance Wang Aug. 11, 2020, 2:27 p.m. UTC | #4 
On Sat, Aug 01, 2020 at 10:09:20AM -0300, James Almer wrote:
> On 8/1/2020 8:22 AM, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavcodec/libsvtav1.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> > index d34c6b3..eb6043b 100644
> > --- a/libavcodec/libsvtav1.c
> > +++ b/libavcodec/libsvtav1.c
> > @@ -273,23 +273,17 @@ static av_cold int eb_enc_init(AVCodecContext *avctx)
> >  
> >      ret = config_enc_params(&svt_enc->enc_params, avctx);
> >      if (ret < 0) {
> > -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> > -        svt_enc->svt_handle = NULL;
> >          av_log(avctx, AV_LOG_ERROR, "Error configuring encoder parameters\n");
> >          return ret;
> >      }
> >  
> >      svt_ret = svt_av1_enc_set_parameter(svt_enc->svt_handle, &svt_enc->enc_params);
> >      if (svt_ret != EB_ErrorNone) {
> > -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> > -        svt_enc->svt_handle = NULL;
> >          return svt_print_error(avctx, svt_ret, "Error setting encoder parameters");
> >      }
> >  
> >      svt_ret = svt_av1_enc_init(svt_enc->svt_handle);
> >      if (svt_ret != EB_ErrorNone) {
> > -        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
> > -        svt_enc->svt_handle = NULL;
> >          return svt_print_error(avctx, svt_ret, "Error initializing encoder");
> >      }
> 
> Did you try to trigger one of these paths without the code you're
> removing? Afaik these were added because otherwise the library would
> segfault during close(), because svt_av1_enc_deinit() was called on it
> on an scenario where svt_av1_enc_init() hadn't ever been called.
> 
> A proper change would be to ensure the above doesn't happen.

Can the patch is OK to apply for I haven't reproduce the segfault.

> _______________________________________________
> 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 mbox series

Patch

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index d34c6b3..eb6043b 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -273,23 +273,17 @@  static av_cold int eb_enc_init(AVCodecContext *avctx)
 
     ret = config_enc_params(&svt_enc->enc_params, avctx);
     if (ret < 0) {
-        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
-        svt_enc->svt_handle = NULL;
         av_log(avctx, AV_LOG_ERROR, "Error configuring encoder parameters\n");
         return ret;
     }
 
     svt_ret = svt_av1_enc_set_parameter(svt_enc->svt_handle, &svt_enc->enc_params);
     if (svt_ret != EB_ErrorNone) {
-        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
-        svt_enc->svt_handle = NULL;
         return svt_print_error(avctx, svt_ret, "Error setting encoder parameters");
     }
 
     svt_ret = svt_av1_enc_init(svt_enc->svt_handle);
     if (svt_ret != EB_ErrorNone) {
-        svt_av1_enc_deinit_handle(svt_enc->svt_handle);
-        svt_enc->svt_handle = NULL;
         return svt_print_error(avctx, svt_ret, "Error initializing encoder");
     }