| Message ID | GV1P250MB073772C3FE33BF652121EB0A8F549@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] swscale/swscale_unscaled: Fix undefined NULL + 0 | expand |
On Wed, Sep 28, 2022 at 08:39:09PM +0200, Andreas Rheinhardt wrote: > Affected the fitsdec-gbrp16 FATE-test. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libswscale/swscale_unscaled.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c > index 8838cc8b53..0b97377934 100644 > --- a/libswscale/swscale_unscaled.c > +++ b/libswscale/swscale_unscaled.c > @@ -695,7 +695,7 @@ static void packed16togbra16(const uint8_t *src, int srcStride, > } > } > for (i = 0; i < 4; i++) > - dst[i] += dstStride[i] >> 1; > + dst[i] = FF_PTR_ADD(dst[i], dstStride[i] >> 1); > } > } > > @@ -729,8 +729,8 @@ static int Rgb16ToPlanarRgb16Wrapper(SwsContext *c, const uint8_t *src[], > } > > for(i=0; i<4; i++) { > - dst2013[i] += stride2013[i] * srcSliceY / 2; > - dst1023[i] += stride1023[i] * srcSliceY / 2; > + dst2013[i] = FF_PTR_ADD(dst2013[i], stride2013[i] * srcSliceY / 2); > + dst1023[i] = FF_PTR_ADD(dst1023[i], stride1023[i] * srcSliceY / 2); > } is there a reason not to check the pointer in the loop ? as in for (i = 0; i < 4 && dst[i]; i++) thx [...]
Michael Niedermayer: > On Wed, Sep 28, 2022 at 08:39:09PM +0200, Andreas Rheinhardt wrote: >> Affected the fitsdec-gbrp16 FATE-test. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libswscale/swscale_unscaled.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c >> index 8838cc8b53..0b97377934 100644 >> --- a/libswscale/swscale_unscaled.c >> +++ b/libswscale/swscale_unscaled.c >> @@ -695,7 +695,7 @@ static void packed16togbra16(const uint8_t *src, int srcStride, >> } >> } >> for (i = 0; i < 4; i++) >> - dst[i] += dstStride[i] >> 1; >> + dst[i] = FF_PTR_ADD(dst[i], dstStride[i] >> 1); >> } >> } >> >> @@ -729,8 +729,8 @@ static int Rgb16ToPlanarRgb16Wrapper(SwsContext *c, const uint8_t *src[], >> } >> >> for(i=0; i<4; i++) { >> - dst2013[i] += stride2013[i] * srcSliceY / 2; >> - dst1023[i] += stride1023[i] * srcSliceY / 2; >> + dst2013[i] = FF_PTR_ADD(dst2013[i], stride2013[i] * srcSliceY / 2); >> + dst1023[i] = FF_PTR_ADD(dst1023[i], stride1023[i] * srcSliceY / 2); >> } > > is there a reason not to check the pointer in the loop ? > as in > for (i = 0; i < 4 && dst[i]; i++) > I consider NULL + 0 to be sane and would be happy to see it being defined in a future version of the spec. So I don't like adding checks to workaround the insanities of the spec. Notice that FF_PTR_ADD() is designed to allow the compiler to optimize the check away. - Andreas
On Wed, Sep 28, 2022 at 09:43:56PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Wed, Sep 28, 2022 at 08:39:09PM +0200, Andreas Rheinhardt wrote: > >> Affected the fitsdec-gbrp16 FATE-test. > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >> --- > >> libswscale/swscale_unscaled.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c > >> index 8838cc8b53..0b97377934 100644 > >> --- a/libswscale/swscale_unscaled.c > >> +++ b/libswscale/swscale_unscaled.c > >> @@ -695,7 +695,7 @@ static void packed16togbra16(const uint8_t *src, int srcStride, > >> } > >> } > >> for (i = 0; i < 4; i++) > >> - dst[i] += dstStride[i] >> 1; > >> + dst[i] = FF_PTR_ADD(dst[i], dstStride[i] >> 1); > >> } > >> } > >> > >> @@ -729,8 +729,8 @@ static int Rgb16ToPlanarRgb16Wrapper(SwsContext *c, const uint8_t *src[], > >> } > >> > >> for(i=0; i<4; i++) { > >> - dst2013[i] += stride2013[i] * srcSliceY / 2; > >> - dst1023[i] += stride1023[i] * srcSliceY / 2; > >> + dst2013[i] = FF_PTR_ADD(dst2013[i], stride2013[i] * srcSliceY / 2); > >> + dst1023[i] = FF_PTR_ADD(dst1023[i], stride1023[i] * srcSliceY / 2); > >> } > > > > is there a reason not to check the pointer in the loop ? > > as in > > for (i = 0; i < 4 && dst[i]; i++) > > > > I consider NULL + 0 to be sane and would be happy to see it being > defined in a future version of the spec. i agree but that will not help our code for a long time > So I don't like adding checks > to workaround the insanities of the spec. Ive seen this less as a workaround and more as a "only work on the active/used pointers" a dst[i] check is more used elsewhere in sws too but i really dont have much of an oppinon libswscale/slice.c: for (i = 0; i < 4 && src[i] != NULL; ++i) { libswscale/swscale.c: for (i = 0; i < 4 && src2[i]; i++) { libswscale/swscale.c: for (i = 0; i < 4 && dst2[i]; i++) { libswscale/swscale_unscaled.c: for (plane = 0; plane < 4 && dst[plane] != NULL; plane++) { libswscale/tests/swscale.c: for (i = 0; i < 4 && dstStride[i]; i++) libswscale/swscale.c: for (int i = 0; i < FF_ARRAY_ELEMS(dst) && parent->frame_dst->data[i]; i++) { [...]
diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c index 8838cc8b53..0b97377934 100644 --- a/libswscale/swscale_unscaled.c +++ b/libswscale/swscale_unscaled.c @@ -695,7 +695,7 @@ static void packed16togbra16(const uint8_t *src, int srcStride, } } for (i = 0; i < 4; i++) - dst[i] += dstStride[i] >> 1; + dst[i] = FF_PTR_ADD(dst[i], dstStride[i] >> 1); } } @@ -729,8 +729,8 @@ static int Rgb16ToPlanarRgb16Wrapper(SwsContext *c, const uint8_t *src[], } for(i=0; i<4; i++) { - dst2013[i] += stride2013[i] * srcSliceY / 2; - dst1023[i] += stride1023[i] * srcSliceY / 2; + dst2013[i] = FF_PTR_ADD(dst2013[i], stride2013[i] * srcSliceY / 2); + dst1023[i] = FF_PTR_ADD(dst1023[i], stride1023[i] * srcSliceY / 2); } switch (c->srcFormat) {
Affected the fitsdec-gbrp16 FATE-test. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libswscale/swscale_unscaled.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)