722555 - Encoder for BMP should support RGBA.

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Kelsey Gilbert [:jgilbert]]

We currently don't support encoding RGBA BMPs, and I don't believe our decoder has complete support for them either.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

This patch refactors the BMP encoder/decoder to use BITMAPV7HEADER instead of BMPINFOHEADER. In contrast to the latter, BITMAPV7HEADER does not require the alpha channel to be stripped, allowing RGBA to be properly supported.

Note that BITMAPV7HEADER is a superset of BITMAPINFOHEADER, and the BMP decoder still only uses the information in the BITMAPINFOHEADER part. To support both header types, the decoder now reads whatever header size it is presented with.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 622892 [details] [diff] [review]
Patch to be reviewed

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

Why skip to V5 headers? Why not just add what we need with BITMAPV3INFOHEADER?

::: image/encoders/bmp/nsBMPEncoder.cpp
@@ +533,5 @@
> +  mBMPInfoHeader.important_colors = 0;
> +  mBMPInfoHeader.red_mask = 0xFF000000;
> +  mBMPInfoHeader.green_mask = 0x00FF0000;
> +  mBMPInfoHeader.blue_mask = 0x0000FF00;
> +  mBMPInfoHeader.alpha_mask = 0x000000FF;

I think these masks are backwards.

@@ +534,5 @@
> +  mBMPInfoHeader.red_mask = 0xFF000000;
> +  mBMPInfoHeader.green_mask = 0x00FF0000;
> +  mBMPInfoHeader.blue_mask = 0x0000FF00;
> +  mBMPInfoHeader.alpha_mask = 0x000000FF;
> +  mBMPInfoHeader.color_space = 0x73524742; // LCS_sRGB

This is probably not going to be sRGB data.

