818004 - new Blurring code isn't big endian compatible

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tobias Netzel]

Attachment: 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.

Comment 1

[Timothy B. Terriberry (:derf)]

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.

Comment 2

[Tobias Netzel]

Attachment: 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.

Comment 3

[Joe Drew (not getting mail)]

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 4

[Jonathan Kew [:jfkthame]]

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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Tobias Netzel]

Attachment: 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.

Comment 7

[Tobias Netzel]

Attachment: updated after review 2 v2

Fixing compiler warning/error,

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ef8f9641e018