diff mbox series

[FFmpeg-devel,v2,4/5] avcodec/mpegvideo: return more specific error codes for ff_mpv_common_init()

Message ID 1588850057-24236-4-git-send-email-lance.lmwang@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v2,1/5] avcodec/mpegvideo_enc: reindent code | expand

Checks

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

Commit Message

Lance Wang May 7, 2020, 11:14 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/mpegvideo.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Marton Balint May 7, 2020, 6:34 p.m. UTC | #1 
On Thu, 7 May 2020, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> libavcodec/mpegvideo.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> index 22cab28..b5ddb1b 100644
> --- a/libavcodec/mpegvideo.c
> +++ b/libavcodec/mpegvideo.c
> @@ -889,7 +889,7 @@ static void clear_context(MpegEncContext *s)
>  */
> av_cold int ff_mpv_common_init(MpegEncContext *s)
> {
> -    int i, ret;
> +    int i, ret = AVERROR(ENOMEM);

This initialization is unnecessary here, you will overwrite ret later.

>     int nb_slices = (HAVE_THREADS &&
>                      s->avctx->active_thread_type & FF_THREAD_SLICE) ?
>                     s->avctx->thread_count : 1;
> @@ -907,7 +907,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
>     if (s->avctx->pix_fmt == AV_PIX_FMT_NONE) {
>         av_log(s->avctx, AV_LOG_ERROR,
>                "decoding to AV_PIX_FMT_NONE is not supported.\n");
> -        return -1;
> +        return AVERROR(EINVAL);
>     }
>
>     if (nb_slices > MAX_THREADS || (nb_slices > s->mb_height && s->mb_height)) {
> @@ -923,7 +923,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
>
>     if ((s->width || s->height) &&
>         av_image_check_size(s->width, s->height, 0, s->avctx))
> -        return -1;
> +        return AVERROR(EINVAL);
>
>     dct_init(s);
> 
> @@ -954,7 +954,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
>     if (!s->new_picture.f)
>         goto fail;
> 
> -    if (init_context_frame(s))
> +    if ((ret = init_context_frame(s)))
>         goto fail;
>
>     s->parse_context.state = -1;
> @@ -971,7 +971,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
>                 if (!s->thread_context[i])
>                     goto fail;
>             }
> -            if (init_duplicate_context(s->thread_context[i]) < 0)
> +            if ((ret = init_duplicate_context(s->thread_context[i])) < 0)
>                 goto fail;
>             s->thread_context[i]->start_mb_y =
>                 (s->mb_height * (i) + nb_slices / 2) / nb_slices;
> @@ -979,7 +979,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
>                 (s->mb_height * (i + 1) + nb_slices / 2) / nb_slices;
>         }
>     } else {
> -        if (init_duplicate_context(s) < 0)
> +        if ((ret = init_duplicate_context(s)) < 0)
>             goto fail;
>         s->start_mb_y = 0;
>         s->end_mb_y   = s->mb_height;
> @@ -990,7 +990,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
>     return 0;
>  fail:
>     ff_mpv_common_end(s);
> -    return -1;
> +    return ret

Not all goto fail set ret. I think it is cleaner if you add a new label 
"fail_nomem" before the fail label and set ret to ENOMEM there, and use 
that label for ENOMEM cases.

Regards,
Marton
Lance Wang May 7, 2020, 11:02 p.m. UTC | #2 
On Thu, May 07, 2020 at 08:34:44PM +0200, Marton Balint wrote:
> 
> 
> On Thu, 7 May 2020, lance.lmwang@gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> > libavcodec/mpegvideo.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> > index 22cab28..b5ddb1b 100644
> > --- a/libavcodec/mpegvideo.c
> > +++ b/libavcodec/mpegvideo.c
> > @@ -889,7 +889,7 @@ static void clear_context(MpegEncContext *s)
> >  */
> > av_cold int ff_mpv_common_init(MpegEncContext *s)
> > {
> > -    int i, ret;
> > +    int i, ret = AVERROR(ENOMEM);
> 
> This initialization is unnecessary here, you will overwrite ret later.
> 
> >     int nb_slices = (HAVE_THREADS &&
> >                      s->avctx->active_thread_type & FF_THREAD_SLICE) ?
> >                     s->avctx->thread_count : 1;
> > @@ -907,7 +907,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
> >     if (s->avctx->pix_fmt == AV_PIX_FMT_NONE) {
> >         av_log(s->avctx, AV_LOG_ERROR,
> >                "decoding to AV_PIX_FMT_NONE is not supported.\n");
> > -        return -1;
> > +        return AVERROR(EINVAL);
> >     }
> > 
> >     if (nb_slices > MAX_THREADS || (nb_slices > s->mb_height && s->mb_height)) {
> > @@ -923,7 +923,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
> > 
> >     if ((s->width || s->height) &&
> >         av_image_check_size(s->width, s->height, 0, s->avctx))
> > -        return -1;
> > +        return AVERROR(EINVAL);
> > 
> >     dct_init(s);
> > 
> > @@ -954,7 +954,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
> >     if (!s->new_picture.f)
> >         goto fail;
> > 
> > -    if (init_context_frame(s))
> > +    if ((ret = init_context_frame(s)))
> >         goto fail;
> > 
> >     s->parse_context.state = -1;
> > @@ -971,7 +971,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
> >                 if (!s->thread_context[i])
> >                     goto fail;
> >             }
> > -            if (init_duplicate_context(s->thread_context[i]) < 0)
> > +            if ((ret = init_duplicate_context(s->thread_context[i])) < 0)
> >                 goto fail;
> >             s->thread_context[i]->start_mb_y =
> >                 (s->mb_height * (i) + nb_slices / 2) / nb_slices;
> > @@ -979,7 +979,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
> >                 (s->mb_height * (i + 1) + nb_slices / 2) / nb_slices;
> >         }
> >     } else {
> > -        if (init_duplicate_context(s) < 0)
> > +        if ((ret = init_duplicate_context(s)) < 0)
> >             goto fail;
> >         s->start_mb_y = 0;
> >         s->end_mb_y   = s->mb_height;
> > @@ -990,7 +990,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
> >     return 0;
> >  fail:
> >     ff_mpv_common_end(s);
> > -    return -1;
> > +    return ret
> 
> Not all goto fail set ret. I think it is cleaner if you add a new label
> "fail_nomem" before the fail label and set ret to ENOMEM there, and use that
> label for ENOMEM cases.

thanks for review, will update to add fail_nomem label.
> 
> Regards,
> Marton
> _______________________________________________
> 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/mpegvideo.c b/libavcodec/mpegvideo.c
index 22cab28..b5ddb1b 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -889,7 +889,7 @@  static void clear_context(MpegEncContext *s)
  */
 av_cold int ff_mpv_common_init(MpegEncContext *s)
 {
-    int i, ret;
+    int i, ret = AVERROR(ENOMEM);
     int nb_slices = (HAVE_THREADS &&
                      s->avctx->active_thread_type & FF_THREAD_SLICE) ?
                     s->avctx->thread_count : 1;
@@ -907,7 +907,7 @@  av_cold int ff_mpv_common_init(MpegEncContext *s)
     if (s->avctx->pix_fmt == AV_PIX_FMT_NONE) {
         av_log(s->avctx, AV_LOG_ERROR,
                "decoding to AV_PIX_FMT_NONE is not supported.\n");
-        return -1;
+        return AVERROR(EINVAL);
     }
 
     if (nb_slices > MAX_THREADS || (nb_slices > s->mb_height && s->mb_height)) {
@@ -923,7 +923,7 @@  av_cold int ff_mpv_common_init(MpegEncContext *s)
 
     if ((s->width || s->height) &&
         av_image_check_size(s->width, s->height, 0, s->avctx))
-        return -1;
+        return AVERROR(EINVAL);
 
     dct_init(s);
 
@@ -954,7 +954,7 @@  av_cold int ff_mpv_common_init(MpegEncContext *s)
     if (!s->new_picture.f)
         goto fail;
 
-    if (init_context_frame(s))
+    if ((ret = init_context_frame(s)))
         goto fail;
 
     s->parse_context.state = -1;
@@ -971,7 +971,7 @@  av_cold int ff_mpv_common_init(MpegEncContext *s)
                 if (!s->thread_context[i])
                     goto fail;
             }
-            if (init_duplicate_context(s->thread_context[i]) < 0)
+            if ((ret = init_duplicate_context(s->thread_context[i])) < 0)
                 goto fail;
             s->thread_context[i]->start_mb_y =
                 (s->mb_height * (i) + nb_slices / 2) / nb_slices;
@@ -979,7 +979,7 @@  av_cold int ff_mpv_common_init(MpegEncContext *s)
                 (s->mb_height * (i + 1) + nb_slices / 2) / nb_slices;
         }
     } else {
-        if (init_duplicate_context(s) < 0)
+        if ((ret = init_duplicate_context(s)) < 0)
             goto fail;
         s->start_mb_y = 0;
         s->end_mb_y   = s->mb_height;
@@ -990,7 +990,7 @@  av_cold int ff_mpv_common_init(MpegEncContext *s)
     return 0;
  fail:
     ff_mpv_common_end(s);
-    return -1;
+    return ret;
 }
 
 /**