resized images have a blue tint on big endian machines

RESOLVED FIXED in mozilla20

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: stevensn, Assigned: stevensn)

Tracking

19 Branch
mozilla20
PowerPC
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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
Status: UNCONFIRMED → NEW
Component: File Handling → ImageLib
Ever confirmed: true
Product: Firefox → Core
Attachment #687479 - Flags: review?(joe)
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
Attachment #687479 - Flags: review?(joe) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 688092 [details] [diff] [review]
v2 of the patch, all one one line but this exceeds 80 characters
(Assignee)

Comment 4

5 years ago
Created attachment 688094 [details] [diff] [review]
v3 has the line break but in a better spot
Attachment #687479 - Attachment is obsolete: true
Pushed v3 in  https://hg.mozilla.org/integration/mozilla-inbound/rev/69b924c155b0
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.
(Assignee)

Comment 7

4 years ago
submitted for review upstream https://codereview.appspot.com/6900063/
(Assignee)

Comment 8

4 years ago
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
Sorry! I was confusing SkPreConfig.h with SkUserConfig.h, and incorrectly figured it didn't need to have a separate patch.
https://hg.mozilla.org/mozilla-central/rev/69b924c155b0
Assignee: nobody → steve
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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)...
(Assignee)

Updated

4 years ago
Attachment #690255 - Flags: review?(joe)
Attachment #690255 - Flags: review?(joe) → review?(gwright)
Attachment #690255 - Flags: review?(gwright) → review+
(Assignee)

Updated

4 years ago
Attachment #690255 - Flags: checkin?
Attachment #690255 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/049b349ac5a1
https://hg.mozilla.org/mozilla-central/rev/049b349ac5a1

Updated

4 years ago
Duplicate of this bug: 844436

Updated

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