diff mbox series

[FFmpeg-devel,1/3] swscale/input: Fix several invalid shifts related to rgb2yuv constants

Message ID 20200121221454.17655-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/3] swscale/input: Fix several invalid shifts related to rgb2yuv constants
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer Jan. 21, 2020, 10:14 p.m. UTC
Fixes: Invalid shifts
Fixes: #8140
Fixes: #8146

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libswscale/input.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Andreas Rheinhardt Jan. 22, 2020, 2:42 p.m. UTC | #1
On Tue, Jan 21, 2020 at 11:18 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> Fixes: Invalid shifts
> Fixes: #8140
> Fixes: #8146
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libswscale/input.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/libswscale/input.c b/libswscale/input.c
> index 064f8da314..159f70307d 100644
> --- a/libswscale/input.c
> +++ b/libswscale/input.c
> @@ -286,8 +286,8 @@ static av_always_inline void
> rgb16_32ToUV_c_template(int16_t *dstU,
>                                                       int gsh, int bsh,
> int S,
>                                                       int32_t *rgb2yuv)
>  {
> -    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX] <<
> gsh, bu = rgb2yuv[BU_IDX] << bsh,
> -              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX] <<
> gsh, bv = rgb2yuv[BV_IDX] << bsh;
> +    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu =
> rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
> +              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv =
> rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh);
>      const unsigned rnd = (256u<<((S)-1)) + (1<<(S-7));
>      int i;
>
> @@ -314,8 +314,8 @@ static av_always_inline void
> rgb16_32ToUV_half_c_template(int16_t *dstU,
>                                                            int gsh, int
> bsh, int S,
>                                                            int32_t
> *rgb2yuv)
>  {
> -    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX] <<
> gsh, bu = rgb2yuv[BU_IDX] << bsh,
> -              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX] <<
> gsh, bv = rgb2yuv[BV_IDX] << bsh,
> +    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu =
> rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
> +              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv =
> rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh),
>                maskgx   = ~(maskr | maskb);
>      const unsigned rnd = (256U<<(S)) + (1<<(S-6));
>      int i;
> --

When I looked into this last year, I was unable to ascertain that the
multiplication wouldn't overflow (in which case one would need to go the
unsigned route, most easily by making the rgb2yuv parameter a uint32_t *).
Can you?

This also fixes lots of FATE-tests, 37 if I am not mistaken:
filter-overlay_yuv420_yuva420, filter-overlay_yuv422_yuva422,
ffmpeg-filter_colorkey, sub2video, vsynth1-ffv1-v3-bgr0,
vsynth3-huffyuvbgr24, vsynth_lena-mov-bpp15, ...

- Andreas
Michael Niedermayer Jan. 22, 2020, 5:44 p.m. UTC | #2
On Wed, Jan 22, 2020 at 03:42:25PM +0100, Andreas Rheinhardt wrote:
> On Tue, Jan 21, 2020 at 11:18 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > Fixes: Invalid shifts
> > Fixes: #8140
> > Fixes: #8146
> >
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libswscale/input.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/libswscale/input.c b/libswscale/input.c
> > index 064f8da314..159f70307d 100644
> > --- a/libswscale/input.c
> > +++ b/libswscale/input.c
> > @@ -286,8 +286,8 @@ static av_always_inline void
> > rgb16_32ToUV_c_template(int16_t *dstU,
> >                                                       int gsh, int bsh,
> > int S,
> >                                                       int32_t *rgb2yuv)
> >  {
> > -    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX] <<
> > gsh, bu = rgb2yuv[BU_IDX] << bsh,
> > -              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX] <<
> > gsh, bv = rgb2yuv[BV_IDX] << bsh;
> > +    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu =
> > rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
> > +              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv =
> > rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh);
> >      const unsigned rnd = (256u<<((S)-1)) + (1<<(S-7));
> >      int i;
> >
> > @@ -314,8 +314,8 @@ static av_always_inline void
> > rgb16_32ToUV_half_c_template(int16_t *dstU,
> >                                                            int gsh, int
> > bsh, int S,
> >                                                            int32_t
> > *rgb2yuv)
> >  {
> > -    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX] <<
> > gsh, bu = rgb2yuv[BU_IDX] << bsh,
> > -              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX] <<
> > gsh, bv = rgb2yuv[BV_IDX] << bsh,
> > +    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu =
> > rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
> > +              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv =
> > rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh),
> >                maskgx   = ~(maskr | maskb);
> >      const unsigned rnd = (256U<<(S)) + (1<<(S-6));
> >      int i;
> > --
> 
> When I looked into this last year, I was unable to ascertain that the
> multiplication wouldn't overflow (in which case one would need to go the
> unsigned route, most easily by making the rgb2yuv parameter a uint32_t *).
> Can you?

