Closed Bug 818004 Opened 11 years ago Closed 11 years ago

new Blurring code isn't big endian compatible


(Core :: Graphics, defect)

19 Branch
Not set





(Reporter: tobias.netzel, Assigned: tobias.netzel)



(2 files, 2 obsolete files)

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+
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
Ever confirmed: true
Attachment #688225 - Flags: review? → checkin?
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));
    currentRowSum += *alphaValues++;
    *aDest++ = *aPreviousRow++ + currentRowSum;
...repeated for each of the 4 bytes.

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!
Keywords: checkin-needed
Attached patch updated after review 2 (obsolete) — Splinter Review
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.
Fixing compiler warning/error,
Attachment #688700 - Attachment is obsolete: true
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.