diff mbox series

[FFmpeg-devel,1/3] lavfi/dnn: fix corruption when TF backend infer failed

Message ID 20230316030016.4096-1-ting.fu@intel.com
State New
Headers show
Series [FFmpeg-devel,1/3] lavfi/dnn: fix corruption when TF backend infer failed | expand

Checks

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

Commit Message

Ting Fu March 16, 2023, 3 a.m. UTC 
Signed-off-by: Ting Fu <ting.fu@intel.com>
---
 libavfilter/dnn/dnn_backend_tf.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Guo, Yejun March 24, 2023, 3:33 a.m. UTC | #1 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Ting
> Fu
> Sent: Thursday, March 16, 2023 11:00 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 1/3] lavfi/dnn: fix corruption when TF
> backend infer failed
> 
> Signed-off-by: Ting Fu <ting.fu@intel.com>
> ---
>  libavfilter/dnn/dnn_backend_tf.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/libavfilter/dnn/dnn_backend_tf.c
> b/libavfilter/dnn/dnn_backend_tf.c
> index 3b5084b67b..fb1a5f1350 100644
> --- a/libavfilter/dnn/dnn_backend_tf.c
> +++ b/libavfilter/dnn/dnn_backend_tf.c
> @@ -176,9 +176,6 @@ static int tf_start_inference(void *args)
>      if (TF_GetCode(request->status) != TF_OK) {
>          av_log(&tf_model->ctx, AV_LOG_ERROR, "%s", TF_Message(request-
> >status));
>          tf_free_request(infer_request);

Please also delete this line since it is handled in execute_model_tf which
handles the error case.

> -        if (ff_safe_queue_push_back(tf_model->request_queue, request) < 0) {
> -            destroy_request_item(&request);
> -        }
>          return DNN_GENERIC_ERROR;
>      }
>      return 0;
> @@ -439,7 +436,9 @@ static int load_tf_model(TFModel *tf_model, const
> char *model_filename)
>      TF_DeleteBuffer(graph_def);
>      if (TF_GetCode(tf_model->status) != TF_OK){
>          TF_DeleteGraph(tf_model->graph);
> +        tf_model->graph = NULL;
>          TF_DeleteStatus(tf_model->status);
> +        tf_model->status = NULL;
>          av_log(ctx, AV_LOG_ERROR, "Failed to import serialized graph to model
> graph\n");
>          av_freep(&sess_config);
>          return DNN_GENERIC_ERROR;
> @@ -453,7 +452,7 @@ static int load_tf_model(TFModel *tf_model, const
> char *model_filename)
>          av_freep(&sess_config);
>          if (TF_GetCode(tf_model->status) != TF_OK) {
>              TF_DeleteGraph(tf_model->graph);

Please also delete this line since ff_dnn_free_model_tf handles the error case.

> -            TF_DeleteStatus(tf_model->status);
> +            tf_model->graph = NULL;

And this line is not needed now.

>              TF_DeleteSessionOptions(sess_opts);
>              av_log(ctx, AV_LOG_ERROR, "Failed to set config for sess options
> with %s\n",
>                                        tf_model->ctx.options.sess_config);
> @@ -466,7 +465,7 @@ static int load_tf_model(TFModel *tf_model, const
> char *model_filename)
>      if (TF_GetCode(tf_model->status) != TF_OK)
>      {

>          TF_DeleteGraph(tf_model->graph);
> -        TF_DeleteStatus(tf_model->status);
> +        tf_model->graph = NULL;

Same comments as above.

>          av_log(ctx, AV_LOG_ERROR, "Failed to create new session with model
> graph\n");
>          return DNN_GENERIC_ERROR;
>      }
> @@ -480,8 +479,11 @@ static int load_tf_model(TFModel *tf_model, const
> char *model_filename)
>          if (TF_GetCode(tf_model->status) != TF_OK)
>          {


>              TF_DeleteSession(tf_model->session, tf_model->status);
Please also delete this line since ff_dnn_free_model_tf handles the error case.

> +            tf_model->session = NULL;
>              TF_DeleteGraph(tf_model->graph);
> +            tf_model->graph = NULL;
>              TF_DeleteStatus(tf_model->status);
> +            tf_model->status = NULL;

Same comments as above.

>              av_log(ctx, AV_LOG_ERROR, "Failed to run session when
> initializing\n");
>              return DNN_GENERIC_ERROR;
>          }
> @@ -940,6 +942,7 @@ DNNModel *ff_dnn_load_model_tf(const char
> *model_filename, DNNFunctionType func_
> 
>      return model;
>  err:
> +    model->model = tf_model;

Please set model->model = tf_model when tf_model is successfully created
and also delete original line 934

>      ff_dnn_free_model_tf(&model);
>      return NULL;
>  }
> --
> 2.25.1
> 
> _______________________________________________
> 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".
Ting Fu March 24, 2023, 7:49 a.m. UTC | #2 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Guo,
> Yejun
> Sent: Friday, March 24, 2023 11:34 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavfi/dnn: fix corruption when TF
> backend infer failed
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Ting
> > Fu
> > Sent: Thursday, March 16, 2023 11:00 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: [FFmpeg-devel] [PATCH 1/3] lavfi/dnn: fix corruption when TF
> > backend infer failed
> >
> > Signed-off-by: Ting Fu <ting.fu@intel.com>
> > ---
> >  libavfilter/dnn/dnn_backend_tf.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/libavfilter/dnn/dnn_backend_tf.c
> > b/libavfilter/dnn/dnn_backend_tf.c
> > index 3b5084b67b..fb1a5f1350 100644
> > --- a/libavfilter/dnn/dnn_backend_tf.c
> > +++ b/libavfilter/dnn/dnn_backend_tf.c
> > @@ -176,9 +176,6 @@ static int tf_start_inference(void *args)
> >      if (TF_GetCode(request->status) != TF_OK) {
> >          av_log(&tf_model->ctx, AV_LOG_ERROR, "%s",
> > TF_Message(request-
> > >status));
> >          tf_free_request(infer_request);
> 
> Please also delete this line since it is handled in execute_model_tf which
> handles the error case.
> 
[...]
Hi Yejun,
Thank you for your review.
PATCH 1/3 was modified as comments in PATCH V2.
> > --
> > 2.25.1
> >
> > _______________________________________________
> > 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".
> _______________________________________________
> 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/libavfilter/dnn/dnn_backend_tf.c b/libavfilter/dnn/dnn_backend_tf.c
index 3b5084b67b..fb1a5f1350 100644
--- a/libavfilter/dnn/dnn_backend_tf.c
+++ b/libavfilter/dnn/dnn_backend_tf.c
@@ -176,9 +176,6 @@  static int tf_start_inference(void *args)
     if (TF_GetCode(request->status) != TF_OK) {
         av_log(&tf_model->ctx, AV_LOG_ERROR, "%s", TF_Message(request->status));
         tf_free_request(infer_request);
-        if (ff_safe_queue_push_back(tf_model->request_queue, request) < 0) {
-            destroy_request_item(&request);
-        }
         return DNN_GENERIC_ERROR;
     }
     return 0;
@@ -439,7 +436,9 @@  static int load_tf_model(TFModel *tf_model, const char *model_filename)
     TF_DeleteBuffer(graph_def);
     if (TF_GetCode(tf_model->status) != TF_OK){
         TF_DeleteGraph(tf_model->graph);
+        tf_model->graph = NULL;
         TF_DeleteStatus(tf_model->status);
+        tf_model->status = NULL;
         av_log(ctx, AV_LOG_ERROR, "Failed to import serialized graph to model graph\n");
         av_freep(&sess_config);
         return DNN_GENERIC_ERROR;
@@ -453,7 +452,7 @@  static int load_tf_model(TFModel *tf_model, const char *model_filename)
         av_freep(&sess_config);
         if (TF_GetCode(tf_model->status) != TF_OK) {
             TF_DeleteGraph(tf_model->graph);
-            TF_DeleteStatus(tf_model->status);
+            tf_model->graph = NULL;
             TF_DeleteSessionOptions(sess_opts);
             av_log(ctx, AV_LOG_ERROR, "Failed to set config for sess options with %s\n",
                                       tf_model->ctx.options.sess_config);
@@ -466,7 +465,7 @@  static int load_tf_model(TFModel *tf_model, const char *model_filename)
     if (TF_GetCode(tf_model->status) != TF_OK)
     {
         TF_DeleteGraph(tf_model->graph);
-        TF_DeleteStatus(tf_model->status);
+        tf_model->graph = NULL;
         av_log(ctx, AV_LOG_ERROR, "Failed to create new session with model graph\n");
         return DNN_GENERIC_ERROR;
     }
@@ -480,8 +479,11 @@  static int load_tf_model(TFModel *tf_model, const char *model_filename)
         if (TF_GetCode(tf_model->status) != TF_OK)
         {
             TF_DeleteSession(tf_model->session, tf_model->status);
+            tf_model->session = NULL;
             TF_DeleteGraph(tf_model->graph);
+            tf_model->graph = NULL;
             TF_DeleteStatus(tf_model->status);
+            tf_model->status = NULL;
             av_log(ctx, AV_LOG_ERROR, "Failed to run session when initializing\n");
             return DNN_GENERIC_ERROR;
         }
@@ -940,6 +942,7 @@  DNNModel *ff_dnn_load_model_tf(const char *model_filename, DNNFunctionType func_
 
     return model;
 err:
+    model->model = tf_model;
     ff_dnn_free_model_tf(&model);
     return NULL;
 }