If there is a situation where these overflow then this needs to be looked
at with a testcase. Simply using unsigned would likely not produce a
outcome the user wants

[...]
Andreas Rheinhardt Jan. 22, 2020, 5:53 p.m. UTC | #3
On Wed, Jan 22, 2020 at 6:44 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Jan 22, 2020 at 03:42:25PM +0100, Andreas Rheinhardt wrote:
> > On Tue, Jan 21, 2020 at 11:18 PM Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> > > Fixes: Invalid shifts
> > > Fixes: #8140
> > > Fixes: #8146
> > >
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libswscale/input.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/libswscale/input.c b/libswscale/input.c
> > > index 064f8da314..159f70307d 100644
> > > --- a/libswscale/input.c
> > > +++ b/libswscale/input.c
> > > @@ -286,8 +286,8 @@ static av_always_inline void
> > > rgb16_32ToUV_c_template(int16_t *dstU,
> > >                                                       int gsh, int bsh,
> > > int S,
> > >                                                       int32_t *rgb2yuv)
> > >  {
> > > -    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX]
> <<
> > > gsh, bu = rgb2yuv[BU_IDX] << bsh,
> > > -              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX]
> <<
> > > gsh, bv = rgb2yuv[BV_IDX] << bsh;
> > > +    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu =
> > > rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
> > > +              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv =
> > > rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh);
> > >      const unsigned rnd = (256u<<((S)-1)) + (1<<(S-7));
> > >      int i;
> > >
> > > @@ -314,8 +314,8 @@ static av_always_inline void
> > > rgb16_32ToUV_half_c_template(int16_t *dstU,
> > >                                                            int gsh, int
> > > bsh, int S,
> > >                                                            int32_t
> > > *rgb2yuv)
> > >  {
> > > -    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX]
> <<
> > > gsh, bu = rgb2yuv[BU_IDX] << bsh,
> > > -              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX]
> <<
> > > gsh, bv = rgb2yuv[BV_IDX] << bsh,
> > > +    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu =
> > > rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
> > > +              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv =
> > > rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh),
> > >                maskgx   = ~(maskr | maskb);
> > >      const unsigned rnd = (256U<<(S)) + (1<<(S-6));
> > >      int i;
> > > --
> >
> > When I looked into this last year, I was unable to ascertain that the
> > multiplication wouldn't overflow (in which case one would need to go the
> > unsigned route, most easily by making the rgb2yuv parameter a uint32_t
> *).
> > Can you?
>
> If there is a situation where these overflow then this needs to be looked
> at with a testcase. Simply using unsigned would likely not produce a
> outcome the user wants
>
> The outcome would not change at all (presuming the ordinary implementation
specified conversion unsigned->int): rgb2yuv is only used to initialize
these const ints that you see in the diff and using unsigned would just
mean that the conversion needs to wrap around.

But I agree that there is no reason to use unsigned for the calculation if
there is no testcase showing an overflow. So proceed as you wish.

