new Blurring code isn't big endian compatible

RESOLVED FIXED in mozilla20

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Tobias Netzel, Assigned: Tobias Netzel)

Tracking

19 Branch
mozilla20
PowerPC
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 688216 [details] [diff] [review]
make new Blur code big endian compatible

This is a regression from bug 509052.
While there was introduced a compile time check for the endianness that doesn't work anymore now that prtypes.h has been removed from most places.
So it actually takes the path that has known dependencies on little endian byte order which causes some rendering artifacts.
In the attached patch I added the necessary code to make the new blurring code big endian compatible and extended the endianness check to have endianness recognized correctly when building with gcc.
Attachment #688216 - Flags: review?(tterribe)
Comment on attachment 688216 [details] [diff] [review]
make new Blur code big endian compatible

Review of attachment 688216 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but see comments.

Please use at least 8 lines of context when generating diffs in the future (put "unified = 8" in the [diff] and [qdiff] sections of your .hgrc).

::: gfx/2d/Blur.cpp
@@ +558,5 @@
>        uint32_t alphaValues = *(uint32_t*)(aSource + (x - aLeftInflation));
> +#if defined WORDS_BIGENDIAN || defined IS_BIG_ENDIAN || defined __BIG_ENDIAN__
> +      currentRowSum += (alphaValues >> 24) & 0xff;
> +      *aDest++ = *aPreviousRow++ + currentRowSum;
> +      alphaValues <<= 8;

Probably better to leave this out and just decrease the right shift amount (e.g., >> 16, >> 8, >> 0) for the following bytes. I don't think the compiler is smart enough to do that for you, and it saves 1 shift/pixel.

@@ +567,5 @@
> +      *aDest++ = *aPreviousRow++ + currentRowSum;
> +      alphaValues <<= 8;
> +      currentRowSum += (alphaValues >> 24) & 0xff;
> +      *aDest++ = *aPreviousRow++ + currentRowSum;
> +#else

Should this #else have a little-endian test, so we can ensure that the defines have been set one way or the other, and #error out otherwise? That would have caught this sooner.
Attachment #688216 - Flags: review?(tterribe) → review+
(Assignee)

Comment 2

5 years ago
Created attachment 688225 [details] [diff] [review]
updated after review

Thanks for the hint.
As endianness checks are very compiler dependent I'd prefer to leave it like it is rather than potentially breaking stuff for someone else. Big endian is a special case nowadays anyway and can be treated as a such.
Attachment #688216 - Attachment is obsolete: true
Attachment #688225 - Flags: review?
Assignee: nobody → tobias.netzel
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

5 years ago
Attachment #688225 - Flags: review? → checkin?
Keywords: checkin-needed
Attachment #688225 - Flags: checkin? → review+
Hey Tobias! Thanks for the patch - I've marked it so that one of the folks who regularly checks things in for those without commit access will get to it before too long.

I hope we haven't messed things up too much for the PowerPC community by ignoring big-endian hosts!
Comment on attachment 688225 [details] [diff] [review]
updated after review

Review of attachment 688225 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Blur.cpp
@@ +563,5 @@
> +      *aDest++ = *aPreviousRow++ + currentRowSum;
> +      currentRowSum += (alphaValues >> 8) & 0xff;
> +      *aDest++ = *aPreviousRow++ + currentRowSum;
> +      currentRowSum += alphaValues & 0xff;
> +      *aDest++ = *aPreviousRow++ + currentRowSum;

Wouldn't it be simpler to just read the alpha values a byte at a time, avoiding the need for shifting and masking?

Something like:
    const uint8_t* alphaValues = (const uint8_t*)(aSource + (x - aLeftInflation));
then:
    currentRowSum += *alphaValues++;
    *aDest++ = *aPreviousRow++ + currentRowSum;
...repeated for each of the 4 bytes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8f9641e018

Tobias, please make sure that hg is configured to generate patches with all the needed metadata per the guidelines below. It makes life easier for those checking in on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Keywords: checkin-needed
(Assignee)

Comment 6

5 years ago
Created attachment 688700 [details] [diff] [review]
updated after review 2

This time following the guidelines from "Creating a patch that can be checked in".
The same change might be done to the little endian version, decrementing the pointer instead of incrementing it.

Now I don't know whether to ask for checkin again.
(Assignee)

Comment 7

5 years ago
Created attachment 688706 [details] [diff] [review]
updated after review 2 v2

Fixing compiler warning/error,
Attachment #688700 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ef8f9641e018
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.