Closed Bug 1500709 Opened 2 years ago Closed 2 years ago
Primitives used for premultiplying are implemented wrong for big endian architectures
gfx/2d/Swizzle.cpp has code that is meant to handle swizzling on big endian architectures. However, it is implemented incorrectly; the values and constants used for BE are wildly incorrect. This results in compile-time warnings such as: 0:31.71 In file included from /var/clean/mozppc/mozilla-unified/obj-powerpc64-foxkit-linux-musl/gfx/2d/Unified_cpp_gfx_2d2.cpp:47:0: 0:31.71 /var/clean/mozppc/mozilla-unified/gfx/2d/Swizzle.cpp: In instantiation of ‘void mozilla::gfx::PremultiplyFallback(const uint8_t*, int32_t, uint8_t*, int32_t, mozilla::gfx::IntSize) [with bool aSwapRB = false; bool aOpaqueAlpha = false; unsigned int aSrcRGBShift = 24u; unsigned int aSrcAShift = 16u; unsigned int aDstRGBShift = 24u; unsigned int aDstAShift = 16u; uint8_t = unsigned char; int32_t = int; mozilla::gfx::IntSize = mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>]’: 0:31.71 /var/clean/mozppc/mozilla-unified/gfx/2d/Swizzle.cpp:334:3: required from here 0:31.71 /var/clean/mozppc/mozilla-unified/gfx/2d/Swizzle.cpp:221:36: warning: result of ‘(65280 << 24u)’ requires 41 bits to represent, but ‘int’ only has 32 bits [-Wshift-overflow=] 0:31.71 uint32_t g = color & (0xFF00 << aSrcRGBShift); 0:31.71 ~~~~~~~~^~~~~~~~~~~~~~~~ 0:31.71 /var/clean/mozppc/mozilla-unified/gfx/2d/Swizzle.cpp:222:25: warning: result of ‘(65280 << 24u)’ requires 41 bits to represent, but ‘int’ only has 32 bits [-Wshift-overflow=] 0:31.72 g = g*a + (0xFF00 << aSrcRGBShift); 0:31.72 ~~~~~~~~^~~~~~~~~~~~~~~~ 0:31.72 /var/clean/mozppc/mozilla-unified/gfx/2d/Swizzle.cpp:223:38: warning: result of ‘(16711680 << 24u)’ requires 49 bits to represent, but ‘int’ only has 32 bits [-Wshift-overflow=] 0:31.72 g = (g + (g >> 8)) & (0xFF0000 << aSrcRGBShift); 0:31.72 ~~~~~~~~~~^~~~~~~~~~~~~~~~ 0:31.72 /var/clean/mozppc/mozilla-unified/gfx/2d/Swizzle.cpp:230:14: warning: right shift count >= width of type [-Wshift-count-overflow] 0:31.72 (rb >> (8 - aDstRGBShift)) | 0:31.72 ~~~~^~~~~~~~~~~~~~~~~~~~~~ [ and so on, for each instantiation of the template ] It also results in run-time failure because the colours become more corrupt with every composition, eventually resulting in blocks of wrong colours (see attached PNG) and then finally a crash from the assertion in VerifyRGBXCorners.
This patch corrects the error and allows BasicCompositor to function flawlessly on big endian CPUs.
Attachment #9018838 - Flags: review?(jmuizelaar)
Attachment #9018838 - Flags: review?(jmuizelaar) → review?(lsalzman)
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f7573deea1 fix big-endian RGBBitShift in Swizzle. r=me
Comment on attachment 9018838 [details] [diff] [review] Implement bit-shift constants for swizzle methods correctly on big endian CPUs. Thank you for catching this. This patch is semantically okay, but there is a simpler way to make this work. I just went ahead and pushed in my own patch so we can get the fix in quickly.
Attachment #9018838 - Flags: review?(lsalzman)
Great, thanks so much for landing this.
You need to log in before you can comment on or make changes to this bug.