@@ +590,5 @@
> +  littleEndianmBIH.blue_mask = NATIVE32_TO_LITTLE(littleEndianmBIH.blue_mask);
> +  littleEndianmBIH.alpha_mask = NATIVE32_TO_LITTLE(littleEndianmBIH.alpha_mask);
> +  littleEndianmBIH.color_space = NATIVE32_TO_LITTLE(
> +                                 littleEndianmBIH.color_space);
> +  littleEndianmBIH.color_space = NATIVE32_TO_LITTLE(

Should be |littleEndianmBIH.white_point.r.x = |, along with the below.

@@ +664,5 @@
>      mImageBufferCurr += sizeof(littleEndianmBIH.colors);
>      memcpy(mImageBufferCurr, &littleEndianmBIH.important_colors, 
>             sizeof(littleEndianmBIH.important_colors));
>      mImageBufferCurr += sizeof(littleEndianmBIH.important_colors);
> +    memcpy(mImageBufferCurr, &littleEndianmBIH.red_mask, 

While we're here, please consider converting these to an inline function or macro. There is so much copy-pasta, especially with the white_point.* data, that this is pretty unreadable.

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 622892 [details] [diff] [review]
Patch to be reviewed

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

So I have some bad news, the icons generated with the v5 header do not display on XP.  
We currently use the ICO encoder for web apps as well as another task for website shortcuts on XP.  In Win7 also for Jump lists.  It seems like both of those XP tasks will need the v3 headers on XP.  The icons do work in Windows 7.  I also tested on Vista and they work there too.  A large % of our user base is XP so we need to work there too.

We could maybe add an encoder option to specify which type of header we want for both the BMP and ICO encoders.  The default would be the v3 40 byte header.

mspaint can read the BMPs of this type though. So I think the clipboard image bug you're working on will work with the v5 headers, but I'm not sure.
It might be good to install an XP VM for continued work on this task.

I ran the reftests and currently get:
REFTEST INFO | Successful: 407 (407 pass, 0 load only)
REFTEST INFO | Unexpected: 100 (100 unexpected fail

Manually loading any ICO BMP fails with the following message:
The image "file:///C:/...." cannot be displayed because it contains errors.

::: image/decoders/nsBMPDecoder.cpp
@@ -238,5 @@
>              PostDataError();
>              return;
>          }
> -        if (mBFH.bihsize == OS2_BIH_LENGTH)
> -            mLOH = OS2_HEADER_LENGTH;

I don't think it's ok to drop OS2 support, but maybe check with Joe Drew.  Maybe I'm misinterpreting the intent of this change.

::: widget/windows/nsImageClipboard.cpp
@@ +190,5 @@
>      }
>      
>      ::GlobalUnlock(glob);
>  
>      *outBitmap = (HANDLE)glob;

You can remove this file from the patch (nsImageClipboard.cpp) since it only contains a whitespace change.

Comment 4

[Eddy Bruel [:ejpbruel]]

(In reply to Brian R. Bondy [:bbondy] from comment #3)
> Comment on attachment 622892 [details] [diff] [review]
> Patch to be reviewed
> 
> Review of attachment 622892 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I have some bad news, the icons generated with the v5 header do not
> display on XP.  
> We currently use the ICO encoder for web apps as well as another task for
> website shortcuts on XP.  In Win7 also for Jump lists.  It seems like both
> of those XP tasks will need the v3 headers on XP.  The icons do work in
> Windows 7.  I also tested on Vista and they work there too.  A large % of
> our user base is XP so we need to work there too.
> 
> We could maybe add an encoder option to specify which type of header we want
> for both the BMP and ICO encoders.  The default would be the v3 40 byte
> header.
> 

I also realized yesterday that the ICO decoder cannot assume that the ICO file will use a v5 header (which it does with the current patch). Instead, it needs to work with whatever header size the file contains (either os2, v3 or v5) and then only use the part it cares about.

> mspaint can read the BMPs of this type though. So I think the clipboard
> image bug you're working on will work with the v5 headers, but I'm not sure.
> It might be good to install an XP VM for continued work on this task.
> 

Most, if not all, sources I've encountered so far seem to confirm that if you put a CF_DIBV5 on the clipboard, Windows automatically converts this to a CF_DIB or CF_BITMAP if you request either of those formats. Your observations on Windows XP seem to confirm this as well, as does ClipSpy (which reports that the latter two are available).

> I ran the reftests and currently get:
> REFTEST INFO | Successful: 407 (407 pass, 0 load only)
> REFTEST INFO | Unexpected: 100 (100 unexpected fail
> 
> Manually loading any ICO BMP fails with the following message:
> The image "file:///C:/...." cannot be displayed because it contains errors.
> 
> ::: image/decoders/nsBMPDecoder.cpp
> @@ -238,5 @@
> >              PostDataError();
> >              return;
> >          }
> > -        if (mBFH.bihsize == OS2_BIH_LENGTH)
> > -            mLOH = OS2_HEADER_LENGTH;
> 
> I don't think it's ok to drop OS2 support, but maybe check with Joe Drew. 
> Maybe I'm misinterpreting the intent of this change.
> 

Thats a misinterpretation. Like I said before, the ICO decoder needs to be able to work with whatever header size is present in the file. The same is true for the BMP decoder. So instead of setting mLOH to the length of the OS2 header, we set it to whatever header size is present. We could do slightly better and assume that the only valid header lengths are OS2, V3, and V5, and assert otherwise. Note however that with this change, the decoder should also be able to work with V7 and up (it will only read the V3 part of a V7 header).

> ::: widget/windows/nsImageClipboard.cpp
> @@ +190,5 @@
> >      }
> >      
> >      ::GlobalUnlock(glob);
> >  
> >      *outBitmap = (HANDLE)glob;
> 
> You can remove this file from the patch (nsImageClipboard.cpp) since it only
> contains a whitespace change.

Comment 5

[Brian R. Bondy [:bbondy]]

> Like I said before, the ICO decoder needs to be able to work with 
> whatever header size is present in the file. The same is true for the BMP decoder.

This is true for our existing decoder but not the new one.  The existing one may not support all of the header fields but has basic support by skipping over the unknown fields.

Comment 6

[Eddy Bruel [:ejpbruel]]

The existing decoder assumes that the length of the bitmap header is mLOH. It is set in only two places: in the constructor, to WIN_HEADER_LENGTH, and on line 242, to OS_HEADER_LENGTH. When mPos == mLOH, the decoder assumes that it has read the header.

Your statement that "The existing one may not support all of the header fields but has basic support by skipping over the unknown fields." seems to contradict the above observation. Am I missing something?

Comment 7

[Brian R. Bondy [:bbondy]]

That variable does not dictate that it won't support other bitmaps, it just holds the value of the length of the fields it knows about.

See:
http://mxr.mozilla.org/mozilla-central/source/image/decoders/nsBMPDecoder.cpp#436

Comment 8

[Eddy Bruel [:ejpbruel]]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 622892 [details] [diff] [review]
> Patch to be reviewed
> 
> Review of attachment 622892 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why skip to V5 headers? Why not just add what we need with
> BITMAPV3INFOHEADER?
> 

The V3 header does not support an alpha channel, according to MSDN, even for 32-bit bitmaps (the alpha bits are suppoed to be 0, and are simply ignored). There is an extension that allows the alpha bits to be actually used, but its only defined for Windows CE. The wiki article on the bitmap format states that the V4 header was the first to introduce alpha channel data into the bitmap,
but its undocumented. The V5 header is therefore the most obvious choice. 

Moreover, my overall goal is to add alpha channel support to the clipboard, and it also support bitmaps with a V5 header (via the CF_DIBV5 format).

> ::: image/encoders/bmp/nsBMPEncoder.cpp
> @@ +533,5 @@
> > +  mBMPInfoHeader.important_colors = 0;
> > +  mBMPInfoHeader.red_mask = 0xFF000000;
> > +  mBMPInfoHeader.green_mask = 0x00FF0000;
> > +  mBMPInfoHeader.blue_mask = 0x0000FF00;
> > +  mBMPInfoHeader.alpha_mask = 0x000000FF;
> 
> I think these masks are backwards.
> 

Looks like you're right. Sorry about that.

> @@ +534,5 @@
> > +  mBMPInfoHeader.red_mask = 0xFF000000;
> > +  mBMPInfoHeader.green_mask = 0x00FF0000;
> > +  mBMPInfoHeader.blue_mask = 0x0000FF00;
> > +  mBMPInfoHeader.alpha_mask = 0x000000FF;
> > +  mBMPInfoHeader.color_space = 0x73524742; // LCS_sRGB
> 
> This is probably not going to be sRGB data.
> 

Ehm, why not? The alternative, LCS_CALIBRATED, requires the white point and gamma values to be set. We don't do this for bitmaps that use the V3 header, so why would we for the V5 header? If I'm mistinterpreting this, please explain how you would do it.

> @@ +590,5 @@
> > +  littleEndianmBIH.blue_mask = NATIVE32_TO_LITTLE(littleEndianmBIH.blue_mask);
> > +  littleEndianmBIH.alpha_mask = NATIVE32_TO_LITTLE(littleEndianmBIH.alpha_mask);
> > +  littleEndianmBIH.color_space = NATIVE32_TO_LITTLE(
> > +                                 littleEndianmBIH.color_space);
> > +  littleEndianmBIH.color_space = NATIVE32_TO_LITTLE(
> 
> Should be |littleEndianmBIH.white_point.r.x = |, along with the below.
> 
> @@ +664,5 @@
> >      mImageBufferCurr += sizeof(littleEndianmBIH.colors);
> >      memcpy(mImageBufferCurr, &littleEndianmBIH.important_colors, 
> >             sizeof(littleEndianmBIH.important_colors));
> >      mImageBufferCurr += sizeof(littleEndianmBIH.important_colors);
> > +    memcpy(mImageBufferCurr, &littleEndianmBIH.red_mask, 
> 
> While we're here, please consider converting these to an inline function or
> macro. There is so much copy-pasta, especially with the white_point.* data,
> that this is pretty unreadable.

Comment 9

[Brian R. Bondy [:bbondy]]

There is an undocumented V3 header that is widely used even by Windows itself and programs like Photoshop/GIMP that does support an alpha channel.  It is also supported in the documented V4 header.  This is what we used to use for ICOs only that contain BMPs and hence why webapps an have transparent ICOs. See Bug 731824 which was solved simply by using 32bpp BMPs inside ICOs with our current encoder.

It's my understanding that the V5 header is supported even on Windows 2000 but possibly not within an ICO file.  

It is not related to Windows CE specifically.  I Think you are thinking of BI_ALPHABITFIELDS for that.  

That being said I'm personally perfectly OK with a V5 header when appropriate.

Comment 10

[Brian R. Bondy [:bbondy]]

Adding in Neil since he knows a lot about the BMP file format and may have an opinion about this task.

Comment 11

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

Ok, so as bbondy pointed out, we dont need v5 header support in order to have alpha channel. However, I still think that having v5 headers as an option in the encoder makes sense, because the clipboard explicitly supports them through the CF_DIBV5 format.

This patch is much simpler than the previous one, because it only uses the v5 header if you specify that as an option to the BMP encoder. The decoder is now virtually unchanged.

jgilbert, I haven't been able to reach you on irc regarding the sRGB issue, but I explained my reasoning in an earlier comment. Feel free to r- if my reasoning is faulty, but please do add an explanation of how I should implement it instead :-)

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 624187 [details] [diff] [review]
Patch to be reviewed

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

The logic seems fine.
I would like assurance that specifying sRGB will not change how the BMP is interpreted, versus with header versions which don't specify it.
A number of our internal sources (e.g. canvas, webgl) use linear RGB, not sRGB, so this in important.

We also probably need test coverage for both decoding and encoding of BMP files of various types.

::: image/encoders/bmp/nsBMPEncoder.cpp
@@ +151,5 @@
>      return NS_ERROR_INVALID_ARG;
>    }
>  
>    // parse and check any provided output options
> +  PRUint32 version;

'version' should be fine as an int. There's no reason to make it sized.
An enum would be best, though.

@@ +287,3 @@
>  {
> +  if (version) {
> +      *version = 3;

4-space indent in 2-space file.

@@ +555,5 @@
>    }
>    mBMPInfoHeader.xppm = 0;
>    mBMPInfoHeader.yppm = 0;
> +  if (aVersion >= 5) {
> +      mBMPInfoHeader.red_mask = 0x000000FF;

4-space indent in 2-space file.

@@ +558,5 @@
> +  if (aVersion >= 5) {
> +      mBMPInfoHeader.red_mask = 0x000000FF;
> +      mBMPInfoHeader.green_mask = 0x0000FF00;
> +      mBMPInfoHeader.blue_mask = 0x00FF0000;
> +      mBMPInfoHeader.alpha_mask = 0xFF000000;

Even these out so they line up, so it's easy to check at a glance.

@@ +559,5 @@
> +      mBMPInfoHeader.red_mask = 0x000000FF;
> +      mBMPInfoHeader.green_mask = 0x0000FF00;
> +      mBMPInfoHeader.blue_mask = 0x00FF0000;
> +      mBMPInfoHeader.alpha_mask = 0xFF000000;
> +      mBMPInfoHeader.color_space = 0x73524742; // =LCS_sRGB

Let's use a define for this.

@@ +624,5 @@
>    littleEndianmBIH.yppm = NATIVE32_TO_LITTLE(littleEndianmBIH.yppm);
>    littleEndianmBIH.colors = NATIVE32_TO_LITTLE(littleEndianmBIH.colors);
>    littleEndianmBIH.important_colors = NATIVE32_TO_LITTLE(
>                                        littleEndianmBIH.important_colors);
> +  littleEndianmBIH.red_mask = NATIVE32_TO_LITTLE(littleEndianmBIH.red_mask);

Strongly consider a helper function or macro here, so we only need something like:
ConvertToLittle(littleEndianmBIH.red_mask);

@@ +662,3 @@
>  
> +  if (mBMPFileHeader.bihsize == OS2_BIH_LENGTH) {
> +      memcpy(mImageBufferCurr, &littleEndianmBIH.width, 2);

4-space indent in 2-space file.

@@ +764,5 @@
> +           sizeof(littleEndianmBIH.profile_size));
> +    mImageBufferCurr += sizeof(littleEndianmBIH.profile_size);
> +    memcpy(mImageBufferCurr, &littleEndianmBIH.reserved, 
> +           sizeof(littleEndianmBIH.reserved));
> +    mImageBufferCurr += sizeof(littleEndianmBIH.reserved);

This is hard to visually parse. Please use a function or macro here so we just have a list of all the vars this needs to be done for.

::: image/src/BMPFileHeaders.h
@@ +70,3 @@
>  
> +    struct xyz {
> +        PRInt32 x, y, z;

4-space indent in 2-space file.

@@ +72,5 @@
> +        PRInt32 x, y, z;
> +    };
> +
> +    struct xyzTriple {
> +        struct xyz r, g, b;

Here also.

@@ +75,5 @@
> +    struct xyzTriple {
> +        struct xyz r, g, b;
> +    };
> +
> +	struct BITMAPV5HEADER {

Tab used as indent.

@@ +89,5 @@
>        PRUint32 colors; // Used Colors
>        PRUint32 important_colors; // Number of important colors. 0=all
> +      PRUint32 red_mask; // Bits used for red component
> +      PRUint32 green_mask; // Bits used for green component
> +	  PRUint32 blue_mask; // Bits used for blue component

This has a tab in the indent.

@@ +96,5 @@
> +      // These members are unused unless color_space == LCS_CALIBRATED_RGB
> +      xyzTriple white_point; // Logical white point
> +      PRUint32 gamma_red; // Red gamma component
> +      PRUint32 gamma_green; // Green gamma component
> +      PRUint32 gamma_blue; // Blue gamma component

Line up the comments for similar variables, such as red/green/blue here.

Comment 13

[Brian R. Bondy [:bbondy]]

> We also probably need test coverage for both decoding and encoding of 
> BMP files of various types.

We have several hundred reftests for this but we don't have coverage for 32bpp decoder.  It should be added here: image/test/reftest/bmp/bmp-32bpp

Also adding some transparent tests for BMP inside that folder like the ones that are already there for ICO would be good:
image/test/reftest/ico/ico-bmp-32bpp

Comment 14

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

This patch should address all the concerns you had with the previous one. Let me know if I missed anything or if there's anything else I messed up.

I still need to get confirmation that specifying sRGB will not change how the BMP is interpreted, but will do that before landing the patch.

Comment 15

[Eddy Bruel [:ejpbruel]]

Jeff, any chance you can get around to reviewing this patch tomorrow?

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 625530 [details] [diff] [review]
Patch to be reviewed

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

nsBMPDecoder looks right, but I want to take a chance to check it more thoroughly.

::: image/decoders/nsBMPDecoder.h
@@ +96,1 @@
>      char mRawBuf[36];

Let's define the size of mRawBuf in terms of WIN_V3_INTERNAL_BIH_LENGTH.

::: image/encoders/bmp/nsBMPEncoder.cpp
@@ +151,5 @@
>      return NS_ERROR_INVALID_ARG;
>    }
>  
>    // parse and check any provided output options
> +  int version;

This should be an enum.

@@ +311,5 @@
>      if (nameValuePair.Length() != 2) {
>        return NS_ERROR_INVALID_ARG;
>      }
>  
> +    // Parse the bpp portion of the string name=value;version=<version_value>;

I don't think we can just add this, since I believe this is passed through from toDataURL requests, for example. (In which case we must ignore extra arguments)

@@ +557,5 @@
>    mBMPInfoHeader.yppm = 0;
> +  if (aVersion >= 5) {
> +      mBMPInfoHeader.red_mask   = 0x000000FF;
> +      mBMPInfoHeader.green_mask = 0x0000FF00;
> +      mBMPInfoHeader.blue_mask  = 0x00FF0000;

Data in gfxASurfaces will allegedly be ARGB, therefore 0xAARRGGBB.

::: image/encoders/bmp/nsBMPEncoder.h
@@ +83,5 @@
> +  void InitInfoHeader(int aVersion, PRUint32 aBPP, PRUint32 aWidth,
> +                      PRUint32 aHeight);
> +  
> +  template<typename T>
> +  inline void Encode(const T& value)

Why is this out here in the header?

::: image/src/BMPFileHeaders.h
@@ +74,5 @@
> +      PRInt32 x, y, z;
> +    };
> +
> +    struct xyzTriple {
> +      struct xyz r, g, b;

Is this c-style struct-type specification necessary?

Comment 17

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

Would this meet with your approval? Please take another look :-)

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 627378 [details] [diff] [review]
Patch to be reviewed

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

::: image/encoders/bmp/nsBMPEncoder.cpp
@@ -308,5 @@
>      if (!ParseString(nameValuePairs[i], '=', nameValuePair)) {
>        return NS_ERROR_INVALID_ARG;
>      }
> -    if (nameValuePair.Length() != 2) {
> -      return NS_ERROR_INVALID_ARG;

We need to keep this to assure that nameValuePair[0] and [1] exist.

@@ +563,5 @@
> +  if (aVersion >= VERSION_5) {
> +      mBMPInfoHeader.red_mask   = 0x00FF0000;
> +      mBMPInfoHeader.green_mask = 0x0000FF00;
> +      mBMPInfoHeader.blue_mask  = 0x000000FF;
> +      mBMPInfoHeader.alpha_mask = 0xFF000000;

I was wrong about these, since we actually convert our 0xBGRA data to 0xABGR in our conversion code. (24-bit is 0xBGR, and old-32-bit is 0xXBGR, so this makes sense)

@@ +564,5 @@
> +      mBMPInfoHeader.red_mask   = 0x00FF0000;
> +      mBMPInfoHeader.green_mask = 0x0000FF00;
> +      mBMPInfoHeader.blue_mask  = 0x000000FF;
> +      mBMPInfoHeader.alpha_mask = 0xFF000000;
> +      mBMPInfoHeader.color_space = LCS_sRGB;

I don't know what exactly to do about this, but I suppose all we can do at the moment is leave it as sRGB, and hope for the best.

Comment 19

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 630173 [details] [diff] [review]
Patch to be reviewed

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

One last thing to help with readability.

::: image/encoders/bmp/nsBMPEncoder.cpp
@@ +538,5 @@
> +template<typename T>
> +inline void nsBMPEncoder::Encode(const T& value)
> +{
> +    memcpy(mImageBufferCurr, &value, sizeof(T));
> +    mImageBufferCurr += sizeof(T);

Make this a thin (and static) helper function, so the context of where we're copying to is apparent in the calling function.

That is something like: |Encode(littleEndianBFH.signature, mImageBufferCurr)| (or the other way around. I think I slightly prefer this order, but whichever)

This should bring us to something like |static inline void Encode(const T& value, PRUint8*& dest)|

::: image/encoders/bmp/nsBMPEncoder.h
@@ +88,5 @@
> +  void InitInfoHeader(Version aVersion, PRUint32 aBPP, PRUint32 aWidth,
> +                      PRUint32 aHeight);
> +  
> +  template<typename T>
> +  inline void Encode(const T& value);

This shouldn't need to be out here in the header.

Comment 21

[Eddy Bruel [:ejpbruel]]

(In reply to Jeff Gilbert [:jgilbert] from comment #20)
> Comment on attachment 630173 [details] [diff] [review]
> Patch to be reviewed
> 
> Review of attachment 630173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One last thing to help with readability.
> 
> ::: image/encoders/bmp/nsBMPEncoder.cpp
> @@ +538,5 @@
> > +template<typename T>
> > +inline void nsBMPEncoder::Encode(const T& value)
> > +{
> > +    memcpy(mImageBufferCurr, &value, sizeof(T));
> > +    mImageBufferCurr += sizeof(T);
> 
> Make this a thin (and static) helper function, so the context of where we're
> copying to is apparent in the calling function.
> 
> That is something like: |Encode(littleEndianBFH.signature,
> mImageBufferCurr)| (or the other way around. I think I slightly prefer this
> order, but whichever)
> 
> This should bring us to something like |static inline void Encode(const T&
> value, PRUint8*& dest)|
> 
Why didn't you mention that in your previous review? We've been going back and forth several times now, improving the readability of code of which most wasn't even introduced by my patch. That's fine to an extent, but its starting to become frustrating.

I'll add the change you proposed, but please, if there's anything else you'd like to see improved, let me know now, not during the next review.

> ::: image/encoders/bmp/nsBMPEncoder.h
> @@ +88,5 @@
> > +  void InitInfoHeader(Version aVersion, PRUint32 aBPP, PRUint32 aWidth,
> > +                      PRUint32 aHeight);
> > +  
> > +  template<typename T>
> > +  inline void Encode(const T& value);
> 
> This shouldn't need to be out here in the header.
Yes it does? The compiler will complain if you define a member function that has not been declared as part of the class.

Of course, it can go away if we turn it into a static function, as you proposed.

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment #16 contains my original concerns about that being in the header, which, had you addressed it in your follow-up response, would have lead to me realize earlier that what I was hoping to be a static helper function was instead a too-opaque member function.

In my previous review, it looks like I overlooked this by accident.

I understand that the review process can be frustrating, but when we're in...'inelegant' code, it really does behoove us to try to improve it somewhat, instead of just following the 'style' of the code around it with our additions. Without this, we would have had a hundred lines or so of unreadable memcpy/pointer-increment pairs. 

This should be the last round of review, since everything else looks great now.

Comment 23

[Eddy Bruel [:ejpbruel]]

(In reply to Jeff Gilbert [:jgilbert] from comment #22)
> Comment #16 contains my original concerns about that being in the header,
> which, had you addressed it in your follow-up response, would have lead to
> me realize earlier that what I was hoping to be a static helper function was
> instead a too-opaque member function.
> 
> In my previous review, it looks like I overlooked this by accident.
> 

If you're referring to the template declaration, that really *should* be there if you want to define a member template. I could not remove it without turning the method into a static function, which I would have done had I known that had your preference.

> I understand that the review process can be frustrating, but when we're
> in...'inelegant' code, it really does behoove us to try to improve it
> somewhat, instead of just following the 'style' of the code around it with
> our additions. Without this, we would have had a hundred lines or so of
> unreadable memcpy/pointer-increment pairs. 
> 

Don't get me wrong. I really do appreciate your efforts to keep code complexity down.

From my point of view, I'm frustrated because the nature of my job requires me to fix bugs in wildly different parts of the platform, so I'm spending most of my time trying to understand parts of the platform I've never seen before. As a result, even a fairly trivial patch likes this takes a substantial amount of my time. I have tons of other stuff to do that has higher priority for my team, so the last thing I want to do is spend a lot of time cleaning up existing code for low priority bugs.

Having said that, I do understand your rationale as well, so I'm sorry if I came across angry. Friends? :-)

> This should be the last round of review, since everything else looks great
> now.

Awesome!

Comment 24

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

I went with the order you didn't prefer, because that's more consistent with the order used by memcpy. I hope that's ok.

I used a pointer to a pointer rather than a reference to a pointer fo pImageBufferCurr to make explicit the fact that the contents of that value are changed by the call to Encode.

Comment 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 630942 [details] [diff] [review]
Patch to be reviewed

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

Great, thanks.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Changing to just target Encoding in this pass, since that's all we need for transparent clipboard DIBs. Transparent decode *might* work, but should probably be double-checked. If not, it should be fixed in a follow up.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Try running:
https://tbpl.mozilla.org/?tree=Try&rev=d4f4ec7cc2ae

Comment 28

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3bd7bd9093

bbondy approved.

Comment 29

[Matt Brubeck (:mbrubeck)]

Sorry, we had to back this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec473a671b9b

Because Linux PGO and Linux64 PGO builds failed with an internal compiler error:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12500962&tree=Mozilla-Inbound

Comment 30

[Kelsey Gilbert [:jgilbert]]

PGO builds are segfaulting on linux. :<

/usr/bin/ccache /tools/gcc-4.5-0moz3/bin/g++ -o nsBMPEncoder.i_o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /builds/slave/m-in-lnx-pgo/build/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -I/builds/slave/m-in-lnx-pgo/build/image/src/ -I/builds/slave/m-in-lnx-pgo/build/image/encoders/bmp -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/builds/slave/m-in-lnx-pgo/build/obj-firefox/dist/include/nspr -I/builds/slave/m-in-lnx-pgo/build/obj-firefox/dist/include/nss      -fPIC  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -fprofile-generate -O3 -fomit-frame-pointer   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/nsBMPEncoder.pp /builds/slave/m-in-lnx-pgo/build/image/encoders/bmp/nsBMPEncoder.cpp
/builds/slave/m-in-lnx-pgo/build/image/encoders/bmp/nsBMPEncoder.cpp: In member function 'void nsBMPEncoder::EncodeFileHeader()':
/builds/slave/m-in-lnx-pgo/build/image/encoders/bmp/nsBMPEncoder.cpp:715:1: internal compiler error: Segmentation fault
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
make[8]: *** [nsBMPEncoder.i_o] Error 1
make[8]: Leaving directory `/builds/slave/m-in-lnx-pgo/build/obj-firefox/image/encoders/bmp'
make[7]: *** [libs] Error 2
make[7]: Leaving directory `/builds/slave/m-in-lnx-pgo/build/obj-firefox/image/encoders'
make[6]: *** [encoders_libs] Error 2
make[6]: *** Waiting for unfinished jobs....

Comment 31

[Brian R. Bondy [:bbondy]]

Any update on this?

Comment 32

[Masatoshi Kimura [:emk]]

(In reply to Eddy Bruel [:ejpbruel] from comment #14)
> I still need to get confirmation that specifying sRGB will not change how
> the BMP is interpreted, but will do that before landing the patch.
Is this confirmed? For PNG, sRGB chunk surely changes the interpretation of the color.
Also note that toDataURL() do not allow including color space information in the output.
http://www.w3.org/TR/html5/the-canvas-element.html#color-spaces-and-color-correction

Comment 33

[Kelsey Gilbert [:jgilbert]]

(In reply to Masatoshi Kimura [:emk] from comment #32)
> (In reply to Eddy Bruel [:ejpbruel] from comment #14)
> > I still need to get confirmation that specifying sRGB will not change how
> > the BMP is interpreted, but will do that before landing the patch.
> Is this confirmed? For PNG, sRGB chunk surely changes the interpretation of
> the color.
> Also note that toDataURL() do not allow including color space information in
> the output.
> http://www.w3.org/TR/html5/the-canvas-element.html#color-spaces-and-color-
> correction

The problem is a lack of options. The only documented option actually appears to be LCS_CALIBRATED_RGB, according to:
http://msdn.microsoft.com/en-us/library/windows/desktop/dd183380%28v=vs.85%29.aspx

Documentation for V5 indicates that we can choose from sRGB, CALIBRATED, or WINDOWS_COLOR_SPACE (which is generally sRGB, according the the same doc):
http://msdn.microsoft.com/en-us/library/windows/desktop/dd183381%28v=vs.85%29.aspx

BITMAPV3INFOHEADER does not require this color space info (while providing RGBA support), but it's not actually documented, at least by Microsoft. 

A third option is to try just putting an invalid value there, but unfortunately, we can't even use '0', since that's LCS_CALIBRATED_RGB. We could use something else (say, the ascii values for 'flat' or just 'rgb'), but there doesn't appear to be a great answer to this problem.

Comment 34

[Masatoshi Kimura [:emk]]

This document claims that only 32-bpp BI_RGB bitmap can contain alpha values.
http://msdn.microsoft.com/en-us/library/windows/desktop/dd183352%28v=vs.85%29.aspx
And the example uses v3 BITMAPINFOHEADER.
http://msdn.microsoft.com/en-us/library/windows/desktop/dd183353%28v=vs.85%29.aspx

Comment 35

[Kelsey Gilbert [:jgilbert]]

(In reply to Masatoshi Kimura [:emk] from comment #34)
> This document claims that only 32-bpp BI_RGB bitmap can contain alpha values.
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd183352%28v=vs.
> 85%29.aspx

This is correct, but BI_RGB (which is what we use) has nothing to do with LCS_sRGB.

> And the example uses v3 BITMAPINFOHEADER.
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd183353%28v=vs.
> 85%29.aspx

I don't see how this uses the v3 version. It seems that it just assumes what the masks should be.

Comment 36

[Masatoshi Kimura [:emk]]

Ah, please disregard comment #34 (it states about alpha support of v3 bitmap).
I think v3 BITMAPINFO is a reasonable option here because it doesn't contain color space information. We don't have to indicate 'flat' or 'linear' or whatever RGB explicitly. Rather, we shouldn't include them in the toDataURL() output per spec.
Anyway my concern doesn't seem to be so important because the current patch will produce v5 BITMAP only when a proprietary option ('-moz-parse-options') is specified.

Comment 37

[Eddy Bruel [:ejpbruel]]

Introducing a temporary variable should have fixed the internal compiler bug issues. Running on try right now:
https://tbpl.mozilla.org/?tree=Try&rev=f5d229797792

Comment 38

[Eddy Bruel [:ejpbruel]]

(In reply to Eddy Bruel [:ejpbruel] from comment #37)
> Introducing a temporary variable should have fixed the internal compiler bug
> issues. Running on try right now:
> https://tbpl.mozilla.org/?tree=Try&rev=f5d229797792

Ignore that last comment. That wasn't the solution.

Comment 39

[Eddy Bruel [:ejpbruel]]

Replacing a templatized function with a macro, however, seems to have done the trick:
https://tbpl.mozilla.org/?tree=Try&rev=3f9e0b4ea425

Comment 40

[Kelsey Gilbert [:jgilbert]]

(In reply to Eddy Bruel [:ejpbruel] from comment #39)
> Replacing a templatized function with a macro, however, seems to have done
> the trick:
> https://tbpl.mozilla.org/?tree=Try&rev=3f9e0b4ea425

So much for better style, eh? :P Well worth it though, if we can land this.

Comment 41

[Eddy Bruel [:ejpbruel]]

Mercurial complained about adding remote heads even though I based my commit on the tip after the latest pull, so I had to do an additional merge before I could push. Am I doing this right?

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e46f0fa911d

Comment 42

[[e-mail]]

You need to import your changeset back into mq before pulling, updating, and exporting your changeset out of mq again, otherwise it will always be a head.

Comment 43

[Brian R. Bondy [:bbondy]]

To push the first patch in your patch queue, I typically do:
hg qpop -a
hg pull
hg update
hg qpush
hg qfinish -a
hg push repo_name

If you get an error that it would create a remote head (because someone pushed after you did the hg pull) then you can:
hg qimport -r tip
and redo the steps above

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5d1fa1a0a7a3