Last Comment Bug 817356 - resized images have a blue tint on big endian machines
: resized images have a blue tint on big endian machines
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: 19 Branch
: PowerPC Linux
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: Steve Singer (:stevensn)
:
Mentors:
: 844436 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-01 18:47 PST by Steve Singer (:stevensn)
Modified: 2013-09-05 00:22 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix. Detect that this is a PPC and set the BENDIAN then use that when extracting the bits (5.12 KB, patch)
2012-12-01 18:52 PST, Steve Singer (:stevensn)
joe: review+
Details | Diff | Splinter Review
v2 of the patch, all one one line but this exceeds 80 characters (5.03 KB, patch)
2012-12-03 20:16 PST, Steve Singer (:stevensn)
no flags Details | Diff | Splinter Review
v3 has the line break but in a better spot (5.04 KB, patch)
2012-12-03 20:32 PST, Steve Singer (:stevensn)
no flags Details | Diff | Splinter Review
adds the changes to SkPreConfig.h as a patch listed in gfx/skia/patches (1.35 KB, patch)
2012-12-09 18:22 PST, Steve Singer (:stevensn)
gwright: review+
Details | Diff | Splinter Review

Description Steve Singer (:stevensn) 2012-12-01 18:47:58 PST
Bug 486918 added new image resizers from SKIA.  Since this was enabled resized images on my ppc32 machine show up with a blue tint.

The convolver code extracts the RGBA bits from each 32 bit pixel but doesn't take the endian order into account.
Comment 1 Steve Singer (:stevensn) 2012-12-01 18:52:10 PST
Created attachment 687479 [details] [diff] [review]
proposed fix.  Detect that this is a PPC and set the BENDIAN then use that when extracting the bits
Comment 2 Joe Drew (not getting mail) 2012-12-03 07:50:36 PST
Comment on attachment 687479 [details] [diff] [review]
proposed fix.  Detect that this is a PPC and set the BENDIAN then use that when extracting the bits

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

::: gfx/2d/convolver.cpp
@@ +206,3 @@
>        if (has_alpha)
> +        accum[3] += cur_filter * source_data_rows[filter_y][byte_offset + 
> +							    A_OFFSET_IDX];

Can you just leave these all on the same line?

@@ +231,5 @@
>        //
>        // We only need to do this when generating the final output row (here).
> +      int max_color_channel = NS_MAX(out_row[byte_offset + R_OFFSET_IDX],
> +          NS_MAX(out_row[byte_offset + G_OFFSET_IDX], out_row[byte_offset 
> +							      + B_OFFSET_IDX]));

Same line here too
Comment 3 Steve Singer (:stevensn) 2012-12-03 20:16:27 PST
Created attachment 688092 [details] [diff] [review]
v2 of the patch, all one one line but this exceeds 80 characters
Comment 4 Steve Singer (:stevensn) 2012-12-03 20:32:56 PST
Created attachment 688094 [details] [diff] [review]
v3 has the line break but in a better spot
Comment 5 Landry Breuil (:gaston) 2012-12-09 12:21:48 PST
Pushed v3 in  https://hg.mozilla.org/integration/mozilla-inbound/rev/69b924c155b0
Comment 6 Matt Woodrow (:mattwoodrow) 2012-12-09 12:28:17 PST
I would strongly recommend that the change to SkPreConfig.h gets a patch file added to gfx/skia/patches, or that this change be upstreamed to google's skia repo (or both!).

Otherwise this change is at risk of being overwritten when we next update skia.
Comment 7 Steve Singer (:stevensn) 2012-12-09 18:21:03 PST
submitted for review upstream https://codereview.appspot.com/6900063/
Comment 8 Steve Singer (:stevensn) 2012-12-09 18:22:35 PST
Created attachment 690255 [details] [diff] [review]
adds the changes to SkPreConfig.h as a patch listed in  gfx/skia/patches

This patch will add a patch file in gfx/skia/patches to alter SkPreConfig.h the next time we import upstream sources
Comment 9 Joe Drew (not getting mail) 2012-12-09 18:52:07 PST
Sorry! I was confusing SkPreConfig.h with SkUserConfig.h, and incorrectly figured it didn't need to have a separate patch.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-12-10 11:06:18 PST
https://hg.mozilla.org/mozilla-central/rev/69b924c155b0
Comment 11 Landry Breuil (:gaston) 2013-03-09 08:23:53 PST
Apparently attachment #690255 [details] [diff] [review] never went through review in m-c nor commited, and neither upstream who didnt reply on codereview. Should it be unbitrotten and r?'ed ?

Stumbled upon this while investigating why skia got broken on ppc again (see #849253)...
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-03-11 17:41:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/049b349ac5a1
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-03-12 12:55:28 PDT
https://hg.mozilla.org/mozilla-central/rev/049b349ac5a1
Comment 14 Fabio 2013-09-03 10:46:14 PDT
*** Bug 844436 has been marked as a duplicate of this bug. ***

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