- Andreas
Michael Niedermayer Jan. 22, 2020, 6:25 p.m. UTC | #4
On Wed, Jan 22, 2020 at 06:53:13PM +0100, Andreas Rheinhardt wrote:
> On Wed, Jan 22, 2020 at 6:44 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Jan 22, 2020 at 03:42:25PM +0100, Andreas Rheinhardt wrote:
> > > On Tue, Jan 21, 2020 at 11:18 PM Michael Niedermayer
> > <michael@niedermayer.cc>
> > > wrote:
> > >
> > > > Fixes: Invalid shifts
> > > > Fixes: #8140
> > > > Fixes: #8146
> > > >
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libswscale/input.c | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/libswscale/input.c b/libswscale/input.c
> > > > index 064f8da314..159f70307d 100644
> > > > --- a/libswscale/input.c
> > > > +++ b/libswscale/input.c
> > > > @@ -286,8 +286,8 @@ static av_always_inline void
> > > > rgb16_32ToUV_c_template(int16_t *dstU,
> > > >                                                       int gsh, int bsh,
> > > > int S,
> > > >                                                       int32_t *rgb2yuv)
> > > >  {
> > > > -    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX]
> > <<
> > > > gsh, bu = rgb2yuv[BU_IDX] << bsh,
> > > > -              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX]
> > <<
> > > > gsh, bv = rgb2yuv[BV_IDX] << bsh;
> > > > +    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu =
> > > > rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
> > > > +              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv =
> > > > rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh);
> > > >      const unsigned rnd = (256u<<((S)-1)) + (1<<(S-7));
> > > >      int i;
> > > >
> > > > @@ -314,8 +314,8 @@ static av_always_inline void
> > > > rgb16_32ToUV_half_c_template(int16_t *dstU,
> > > >                                                            int gsh, int
> > > > bsh, int S,
> > > >                                                            int32_t
> > > > *rgb2yuv)
> > > >  {
> > > > -    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX]
> > <<
> > > > gsh, bu = rgb2yuv[BU_IDX] << bsh,
> > > > -              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX]
> > <<
> > > > gsh, bv = rgb2yuv[BV_IDX] << bsh,
> > > > +    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu =
> > > > rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
> > > > +              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv =
> > > > rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh),
> > > >                maskgx   = ~(maskr | maskb);
> > > >      const unsigned rnd = (256U<<(S)) + (1<<(S-6));
> > > >      int i;
> > > > --
> > >
> > > When I looked into this last year, I was unable to ascertain that the
> > > multiplication wouldn't overflow (in which case one would need to go the
> > > unsigned route, most easily by making the rgb2yuv parameter a uint32_t
> > *).
> > > Can you?
> >
> > If there is a situation where these overflow then this needs to be looked
> > at with a testcase. Simply using unsigned would likely not produce a
> > outcome the user wants
> >
> > The outcome would not change at all (presuming the ordinary implementation
> specified conversion unsigned->int): rgb2yuv is only used to initialize
> these const ints that you see in the diff and using unsigned would just
> mean that the conversion needs to wrap around.
> 

> But I agree that there is no reason to use unsigned for the calculation if
> there is no testcase showing an overflow. So proceed as you wish.

ok

thx

[...]
diff mbox series

Patch

diff --git a/libswscale/input.c b/libswscale/input.c
index 064f8da314..159f70307d 100644
--- a/libswscale/input.c
+++ b/libswscale/input.c
@@ -286,8 +286,8 @@  static av_always_inline void rgb16_32ToUV_c_template(int16_t *dstU,
                                                      int gsh, int bsh, int S,
                                                      int32_t *rgb2yuv)
 {
-    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX] << gsh, bu = rgb2yuv[BU_IDX] << bsh,
-              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX] << gsh, bv = rgb2yuv[BV_IDX] << bsh;
+    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu = rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
+              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv = rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh);
     const unsigned rnd = (256u<<((S)-1)) + (1<<(S-7));
     int i;
 
@@ -314,8 +314,8 @@  static av_always_inline void rgb16_32ToUV_half_c_template(int16_t *dstU,
                                                           int gsh, int bsh, int S,
                                                           int32_t *rgb2yuv)
 {
-    const int ru       = rgb2yuv[RU_IDX] << rsh, gu = rgb2yuv[GU_IDX] << gsh, bu = rgb2yuv[BU_IDX] << bsh,
-              rv       = rgb2yuv[RV_IDX] << rsh, gv = rgb2yuv[GV_IDX] << gsh, bv = rgb2yuv[BV_IDX] << bsh,
+    const int ru       = rgb2yuv[RU_IDX] * (1 << rsh), gu = rgb2yuv[GU_IDX] * (1 << gsh), bu = rgb2yuv[BU_IDX] * (1 << bsh),
+              rv       = rgb2yuv[RV_IDX] * (1 << rsh), gv = rgb2yuv[GV_IDX] * (1 << gsh), bv = rgb2yuv[BV_IDX] * (1 << bsh),
               maskgx   = ~(maskr | maskb);
     const unsigned rnd = (256U<<(S)) + (1<<(S-6));
     int i;