| Message ID | 20200121221454.17655-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/3] swscale/input: Fix several invalid shifts related to rgb2yuv constants | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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
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 [...]
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
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 --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;
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(-)