995807 - Make the image encoders (e.g. nsBMPEncoder) support Moz2D's SurfaceFormat::B8G8R8X8 format

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Jonathan Watt [:jwatt]]

It looks to me like nsBMPEncoder doesn't play nicely with Moz2D's SurfaceFormat::B8G8R8X8. For example, consider this code:

https://mxr.mozilla.org/mozilla-central/source/widget/windows/nsImageClipboard.cpp?rev=02876730f55f&mark=156,157,158#151

First of all, passing INPUT_FORMAT_RGB seems to be wrong since the physical byte order for SourceSurface data (at least on little-endian machines) is BGR, not RGB. Maybe we should be using INPUT_FORMAT_HOSTARGB here and relying on the decoder to ignore the fourth physical byte of each pixel if we pass the option bbp=24?

Regarding the bpp=24 option though, the nsBMPEncoder doesn't look like it handles that correctly for SurfaceFormat::B8G8R8X8 data. Passing that option means that nsBMPEncoder::mBMPInfoHeader.bpp ends up set to 24. That seems to mean both that we end up under nsBMPEncoder::EncodeImageDataRow24() and that that function reads data using 3 byte offsets (since it uses mBMPInfoHeader.bpp to determine that). For SurfaceFormat::B8G8R8X8 that's going to do the wrong thing though because the pixel data contains a fourth (unused) byte.

I've not looked at the other encoders, but I'd guess they must have similar bpp=24 problems.

Comment 1

[Jonathan Watt [:jwatt]]

Seth, it looks like we need an INPUT_FORMAT_HOSTXRGB flag to compliment INPUT_FORMAT_HOSTARGB. Do you agree?

Comment 2

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] [offline 17-28 April] from comment #0)
> First of all, passing INPUT_FORMAT_RGB seems to be wrong since the physical
> byte order for SourceSurface data (at least on little-endian machines)

Bas tells me that the intention for Moz2D is that the physical byte order is the same on both little-endian and big-endian architectures, but nobody seems sure whether that is the case or not. If it _is_ then we have another problem which is described in bug 986328.

Comment 3

[Jonathan Watt [:jwatt]]

The reason that this doesn't currently cause failures is because of the GetAsReadableARGB32ImageSurface that prevents us even getting into the |case SurfaceFormat::B8G8R8X8:| block:

https://mxr.mozilla.org/mozilla-central/source/widget/windows/nsImageClipboard.cpp?rev=02876730f55f&mark=128-129#128

Prior to bug 979853 there was a GetAsReadableARGB32ImageSurface() call that also made the gfxImageFormat::RGB24 handling block dead code.

With the current incarnation of bug 950372's patch we fail JP test:

https://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-clipboard.js?rev=02876730f55f&mark=181-194#181

Comment 4

[Seth Fowler [:seth] [:s2h]]

(In reply to Jonathan Watt [:jwatt] from comment #1)
> Seth, it looks like we need an INPUT_FORMAT_HOSTXRGB flag to compliment
> INPUT_FORMAT_HOSTARGB. Do you agree?

With the caveat that the encoders are the part of imagelib I'm least familiar with, yes, I agree. Even if there were a hack around the need for one, the appropriate place for that hack would be inside the decoder. Callers should be presented with an interface that makes the right thing to do obvious, and it seems to me that in this case that would mean adding an INPUT_FORMAT_HOSTXRGB flag.