| Message ID | 20200223122256.23402-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] swscale/x86/yuv2rgb: Fix build without SSSE3 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
lgtm On 2/23/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > From: Parker Ernest <@> > > commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on > x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libswscale/x86/yuv2rgb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libswscale/x86/yuv2rgb.c b/libswscale/x86/yuv2rgb.c > index c12e88cbb5..4791e5b93a 100644 > --- a/libswscale/x86/yuv2rgb.c > +++ b/libswscale/x86/yuv2rgb.c > @@ -83,6 +83,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) > #if HAVE_X86ASM > int cpu_flags = av_get_cpu_flags(); > > +#if HAVE_SSSE3 > if (EXTERNAL_SSSE3(cpu_flags)) { > switch (c->dstFormat) { > case AV_PIX_FMT_RGB32: > @@ -111,6 +112,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) > return yuv420_rgb15_ssse3; > } > } > +#endif > > if (EXTERNAL_MMXEXT(cpu_flags)) { > switch (c->dstFormat) { > -- > 2.17.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 2020-02-23 13:22, Michael Niedermayer wrote: > From: Parker Ernest <@> > > commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on > x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libswscale/x86/yuv2rgb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libswscale/x86/yuv2rgb.c b/libswscale/x86/yuv2rgb.c > index c12e88cbb5..4791e5b93a 100644 > --- a/libswscale/x86/yuv2rgb.c > +++ b/libswscale/x86/yuv2rgb.c > @@ -83,6 +83,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) > #if HAVE_X86ASM > int cpu_flags = av_get_cpu_flags(); > > +#if HAVE_SSSE3 > if (EXTERNAL_SSSE3(cpu_flags)) { > switch (c->dstFormat) { > case AV_PIX_FMT_RGB32: > @@ -111,6 +112,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) > return yuv420_rgb15_ssse3; > } > } > +#endif > > if (EXTERNAL_MMXEXT(cpu_flags)) { > switch (c->dstFormat) { > What? Why doesn't the the EXTERNAL_SSSE3 macro stop the code from entering that branch? The #if would only stop the section from being compiled with --disable-ssse3. A normal build would still enter that branch on that CPU.
On 2/23/2020 9:33 AM, Paul B Mahol wrote: > lgtm No, it's not ok. The EXTERNAL_SSSE3() macro should be enough to prevent any of these functions from running on old CPUs. It would help actually knowing what kind of failure is the user getting. > > On 2/23/20, Michael Niedermayer <michael@niedermayer.cc> wrote: >> From: Parker Ernest <@> >> >> commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on >> x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libswscale/x86/yuv2rgb.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/libswscale/x86/yuv2rgb.c b/libswscale/x86/yuv2rgb.c >> index c12e88cbb5..4791e5b93a 100644 >> --- a/libswscale/x86/yuv2rgb.c >> +++ b/libswscale/x86/yuv2rgb.c >> @@ -83,6 +83,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) >> #if HAVE_X86ASM >> int cpu_flags = av_get_cpu_flags(); >> >> +#if HAVE_SSSE3 >> if (EXTERNAL_SSSE3(cpu_flags)) { >> switch (c->dstFormat) { >> case AV_PIX_FMT_RGB32: >> @@ -111,6 +112,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) >> return yuv420_rgb15_ssse3; >> } >> } >> +#endif >> >> if (EXTERNAL_MMXEXT(cpu_flags)) { >> switch (c->dstFormat) { >> -- >> 2.17.1 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Am So., 23. Feb. 2020 um 13:30 Uhr schrieb Michael Niedermayer <michael@niedermayer.cc>: > > From: Parker Ernest <@> > > commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on > x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II Does the commit break build on specific CPUs or specific toolchains? Carl Eugen
On 2/23/20, James Almer <jamrial@gmail.com> wrote: > On 2/23/2020 9:33 AM, Paul B Mahol wrote: >> lgtm > > No, it's not ok. The EXTERNAL_SSSE3() macro should be enough to prevent > any of these functions from running on old CPUs. this is about building. > > It would help actually knowing what kind of failure is the user getting. > >> >> On 2/23/20, Michael Niedermayer <michael@niedermayer.cc> wrote: >>> From: Parker Ernest <@> >>> >>> commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on >>> x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II >>> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libswscale/x86/yuv2rgb.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/libswscale/x86/yuv2rgb.c b/libswscale/x86/yuv2rgb.c >>> index c12e88cbb5..4791e5b93a 100644 >>> --- a/libswscale/x86/yuv2rgb.c >>> +++ b/libswscale/x86/yuv2rgb.c >>> @@ -83,6 +83,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) >>> #if HAVE_X86ASM >>> int cpu_flags = av_get_cpu_flags(); >>> >>> +#if HAVE_SSSE3 >>> if (EXTERNAL_SSSE3(cpu_flags)) { >>> switch (c->dstFormat) { >>> case AV_PIX_FMT_RGB32: >>> @@ -111,6 +112,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) >>> return yuv420_rgb15_ssse3; >>> } >>> } >>> +#endif >>> >>> if (EXTERNAL_MMXEXT(cpu_flags)) { >>> switch (c->dstFormat) { >>> -- >>> 2.17.1 >>> >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> >>> To unsubscribe, visit link above, or email >>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sun, Feb 23, 2020 at 6:26 PM Paul B Mahol <onemda@gmail.com> wrote: > > On 2/23/20, James Almer <jamrial@gmail.com> wrote: > > On 2/23/2020 9:33 AM, Paul B Mahol wrote: > >> lgtm > > > > No, it's not ok. The EXTERNAL_SSSE3() macro should be enough to prevent > > any of these functions from running on old CPUs. > > this is about building. > We don't protect other parts of the SIMD code with preprocessor checks, we only typically do it for AVX2, but not any of the SSE variants, so at the very least some clarity would be appreciated why and when this is needed. Because the commit message does not. - Hendrik
On Sun, Feb 23, 2020 at 05:03:36PM +0100, Carl Eugen Hoyos wrote: > Am So., 23. Feb. 2020 um 13:30 Uhr schrieb Michael Niedermayer > <michael@niedermayer.cc>: > > > > From: Parker Ernest <@> > > > > commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on > > x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II > > Does the commit break build on specific CPUs or specific toolchains? I dont know what the testcase was the author encountered, i just posted this here as the author wanted me to post it for him. but a simple make distclean ; ./configure --disable-ssse3 && make -j32 replicates the build failure here (see below for the errors) We do have the extern void RENAME(ff_yuv_420_rgb32)(x86_reg index, uint8_t *image, const uint8_t *pu_index, const uint8_t *pv_index, const uint64_t *pointer_c_dither, const uint8_t *py_2index); ... under #if HAVE_SSSE3 so i think either that needs to be changed or the code using it needs to be similarly protected. the if(...) is not good enough because it is not something failingh at linking stage but already before because the compiler has no idea what the identifer even is. (compared to knowing what it is but not having a implemenetation later, which works as disabled code and optimized out references dont fail to link) libswscale/x86/yuv2rgb.c: In function ‘ff_yuv2rgb_init_x86’: libswscale/x86/yuv2rgb.c:91:24: error: ‘yuva420_rgb32_ssse3’ undeclared (first use in this function); did you mean ‘yuva420_rgb32_mmx’? return yuva420_rgb32_ssse3; ^~~~~~~~~~~~~~~~~~~ yuva420_rgb32_mmx libswscale/x86/yuv2rgb.c:91:24: note: each undeclared identifier is reported only once for each function it appears in libswscale/x86/yuv2rgb.c:95:24: error: ‘yuv420_rgb32_ssse3’ undeclared (first use in this function); did you mean ‘yuva420_rgb32_ssse3’? return yuv420_rgb32_ssse3; ^~~~~~~~~~~~~~~~~~ yuva420_rgb32_ssse3 libswscale/x86/yuv2rgb.c:99:24: error: ‘yuva420_bgr32_ssse3’ undeclared (first use in this function); did you mean ‘yuva420_rgb32_ssse3’? return yuva420_bgr32_ssse3; ^~~~~~~~~~~~~~~~~~~ yuva420_rgb32_ssse3 libswscale/x86/yuv2rgb.c:103:24: error: ‘yuv420_bgr32_ssse3’ undeclared (first use in this function); did you mean ‘yuva420_bgr32_ssse3’? return yuv420_bgr32_ssse3; ^~~~~~~~~~~~~~~~~~ yuva420_bgr32_ssse3 libswscale/x86/yuv2rgb.c:105:20: error: ‘yuv420_rgb24_ssse3’ undeclared (first use in this function); did you mean ‘yuv420_rgb32_ssse3’? return yuv420_rgb24_ssse3; ^~~~~~~~~~~~~~~~~~ yuv420_rgb32_ssse3 libswscale/x86/yuv2rgb.c:107:20: error: ‘yuv420_bgr24_ssse3’ undeclared (first use in this function); did you mean ‘yuv420_rgb24_ssse3’? return yuv420_bgr24_ssse3; ^~~~~~~~~~~~~~~~~~ yuv420_rgb24_ssse3 libswscale/x86/yuv2rgb.c:109:20: error: ‘yuv420_rgb16_ssse3’ undeclared (first use in this function); did you mean ‘yuv420_rgb24_ssse3’? return yuv420_rgb16_ssse3; ^~~~~~~~~~~~~~~~~~ yuv420_rgb24_ssse3 libswscale/x86/yuv2rgb.c:111:20: error: ‘yuv420_rgb15_ssse3’ undeclared (first use in this function); did you mean ‘yuv420_rgb16_ssse3’? return yuv420_rgb15_ssse3; ^~~~~~~~~~~~~~~~~~ yuv420_rgb16_ssse3 ffbuild/common.mak:59: recipe for target 'libswscale/x86/yuv2rgb.o' failed make: *** [libswscale/x86/yuv2rgb.o] Error 1 make: *** Waiting for unfinished jobs.... [...]
On 2/23/2020 2:58 PM, Michael Niedermayer wrote: > On Sun, Feb 23, 2020 at 05:03:36PM +0100, Carl Eugen Hoyos wrote: >> Am So., 23. Feb. 2020 um 13:30 Uhr schrieb Michael Niedermayer >> <michael@niedermayer.cc>: >>> >>> From: Parker Ernest <@> >>> >>> commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on >>> x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II >> >> Does the commit break build on specific CPUs or specific toolchains? > > I dont know what the testcase was the author encountered, i just posted > this here as the author wanted me to post it for him. > but a simple > make distclean ; ./configure --disable-ssse3 && make -j32 > replicates the build failure here (see below for the errors) > > We do have the > extern void RENAME(ff_yuv_420_rgb32)(x86_reg index, uint8_t *image, const uint8_t *pu_index, > const uint8_t *pv_index, const uint64_t *pointer_c_dither, > const uint8_t *py_2index); > ... > > under #if HAVE_SSSE3 > > so i think either that needs to be changed or the code using it needs to be > similarly protected. > the if(...) > is not good enough because it is not something failingh at linking stage but > already before because the compiler has no idea what the identifer even is. > (compared to knowing what it is but not having a implemenetation later, which > works as disabled code and optimized out references dont fail to link) This would also happen with --disable-mmxext. IMO, the #if HAVE_* checks should be removed and always include all three instances of yuv2rgb_template.c. The actual asm code is always assembled after all, and as Hendrik mentioned, we only ever make AVX functions or higher optional, mainly because of assembler support and because disabling such instruction sets also affects alignment and stride constants across the codebase. > > > > libswscale/x86/yuv2rgb.c: In function ‘ff_yuv2rgb_init_x86’: > libswscale/x86/yuv2rgb.c:91:24: error: ‘yuva420_rgb32_ssse3’ undeclared (first use in this function); did you mean ‘yuva420_rgb32_mmx’? > return yuva420_rgb32_ssse3; > ^~~~~~~~~~~~~~~~~~~ > yuva420_rgb32_mmx > libswscale/x86/yuv2rgb.c:91:24: note: each undeclared identifier is reported only once for each function it appears in > libswscale/x86/yuv2rgb.c:95:24: error: ‘yuv420_rgb32_ssse3’ undeclared (first use in this function); did you mean ‘yuva420_rgb32_ssse3’? > return yuv420_rgb32_ssse3; > ^~~~~~~~~~~~~~~~~~ > yuva420_rgb32_ssse3 > libswscale/x86/yuv2rgb.c:99:24: error: ‘yuva420_bgr32_ssse3’ undeclared (first use in this function); did you mean ‘yuva420_rgb32_ssse3’? > return yuva420_bgr32_ssse3; > ^~~~~~~~~~~~~~~~~~~ > yuva420_rgb32_ssse3 > libswscale/x86/yuv2rgb.c:103:24: error: ‘yuv420_bgr32_ssse3’ undeclared (first use in this function); did you mean ‘yuva420_bgr32_ssse3’? > return yuv420_bgr32_ssse3; > ^~~~~~~~~~~~~~~~~~ > yuva420_bgr32_ssse3 > libswscale/x86/yuv2rgb.c:105:20: error: ‘yuv420_rgb24_ssse3’ undeclared (first use in this function); did you mean ‘yuv420_rgb32_ssse3’? > return yuv420_rgb24_ssse3; > ^~~~~~~~~~~~~~~~~~ > yuv420_rgb32_ssse3 > libswscale/x86/yuv2rgb.c:107:20: error: ‘yuv420_bgr24_ssse3’ undeclared (first use in this function); did you mean ‘yuv420_rgb24_ssse3’? > return yuv420_bgr24_ssse3; > ^~~~~~~~~~~~~~~~~~ > yuv420_rgb24_ssse3 > libswscale/x86/yuv2rgb.c:109:20: error: ‘yuv420_rgb16_ssse3’ undeclared (first use in this function); did you mean ‘yuv420_rgb24_ssse3’? > return yuv420_rgb16_ssse3; > ^~~~~~~~~~~~~~~~~~ > yuv420_rgb24_ssse3 > libswscale/x86/yuv2rgb.c:111:20: error: ‘yuv420_rgb15_ssse3’ undeclared (first use in this function); did you mean ‘yuv420_rgb16_ssse3’? > return yuv420_rgb15_ssse3; > ^~~~~~~~~~~~~~~~~~ > yuv420_rgb16_ssse3 > ffbuild/common.mak:59: recipe for target 'libswscale/x86/yuv2rgb.o' failed > make: *** [libswscale/x86/yuv2rgb.o] Error 1 > make: *** Waiting for unfinished jobs.... > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On Sun, Feb 23, 2020 at 6:58 PM Michael Niedermayer <michaelni@gmx.at> wrote: > > On Sun, Feb 23, 2020 at 05:03:36PM +0100, Carl Eugen Hoyos wrote: > > Am So., 23. Feb. 2020 um 13:30 Uhr schrieb Michael Niedermayer > > <michael@niedermayer.cc>: > > > > > > From: Parker Ernest <@> > > > > > > commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on > > > x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II > > > > Does the commit break build on specific CPUs or specific toolchains? > > I dont know what the testcase was the author encountered, i just posted > this here as the author wanted me to post it for him. > but a simple > make distclean ; ./configure --disable-ssse3 && make -j32 > replicates the build failure here (see below for the errors) > > We do have the > extern void RENAME(ff_yuv_420_rgb32)(x86_reg index, uint8_t *image, const uint8_t *pu_index, > const uint8_t *pv_index, const uint64_t *pointer_c_dither, > const uint8_t *py_2index); > ... > > under #if HAVE_SSSE3 > > so i think either that needs to be changed or the code using it needs to be > similarly protected. The extern declaration should be removed from that guard then, like other SIMD code in the codebase. - Hendrik
On 2020-02-23 18:58, Michael Niedermayer wrote: > On Sun, Feb 23, 2020 at 05:03:36PM +0100, Carl Eugen Hoyos wrote: >> Am So., 23. Feb. 2020 um 13:30 Uhr schrieb Michael Niedermayer >> <michael@niedermayer.cc>: >>> >>> From: Parker Ernest <@> >>> >>> commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on >>> x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II >> >> Does the commit break build on specific CPUs or specific toolchains? > > I dont know what the testcase was the author encountered, i just posted > this here as the author wanted me to post it for him. > but a simple > make distclean ; ./configure --disable-ssse3 && make -j32 > replicates the build failure here (see below for the errors) Okay, it breaks the build when you do --disable-sse3. I see that too. It is okay to fix that any way you want. This patch is fine by me but please don't imply that it fixes a run time error in the commit message, which is what I first thought. I see a discussion has sprung up on the best way to fix it so I guess that has to be resolved first.
Am So., 23. Feb. 2020 um 23:43 Uhr schrieb James Darnley <james.darnley@gmail.com>: > > On 2020-02-23 18:58, Michael Niedermayer wrote: > > On Sun, Feb 23, 2020 at 05:03:36PM +0100, Carl Eugen Hoyos wrote: > >> Am So., 23. Feb. 2020 um 13:30 Uhr schrieb Michael Niedermayer > >> <michael@niedermayer.cc>: > >>> > >>> From: Parker Ernest <@> > >>> > >>> commit fc6a5883d6af8cae0e96af84dda0ad74b360a084 breaks build on > >>> x86_64 CPUs which do not have SSSE3, e.g. AMD Phenom-II > >> > >> Does the commit break build on specific CPUs or specific toolchains? > > > > I dont know what the testcase was the author encountered, i just posted > > this here as the author wanted me to post it for him. > > but a simple > > make distclean ; ./configure --disable-ssse3 && make -j32 > > replicates the build failure here (see below for the errors) > > Okay, it breaks the build when you do --disable-sse3. I see that too. > > It is okay to fix that any way you want. This patch is fine by me but > please don't imply that it fixes a run time error in the commit message, > which is what I first thought. I also feel that mentioning a specific cpu instead of --disable-ssse3 is misleading. Carl Eugen
diff --git a/libswscale/x86/yuv2rgb.c b/libswscale/x86/yuv2rgb.c index c12e88cbb5..4791e5b93a 100644 --- a/libswscale/x86/yuv2rgb.c +++ b/libswscale/x86/yuv2rgb.c @@ -83,6 +83,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) #if HAVE_X86ASM int cpu_flags = av_get_cpu_flags(); +#if HAVE_SSSE3 if (EXTERNAL_SSSE3(cpu_flags)) { switch (c->dstFormat) { case AV_PIX_FMT_RGB32: @@ -111,6 +112,7 @@ av_cold SwsFunc ff_yuv2rgb_init_x86(SwsContext *c) return yuv420_rgb15_ssse3; } } +#endif if (EXTERNAL_MMXEXT(cpu_flags)) { switch (c->dstFormat) {