Primitives used for premultiplying are implemented wrong for big endian architectures

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: awilfox, Assigned: lsalzman)

Tracking

64 Branch
mozilla65
Desktop
Linux
Points:
---

Firefox Tracking Flags

(firefox64 wontfix, firefox65 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

8 months ago
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.
Reporter

Comment 1

8 months ago
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)

Comment 2

8 months ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f7573deea1
fix big-endian RGBBitShift in Swizzle. r=me
Assignee

Comment 3

8 months ago
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)
Reporter

Comment 4

8 months ago
Great, thanks so much for landing this.

Comment 5

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0f7573deea1
Status: UNCONFIRMED → RESOLVED
Closed: 8 months 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.