Potential bugs in SkPixmap.cpp

[mattl...@live.com]

Hi there, sorry for the long and bug mentioning discussion. I seem to not be able to log bugs in the bugs.skia.org site anymore :O

And the bugs here I have not confirmed, but a SkiaSharp community member updating to the latest skia found this and had to fix in my fork. I have not yet tested, but he, copilot and claude all feel that it is a real bug, so I am posting here until I get the ability to log bugs again.

# SkPixmap::getColor() returns wrong values for several color types

## Summary

Commit `116f237bb3` ("Use SkColor helpers for color generation in SkPixmap") introduced 4 bugs in `SkPixmap::getColor()` by passing arguments to `SkColorSetARGB()` in the wrong order and with incorrect scaling. These bugs are present on `main` as of `3f467a5819`.

## The bugs

`SkColorSetARGB` has signature `(U8CPU a, U8CPU r, U8CPU g, U8CPU b)` — **alpha first**.

### Bug 1 — `kSRGBA_8888_SkColorType`: argument order swapped

**[src/core/SkPixmap.cpp, line 284](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/core/SkPixmap.cpp;l=284)**

```cpp

// Current (wrong) — passes (r, g, b, a) instead of (a, r, g, b)

return SkColorSetARGB(r, g, b, a);

// Should be:

return SkColorSetARGB(a, r, g, b);

```

**Before the refactor**, this code manually packed the bits correctly:

```cpp

return (uint32_t)(r * 255.0f) << 16

| (uint32_t)(g * 255.0f) << 8

| (uint32_t)(b * 255.0f) << 0

| (uint32_t)(a * 255.0f) << 24;

```

**Effect:** The red value is stored in the alpha channel and the alpha is stored in the blue channel. For fully opaque pixels this may not be visible (a=255 maps to valid red), but semi-transparent sRGBA pixels will have completely wrong colors and alpha.

### Bug 2 — `kRGBA_1010102_SkColorType`: R and B channels swapped

**[src/core/SkPixmap.cpp, lines 305–320](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/core/SkPixmap.cpp;l=305)**

The case statement handles both `kRGBA_1010102` and `kBGRA_1010102` but reads bits assuming BGRA layout unconditionally:

```cpp

case kRGBA_1010102_SkColorType:

case kBGRA_1010102_SkColorType: {

uint32_t value = *this->addr32(x, y);

float b = ((value >> 0) & 0x3ff) * (1/1023.0f), // ← reads bits 0-9 as Blue always

...

r = ((value >> 20) & 0x3ff) * (1/1023.0f), // ← reads bits 20-29 as Red always

```

For `kRGBA_1010102`, bits 0–9 are **Red**, not Blue. The refactor dropped the `std::swap(r, b)` that the pre-refactor code used to handle both layouts.

**Fix:** Read as RGBA, then swap for BGRA:

```cpp

float r = ((value >> 0) & 0x3ff) * (1/1023.0f),

g = ((value >> 10) & 0x3ff) * (1/1023.0f),

b = ((value >> 20) & 0x3ff) * (1/1023.0f),

a = ((value >> 30) & 0x3 ) * (1/ 3.0f);

if (this->colorType() == kBGRA_1010102_SkColorType) {

std::swap(r, b);

}

```

**Effect:** Red and blue channels swapped for all `kRGBA_1010102` pixels.

### Bug 3 — `kRGBA_10x6_SkColorType`: missing ×255 scale factor

**[src/core/SkPixmap.cpp, line 328](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/core/SkPixmap.cpp;l=328)**

```cpp

// Current (wrong) — produces float 0.0–1.0, but SkColorSetARGB expects uint8 0–255

return SkColorSetARGB(((value >> 54) & 0x3ff) * (1/1023.0f),

((value >> 6) & 0x3ff) * (1/1023.0f),

...);

// Should be:

return SkColorSetARGB(((value >> 54) & 0x3ff) * (255/1023.0f),

((value >> 6) & 0x3ff) * (255/1023.0f),

...);

```

**Before the refactor**, the scale factor was `255/1023.0f`.

**Effect:** The float values 0.0–1.0 are implicitly truncated to `U8CPU`, so every pixel reads as `SkColorSetARGB(0, 0, 0, 0)` — transparent black.

### Bug 4 — `kR16G16B16A16_unorm_SkColorType`: same argument order as Bug 1

**[src/core/SkPixmap.cpp, line 348](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/core/SkPixmap.cpp;l=348)**

Same pattern as Bug 1:

```cpp

// Current (wrong):

return SkColorSetARGB(r, g, b, a);

// Should be:

return SkColorSetARGB(a, r, g, b);

```

## Reproducing

These can be verified by inspection — no runtime test needed:

1. `SkColorSetARGB` signature is `(a, r, g, b)` ([include/core/SkColor.h, line 50](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/include/core/SkColor.h;l=50))

2. Lines 284 and 348 pass `(r, g, b, a)` — alpha and red are swapped

3. Lines 305–320 read bits assuming BGRA for both RGBA and BGRA variants

4. Line 328 scales to [0,1] instead of [0,255]

Alternatively, create a pixmap of each affected type, write a known pixel value, call `getColor()`, and compare the result.

## Introducing commit

[`116f237bb3`](https://skia.googlesource.com/skia/+/116f237bb39d980c4127270c25fc287df358ad22) — "Use SkColor helpers for color generation in SkPixmap" (Nov 6, 2025)

## Version info

- **Introduced:** after chrome/m133, present from at least chrome/m147

- **Still present on `main`:** yes (verified against `3f467a5819`)

[mattl...@live.com]

[mattl...@live.com]

I think this fiddle also proves it: https://fiddle.skia.org/c/3a0df072cddd29233266ca262fcf8239

The top RGBW should be the same as the bottom one.

[Robert Phillips]

I have created: https://g-issues.skia.org/issues/506959794

--
You received this message because you are subscribed to the Google Groups "skia-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to skia-discuss...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/skia-discuss/01593029-8e9b-468a-9a4a-c80f1cd60556n%40googlegroups.com.