Last Comment Bug 818004 - new Blurring code isn't big endian compatible
: new Blurring code isn't big endian compatible
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 19 Branch
: PowerPC All
: -- normal (vote)
: mozilla20
Assigned To: Tobias Netzel
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2012-12-04 04:45 PST by Tobias Netzel
Modified: 2012-12-05 07:32 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

make new Blur code big endian compatible (2.53 KB, patch)
2012-12-04 04:45 PST, Tobias Netzel
tterribe: review+
Details | Diff | Splinter Review
updated after review (3.82 KB, patch)
2012-12-04 05:41 PST, Tobias Netzel
joe: review+
Details | Diff | Splinter Review
updated after review 2 (3.90 KB, patch)
2012-12-05 03:42 PST, Tobias Netzel
no flags Details | Diff | Splinter Review
updated after review 2 v2 (3.91 KB, patch)
2012-12-05 03:49 PST, Tobias Netzel
no flags Details | Diff | Splinter Review

Description Tobias Netzel 2012-12-04 04:45:20 PST
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.
Comment 1 Timothy B. Terriberry (:derf) 2012-12-04 05:03:50 PST
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 2012-12-04 05:41:14 PST
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.
Comment 3 Joe Drew (not getting mail) 2012-12-04 07:06:21 PST
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) 2012-12-04 07:59:35 PST
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.
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-12-04 18:03:27 PST

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!
Comment 6 Tobias Netzel 2012-12-05 03:42:34 PST
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.
Comment 7 Tobias Netzel 2012-12-05 03:49:51 PST
Created attachment 688706 [details] [diff] [review]
updated after review 2 v2

Fixing compiler warning/error,
Comment 8 Ed Morley [:emorley] 2012-12-05 07:32:26 PST

Note You need to log in before you can comment on or make changes to this bug.