Closed Bug 1500709 Opened 2 years ago Closed 2 years ago

Primitives used for premultiplying are implemented wrong for big endian architectures

Categories

(Core :: Graphics: Layers, defect, P3)

64 Branch
Desktop
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: awilfox, Assigned: lsalzman)

Details

Attachments

(2 files)

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 lsalzman@mozilla.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.
https://hg.mozilla.org/mozilla-central/rev/f0f7573deea1
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee: nobody → lsalzman
You need to log in before you can comment on or make changes to this bug.