diff mbox

[FFmpeg-devel,2/2] avcodec/snowenc: Replace "return -1" by named constants

Message ID 20170923010154.1663-2-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Sept. 23, 2017, 1:01 a.m. UTC 
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/snowenc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

James Almer Sept. 23, 2017, 1:14 a.m. UTC | #1 
On 9/22/2017 10:01 PM, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/snowenc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
> index bb113d0a2b..f5497f958c 100644
> --- a/libavcodec/snowenc.c
> +++ b/libavcodec/snowenc.c
> @@ -50,7 +50,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>         && (avctx->flags & AV_CODEC_FLAG_QSCALE)
>         && avctx->global_quality == 0){
>          av_log(avctx, AV_LOG_ERROR, "The 9/7 wavelet is incompatible with lossless mode.\n");
> -        return -1;
> +        return AVERROR(EINVAL);
>      }
>  #if FF_API_MOTION_EST
>  FF_DISABLE_DEPRECATION_WARNINGS
> @@ -107,8 +107,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              return AVERROR(ENOMEM);
>      }
>      if((avctx->flags&AV_CODEC_FLAG_PASS2) || !(avctx->flags&AV_CODEC_FLAG_QSCALE)){
> -        if(ff_rate_control_init(&s->m) < 0)
> -            return -1;
> +        if((ret = ff_rate_control_init(&s->m)) < 0)
> +            return ret;

ret = foo();
if (ret < 0)
    return ret;

No more combined assignment and comparisons for new code if possible.
It's too prone to mistakes.

>      }
>      s->pass1_rc= !(avctx->flags & (AV_CODEC_FLAG_QSCALE|AV_CODEC_FLAG_PASS2));
>  
> @@ -130,7 +130,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>          break;*/
>      default:
>          av_log(avctx, AV_LOG_ERROR, "pixel format not supported\n");
> -        return -1;
> +        return AVERROR_PATCHWELCOME;
>      }
>      avcodec_get_chroma_sub_sample(avctx->pix_fmt, &s->chroma_h_shift, &s->chroma_v_shift);
>  
> @@ -833,7 +833,7 @@ static int encode_subband_c0run(SnowContext *s, SubBand *b, const IDWTELEM *src,
>          for(y=0; y<h; y++){
>              if(s->c.bytestream_end - s->c.bytestream < w*40){
>                  av_log(s->avctx, AV_LOG_ERROR, "encoded frame too large\n");
> -                return -1;
> +                return AVERROR(ENOMEM);
>              }
>              for(x=0; x<w; x++){
>                  int v, p=0;
> 

Should be good aside from the above.
Michael Niedermayer Sept. 23, 2017, 10:07 p.m. UTC | #2 
On Fri, Sep 22, 2017 at 10:14:01PM -0300, James Almer wrote:
> On 9/22/2017 10:01 PM, Michael Niedermayer wrote:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/snowenc.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
> > index bb113d0a2b..f5497f958c 100644
> > --- a/libavcodec/snowenc.c
> > +++ b/libavcodec/snowenc.c
> > @@ -50,7 +50,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >         && (avctx->flags & AV_CODEC_FLAG_QSCALE)
> >         && avctx->global_quality == 0){
> >          av_log(avctx, AV_LOG_ERROR, "The 9/7 wavelet is incompatible with lossless mode.\n");
> > -        return -1;
> > +        return AVERROR(EINVAL);
> >      }
> >  #if FF_API_MOTION_EST
> >  FF_DISABLE_DEPRECATION_WARNINGS
> > @@ -107,8 +107,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >              return AVERROR(ENOMEM);
> >      }
> >      if((avctx->flags&AV_CODEC_FLAG_PASS2) || !(avctx->flags&AV_CODEC_FLAG_QSCALE)){
> > -        if(ff_rate_control_init(&s->m) < 0)
> > -            return -1;
> > +        if((ret = ff_rate_control_init(&s->m)) < 0)
> > +            return ret;
> 
> ret = foo();
> if (ret < 0)
>     return ret;
> 
> No more combined assignment and comparisons for new code if possible.
> It's too prone to mistakes.

I dont make that mistake (anymore), but yes it gives a bad example.
Will change


> 
> >      }
> >      s->pass1_rc= !(avctx->flags & (AV_CODEC_FLAG_QSCALE|AV_CODEC_FLAG_PASS2));
> >  
> > @@ -130,7 +130,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >          break;*/
> >      default:
> >          av_log(avctx, AV_LOG_ERROR, "pixel format not supported\n");
> > -        return -1;
> > +        return AVERROR_PATCHWELCOME;
> >      }
> >      avcodec_get_chroma_sub_sample(avctx->pix_fmt, &s->chroma_h_shift, &s->chroma_v_shift);
> >  
> > @@ -833,7 +833,7 @@ static int encode_subband_c0run(SnowContext *s, SubBand *b, const IDWTELEM *src,
> >          for(y=0; y<h; y++){
> >              if(s->c.bytestream_end - s->c.bytestream < w*40){
> >                  av_log(s->avctx, AV_LOG_ERROR, "encoded frame too large\n");
> > -                return -1;
> > +                return AVERROR(ENOMEM);
> >              }
> >              for(x=0; x<w; x++){
> >                  int v, p=0;
> > 
> 
> Should be good aside from the above.

will apply

thanks

[...]
Carl Eugen Hoyos Sept. 24, 2017, 11:06 p.m. UTC | #3 
2017-09-23 3:14 GMT+02:00 James Almer <jamrial@gmail.com>:
> On 9/22/2017 10:01 PM, Michael Niedermayer wrote:

>> +        if((ret = ff_rate_control_init(&s->m)) < 0)
>> +            return ret;
>
> ret = foo();
> if (ret < 0)
>     return ret;
>
> No more combined assignment and comparisons for
> new code if possible. It's too prone to mistakes.

This comment looks strange given today's patches from you.

Carl Eugen
James Almer Sept. 24, 2017, 11:16 p.m. UTC | #4 
On 9/24/2017 8:06 PM, Carl Eugen Hoyos wrote:
> 2017-09-23 3:14 GMT+02:00 James Almer <jamrial@gmail.com>:
>> On 9/22/2017 10:01 PM, Michael Niedermayer wrote:
> 
>>> +        if((ret = ff_rate_control_init(&s->m)) < 0)
>>> +            return ret;
>>
>> ret = foo();
>> if (ret < 0)
>>     return ret;
>>
>> No more combined assignment and comparisons for
>> new code if possible. It's too prone to mistakes.
> 
> This comment looks strange given today's patches from you.
> 
> Carl Eugen

You mean the patch where i did a sed replace of a function name in code
that existed beforehand? I don't think it's the same as manually adding
a brand new case of assignment and comparison in one statement.

Do you want me to split those lines before i push the patch? Guess it
would kill two birds with one stone, so probably worth it.
Carl Eugen Hoyos Sept. 24, 2017, 11:19 p.m. UTC | #5 
2017-09-25 1:16 GMT+02:00 James Almer <jamrial@gmail.com>:
> On 9/24/2017 8:06 PM, Carl Eugen Hoyos wrote:
>> 2017-09-23 3:14 GMT+02:00 James Almer <jamrial@gmail.com>:
>>> On 9/22/2017 10:01 PM, Michael Niedermayer wrote:
>>
>>>> +        if((ret = ff_rate_control_init(&s->m)) < 0)
>>>> +            return ret;
>>>
>>> ret = foo();
>>> if (ret < 0)
>>>     return ret;
>>>
>>> No more combined assignment and comparisons for
>>> new code if possible. It's too prone to mistakes.
>>
>> This comment looks strange given today's patches from you.

> You mean the patch where i did a sed replace of a function
> name in code that existed beforehand?

Sorry, but I believe using sed here is worse than above
code which is why I hadn't realized you used it.

Please feel free to ignore, Carl Eugen
diff mbox

Patch

diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
index bb113d0a2b..f5497f958c 100644
--- a/libavcodec/snowenc.c
+++ b/libavcodec/snowenc.c
@@ -50,7 +50,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
        && (avctx->flags & AV_CODEC_FLAG_QSCALE)
        && avctx->global_quality == 0){
         av_log(avctx, AV_LOG_ERROR, "The 9/7 wavelet is incompatible with lossless mode.\n");
-        return -1;
+        return AVERROR(EINVAL);
     }
 #if FF_API_MOTION_EST
 FF_DISABLE_DEPRECATION_WARNINGS
@@ -107,8 +107,8 @@  FF_ENABLE_DEPRECATION_WARNINGS
             return AVERROR(ENOMEM);
     }
     if((avctx->flags&AV_CODEC_FLAG_PASS2) || !(avctx->flags&AV_CODEC_FLAG_QSCALE)){
-        if(ff_rate_control_init(&s->m) < 0)
-            return -1;
+        if((ret = ff_rate_control_init(&s->m)) < 0)
+            return ret;
     }
     s->pass1_rc= !(avctx->flags & (AV_CODEC_FLAG_QSCALE|AV_CODEC_FLAG_PASS2));
 
@@ -130,7 +130,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
         break;*/
     default:
         av_log(avctx, AV_LOG_ERROR, "pixel format not supported\n");
-        return -1;
+        return AVERROR_PATCHWELCOME;
     }
     avcodec_get_chroma_sub_sample(avctx->pix_fmt, &s->chroma_h_shift, &s->chroma_v_shift);
 
@@ -833,7 +833,7 @@  static int encode_subband_c0run(SnowContext *s, SubBand *b, const IDWTELEM *src,
         for(y=0; y<h; y++){
             if(s->c.bytestream_end - s->c.bytestream < w*40){
                 av_log(s->avctx, AV_LOG_ERROR, "encoded frame too large\n");
-                return -1;
+                return AVERROR(ENOMEM);
             }
             for(x=0; x<w; x++){
                 int v, p=0;