diff mbox series

[FFmpeg-devel,2/2] swscale/tests/swscale: use codes < 128 for indicating erros

Message ID 20200716072728.25072-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/tdsc: Fix tile checks | expand

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

Michael Niedermayer July 16, 2020, 7:27 a.m. UTC
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libswscale/tests/swscale.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Martin Storsjö July 16, 2020, 7:57 a.m. UTC | #1
On Thu, 16 Jul 2020, Michael Niedermayer wrote:

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libswscale/tests/swscale.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libswscale/tests/swscale.c b/libswscale/tests/swscale.c
> index 845ced61bb..9c0b5a4b11 100644
> --- a/libswscale/tests/swscale.c
> +++ b/libswscale/tests/swscale.c
> @@ -248,7 +248,7 @@ end:
>         if (dstStride[i])
>             av_free(dst[i]);
> 
> -    return res;
> +    return res & 127;
> }

Is there a valuable distinction between the different return values in 
this test tool so far? Because this code will e.g. fail to return a proper 
error return code if res happens to be -128.

Otherwise this could be made into e.g. "return !!res;" or something like 
that.

// Martin
Michael Niedermayer July 16, 2020, 8:27 a.m. UTC | #2
On Thu, Jul 16, 2020 at 10:57:27AM +0300, Martin Storsjö wrote:
> On Thu, 16 Jul 2020, Michael Niedermayer wrote:
> 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> > libswscale/tests/swscale.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libswscale/tests/swscale.c b/libswscale/tests/swscale.c
> > index 845ced61bb..9c0b5a4b11 100644
> > --- a/libswscale/tests/swscale.c
> > +++ b/libswscale/tests/swscale.c
> > @@ -248,7 +248,7 @@ end:
> >         if (dstStride[i])
> >             av_free(dst[i]);
> > 
> > -    return res;
> > +    return res & 127;
> > }
> 
> Is there a valuable distinction between the different return values in this
> test tool so far? Because this code will e.g. fail to return a proper error
> return code if res happens to be -128.

currently no difference


> 
> Otherwise this could be made into e.g. "return !!res;" or something like
> that.

ok will push with that

thx

[...]
Nicolas George July 16, 2020, 8:27 a.m. UTC | #3
Michael Niedermayer (12020-07-16):
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libswscale/tests/swscale.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libswscale/tests/swscale.c b/libswscale/tests/swscale.c
> index 845ced61bb..9c0b5a4b11 100644
> --- a/libswscale/tests/swscale.c
> +++ b/libswscale/tests/swscale.c
> @@ -248,7 +248,7 @@ end:
>          if (dstStride[i])
>              av_free(dst[i]);
>  
> -    return res;
> +    return res & 127;
>  }
>  
>  static void selfTest(const uint8_t * const ref[4], int refStride[4],

This looks fragile: there is no local guarantee that res is not a
multiple of 128, and changes in other parts of the code could have that
effect.

Better change the ultimate return value of main to !!res IMHO.

Regards,
diff mbox series

Patch

diff --git a/libswscale/tests/swscale.c b/libswscale/tests/swscale.c
index 845ced61bb..9c0b5a4b11 100644
--- a/libswscale/tests/swscale.c
+++ b/libswscale/tests/swscale.c
@@ -248,7 +248,7 @@  end:
         if (dstStride[i])
             av_free(dst[i]);
 
-    return res;
+    return res & 127;
 }
 
 static void selfTest(const uint8_t * const ref[4], int refStride[4],