search

strukturag / libde265

Open h.265 video codec implementation.
Other
1.7k stars 457 forks source link

CMakeLists: fix clang SSE detection #410

Closed chouquette closed 1 year ago

chouquette commented 1 year ago

When passing -msse to clang, it only issues a warning which causes a false positive for SSE detection. This will cause build failures later on if the targeted architecture isn't X86

chouquette commented 1 year ago

This should help with #266 and #308

farindk commented 1 year ago

A cleaner approach might be to replace the cmake variable DISABLE_SSE which you probably should have set for a non-x86 architecture with an automatic detection of an x86 target.

chouquette commented 1 year ago

A cleaner approach might be to replace the cmake variable DISABLE_SSE which you probably should have set for a non-x86 architecture with an automatic detection of an x86 target.

That also works for me. To clarify, do you mean to discard the changes from this PR and update the DISABLE_SSL default value to something like "arch != x86" ? Or update that default value in addition to ensuring the check doesn't lead to a false positive?

farindk commented 1 year ago

I think we could remove the DISABLE_SSE flag completely since this is tested at runtime anyway. Even for testing-purposes it is possible to disable accelerations at runtime. Thus I don't see any need for manually setting this flag.

chouquette commented 1 year ago

I'd argue that it's quite handy to be able to disable hardware accelerations, especially on some platforms such as wasm/nasm, but I can live with converting DISABLE_SSE from an option to an internal variable

farindk commented 1 year ago

Sure, but when compiling to wasm it should be deactivated automatically. We just replace the manual flag with an automatic detection.

chouquette commented 1 year ago

Alternative implementation submitted: https://github.com/strukturag/libde265/pull/411

farindk commented 1 year ago

Merged PR #410 instead