986328 - The image encoders' don't handle Moz2D SurfaceFormat::B8G8R8A8 data correctly on big-endian machines

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Jonathan Watt [:jwatt]]

It seems to me that the image encoders do not handle INPUT_FORMAT_HOSTARGB correctly. The code is a little tricky to follow since the INPUT_FORMAT_* constants specify the physical order of the bytes of the data that is passed while the conversion code has a mixture of physical- and logical-order operations.

The encoders want to handle data in RGBA physical order, so they have ConvertHostARGBRow functions to convert ARGB to RGBA. For example:

void
nsPNGEncoder::ConvertHostARGBRow(const uint8_t* aSrc, uint8_t* aDest,
                                 uint32_t aPixelWidth,
                                 bool aUseTransparency)
{
  uint32_t pixelStride = aUseTransparency ? 4 : 3;
  for (uint32_t x = 0; x < aPixelWidth; x ++) {
    const uint32_t& pixelIn = ((const uint32_t*)(aSrc))[x];
    uint8_t *pixelOut = &aDest[x * pixelStride];

    uint8_t alpha = (pixelIn & 0xff000000) >> 24;
    if (alpha == 0) {
      pixelOut[0] = pixelOut[1] = pixelOut[2] = pixelOut[3] = 0;
    } else {
      pixelOut[0] = (((pixelIn & 0xff0000) >> 16) * 255 + alpha / 2) / alpha;
      pixelOut[1] = (((pixelIn & 0x00ff00) >>  8) * 255 + alpha / 2) / alpha;
      pixelOut[2] = (((pixelIn & 0x0000ff) >>  0) * 255 + alpha / 2) / alpha;
      if (aUseTransparency)
        pixelOut[3] = alpha;
    }
  }
}

Taking the line:

      pixelOut[0] = (((pixelIn & 0xff0000) >> 16) * 255 + alpha / 2) / alpha;

the code is assigning to the first physical-order byte of the output. In other words we're setting the R component. To the right of the assignment operator we're operating on logical bytes, the code getting the second highest logical-order byte. On little endian machines the lowest logical-order byte is the first physical-order byte, so we're setting it to the third physical-order byte of the input. Since pixelIn is assumed to contain physical bytes in the order ARGB, which means we're setting it to the G component, not the R component.

So from the code it looks like the encoders are messing up the channel ordering during this conversion, but of course they don't or we would have noticed by now.

The reason that the component ordering is not messed up is because the input is actually in the reverse order from that claimed by the INPUT_FORMAT_HOSTARGB value and implied by the "ARGB" in ConvertHostARGBRow's name. We're actually passing Moz2D SurfaceFormat::B8G8R8A8 data to the encoder. So BGRA, not ARGB.

So the encoder's conversion code is getting bytes in the wrong (reverse) order for little endian machines (virtually everything), but because the bytes of the input data are also in the wrong (reverse) order to the order they should be in, things just work.

Comment 1

[Jonathan Watt [:jwatt]]

Actually the comment for INPUT_FORMAT_HOSTARGB says that the pixel data is in BGRA on little endian machines:

  // Input is host-endian ARGB: On big-endian machines each pixel is therefore
  // ARGB, and for little-endian machiens (Intel) each pixel is BGRA
  // (This is used by canvas to match it's internal representation)

So as we change endianness both the input bytes and the logical-order operations should flip, making this code work both on little endian and big endian machines.

There is a bug though when it comes to passing Moz2D SourceFormat::B8G8R8A8 data. For this source format I believe the physical byte order is always BGRA, regardless of endianness. If that is true then we are introducing brokenness as we convert to Moz2D code that is passing data to the encoders. If that's the case, and if we want to support big endian machines (I believe some B2G hardware is big endian), then we will need to extend the encoders to handle a new INPUT_FORMAT_BGRA flag and start passing that to the encoders when passing Moz2D B8G8R8A8 data.

Bas, can you confirm that the physical order of SourceFormat::B8G8R8A8 pixel data is always BGRA? I believe that was your intention, at least.

Comment 2

[Jonathan Watt [:jwatt]]

These tests should catch such encoder failures, but I don't think we're running them on any big endian machines:

https://mxr.mozilla.org/mozilla-central/source/image/test/reftest/encoders-lossless/encoder.html?force=1
https://mxr.mozilla.org/mozilla-central/source/image/test/unit/test_encoder_png.js

Comment 3

[Jonathan Watt [:jwatt]]

It's worth noting that we're not running the encoders-lossless reftests on Fennec or B2G currently:

https://mxr.mozilla.org/mozilla-central/source/image/test/reftest/reftest.list

We do seem to be running the test_encoder_* tests though:

https://mxr.mozilla.org/mozilla-central/source/image/test/unit/xpcshell.ini

Comment 4

[Bas Schouten (:bas.schouten)]

I believe we discussed this on IRC.

Comment 5

[Shelly Lin [:shelly]]

Hi Jonathan, 
That TV is based on gecko 26, will it be a concern or effect of this bug? 

Anyhow, if I run the "image/test/reftest/encoders-lossless/encoder.html" on TV (which claimed itself a big-endian machine), what's the expected false result?

Comment 6

[Curtis Hamilton]

After struggling with this for several months I've found what I believe is the source of this bug.  I had to go back and look at Firefox 44.0, the last version to build without exhibiting the flashing.  I discovered that a change between versions 44.0 and 45.0, Bug 1209812 - Add endian-neutral variants to SurfaceFormat, was the culprit.

I believe error is in the below code changes to gfx/2d/Types.h:

  // The following values are endian-independent synonyms. The _UINT32 suffix
  // indicates that the name reflects the layout when viewed as a uint32_t
  // value.
#if MOZ_LITTLE_ENDIAN
  A8R8G8B8_UINT32 = B8G8R8A8,       // 0xAARRGGBB
  X8R8G8B8_UINT32 = B8G8R8X8        // 0x00RRGGBB
#elif MOZ_BIG_ENDIAN
  A8R8G8B8_UINT32 = A8R8G8B8,       // 0xAARRGGBB
  X8R8G8B8_UINT32 = X8R8G8B8        // 0x00RRGGBB
#else
# error "bad endianness"
#endif

As I understand from reading the endianness values that preceed this code, the endian test should be reserved to be:

#if MOZ_BIG_ENDIAN
  A8R8G8B8_UINT32 = B8G8R8A8,       // 0xAARRGGBB
  X8R8G8B8_UINT32 = B8G8R8X8        // 0x00RRGGBB
#elif MOZ_LITTLE_ENDIAN
  A8R8G8B8_UINT32 = A8R8G8B8,       // 0xAARRGGBB
  X8R8G8B8_UINT32 = X8R8G8B8        // 0x00RRGGBB
#else
# error "bad endianness"
#endif

I've used this to patch Firefox which allows me to successfully build both Firefox 46 and 47 that do not exhibit the graphics flashing.