670466 - Add basic support for .bmp encoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Brian R. Bondy [:bbondy]]

Bug 549468 calls for an ICO encoder.
ICO is a container format though and holds either a (or many) BMP or a PNG.

It would be the cleanest code if we first create a BMP encoder and then the ICO encoder would contain either a BMP or a PNG.
A PNG encoder already exists.

Creating the ICO encoder in that way would match the recent refactoring for the decoder that Bobby Holley recommended and I implemented inside of Bug 600556.

This bug is to create a BMP encoder.
I'm filing this as a separate task so that the patch and tests can be kept separate.

Comment 1

[Bobby Holley (:bholley)]

Heck yeah - I'm super pumped that all this is happening. Thanks Brian!

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Note that encoders are exposed to the web (through <canvas>.toDataURL) so we'll want tests there as well.

Comment 3

[Brian R. Bondy [:bbondy]]

bholley:
Thanks, I'm super pumped to work on it :)

khuey:
Thanks for the info, I'll use it.

Comment 4

[Brian R. Bondy [:bbondy]]

I'm starting on this task today. 

Plan:
- I plan to implement the BMP encoder and test it while developing through <canvas>.toDataURL.  
- I plan to use the second parameter of toDataURL to control the encoder options.
- I do not plan to implement the RLE8 and RLE4 compression inside this task, but will do all BPPs.
- I plan to create some reftests by:
  - Comparing to a reference PNG.
  - Creating a wrapper page that draws the reference PNG on a canvas.
  - Then uses toDataURL to invoke the encoder and get the image/bmp data.
  - Then hide's the canvas and create's an image with the data URL scheme.
  - I'll use the same reference PNGs as the decoder so I don't need to add a bunch of new files.

Comment 5

[Brian R. Bondy [:bbondy]]

Added depends on Bug 600556 because Bug 600556 should be applied/merged before this task's patch is applied once the patch is available.

Comment 6

[Rain [:rain1] (inactive)]

So assuming you only care about Windows 7 (for jumplist icons) here, the PNG encoder should be sufficient, since Windows is happy dealing with 16x16 PNGs.

Comment 7

[Brian R. Bondy [:bbondy]]

I thought of this too, but I wasn't sure if the encoders would be used in other places.  For example there is a shell integration task for pinned apps that may need to work before win7 with BMP ICOs.  

Another example is if someone uses toDataURL and then saves the data, the OS wouldn't be able to read it pre win7.

I am going to switch back to ICO temporarily and come back to this one to make a better ICO encoder once I'm done.

Comment 8

[Brian R. Bondy [:bbondy]]

What I'll do since most of the BMP encoder is almost done is always encode the ICOs with the contained BMP instead of PNG. If we later need encoding of PNG icons we can add that in a different task.   The ICO code will stay nice though and will use the BMP encoder instead of re-implementing the same code, so PNG could be added easily. 

That will work on older Windows OS as well if the icons are used in the OS.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Intermediate patch for bmp encoder

Attached is an intermediate patch for a BMP encoder that supports 24BPP and 32BPP.
Tomorrow I will finish up and also create ref tests.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: reftests for Bug 670466 and Bug 549468

Created a set of encoder reftests that can be used for any image encoder that has lossless compression (So PNG, BMP, ICO).
You could also use it for lossly formats but you'd have to use reference images of the same type instead of the PNG ones included.

The way the tests work is: 
- Copy a PNG image to a canvas using canvas.drawImage
- Then use canvas.toDataUrl to get the data
- Then hide the old image and canvas
- Then set the data to a new image 

So the data of the image is retrieved by using the appropriate encoder. 

Here is an example reftest:
HTTP == png.html?size-1x1.png   encoder.html?img=size-1x1.png&mime=image/png

The params on the right are the encoder to use and the source image to compare.

Took me a while to create these ref tests because I was getting security errors when using canvas.drawImage and canvas.toDataURL.
I learnt it was because of the origin-clean flag which is set if the origin is different for the image you draw to the canvas.
I debugged why and found that the file:// resource is considered to be a different origin for every path unless it is the same path.

To get around this I ended up having to make the ref tests run over HTTP so that the origin would be the same, and hence no security errors. 

Please do not test the BMP and ICO reftests until after I submit a new patch for the actual code changes (some are needed to pass the tests).

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Finalized patch for bmp encoder

The task is done but I'll wait until I do the ICO/BMP decoder review comments before asking for a review here in case there are formatting items and etc I can fix.

I support only 24BPP and 32BPP and in the context of this patch I think that is enough. Parsing options can be set like the other encoders.  Also in this patch is code for using canvas.toDataURL's second parameter for parsing options.

toDataURL's second parameter can be set to a 19 byte prefix followed by an image lib encoder options string. The prefix will ensure that if canvas does define some parameters for the image types in the future, that we will work properly.   The prefix is "-moz-parse-options:".  It works for png, jpeg, bmp, ico (all encoders).

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: reftests for Bug 670466 and Bug 549468

The reftests can be reviewed whenever you are available. From the last intermediate reftest patch, this one adds support for testing the different formats of each encoder.

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 545585 [details] [diff] [review]
reftests for Bug 670466 and Bug 549468

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

::: modules/libpr0n/test/reftest/encoders-lossless/encoder.html
@@ +5,5 @@
> +</head>
> +
> +<body> 
> +
> +  <img id="image_from_encoder" style="position: absolute;top:0px;left:0px;">

Is the position: absolute required? I thought that loading images bare and loading them with a single <img> was identical.

@@ +17,5 @@
> +    {
> +      name = name.replace(/[\[]/,"\\\[").replace(/[\]]/,"\\\]");
> +      var regexS = "[\\?&]"+name+"=([^&#]*)";
> +      var regex = new RegExp( regexS );
> +      var results = regex.exec( window.location.href );

If you use window.location.search, this can be made simpler. Take a look at https://developer.mozilla.org/en/window.location - in particular, the section titled "Nestle the variables obtained through the window.location.search string".

@@ +33,5 @@
> +    // init the reference image, and its info
> +    var img = document.getElementById('image1');
> +    img.src=imgURL;
> +    var width = img.width;
> +    var height = img.height;

You can't get the width and height of an image (or drawImage it) right after loading it. You need the rest of this function to be in an onload handler on the image.

@@ +51,5 @@
> +      dataURL = canvas.toDataURL(mimeType);
> +
> +    // Remove the canvas and other image elemnt
> +    document.body.removeChild(canvas);
> +    document.body.removeChild(img);

Since you're just removing these tags, you may as well not put them into the page's DOM at all. Instead:

var canvas = document.createElement("canvas");
var img = new Image();

(Image is special-cased; in general, you need to use document.createElement(), and document.createElement("img") is AIUI equivalent to new Image().)

@@ +60,5 @@
> +    if(dataURL.substring(0, mimeType.length+5) == "data:" + mimeType) {
> +      // Set the image to the BMP data URl
> +      var image_from_encoder = document.getElementById('image_from_encoder');
> +      image_from_encoder.width = width;
> +      image_from_encoder.height = height;

Don't set these; the width and height should be implicit in the data. If we encode the right image but at the wrong size, it's possible that setting these attributes could cause us to incorrectly pass the test.

@@ +61,5 @@
> +      // Set the image to the BMP data URl
> +      var image_from_encoder = document.getElementById('image_from_encoder');
> +      image_from_encoder.width = width;
> +      image_from_encoder.height = height;
> +      image_from_encoder.src = dataURL; 

Since image loading is asynchronous, you need to use "reftest-wait" to ensure that all your processing is finished before the reftest framework takes its snapshot. See "asynchronous tests" in http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt .

Here, you need to set an image_from_encoder onload handler to remove the reftest-wait class on the root element.

::: modules/libpr0n/test/reftest/encoders-lossless/png.html
@@ +10,5 @@
> +  <script>
> +
> +    var img = document.getElementById('image1');
> +    var imgURL = document.location.search.substr(1);
> +    img.src=imgURL;

You need reftest-wait here. See my comments on encoder.html.

::: modules/libpr0n/test/reftest/encoders-lossless/reftest.list
@@ +10,5 @@
> +# Then set the data to a new image hence invoking
> +# the appropriate encoder
> +#
> +# The tests should only be used with lossless images
> +# So not JPEG

I think you should say "lossless encoders" here.

@@ +112,5 @@
> +HTTP == png.html?size-16x16.png encoder.html?img=size-16x16.png&mime=image/ico&options=-moz-parse-options%3Abpp%3D32%3Bpng%3Dno
> +HTTP == png.html?size-17x17.png encoder.html?img=size-17x17.png&mime=image/ico&options=-moz-parse-options%3Abpp%3D32%3Bpng%3Dno
> +HTTP == png.html?size-31x31.png encoder.html?img=size-31x31.png&mime=image/ico&options=-moz-parse-options%3Abpp%3D32%3Bpng%3Dno
> +HTTP == png.html?size-32x32.png encoder.html?img=size-32x32.png&mime=image/ico&options=-moz-parse-options%3Abpp%3D32%3Bpng%3Dno
> +HTTP == png.html?size-33x33.png encoder.html?img=size-33x33.png&mime=image/ico&options=-moz-parse-options%3Abpp%3D32%3Bpng%3Dno

There are really a lot of options in this search string. A little documentation in this file on the syntax of the options would not go astray.

Comment 14

[Brian R. Bondy [:bbondy]]

Thanks for all the great feedback, I will implement it all.  

I had read about and used at one point reftest-wait but then removed it because I thought it wasn't needed.  I'll re-add it.

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Patch for working BMP encoder

Minor fixes so that the patch works with its dependant Bug 600556

I will wait for r? until I do some extra refactoring/commenting on the patch.

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: reftests for Bug 670466 and Bug 549468

Implemented all of the review feedback for the reftest patch

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch for working BMP encoder

Like the last patch but with more comments, ready for review.

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Patch for working BMP encoder

Same as last patch but fixes a couple of compile time warnings

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Patch for working BMP encoder

Rebased patch from head of mozilla-central (EnsureFrame instead of AppendFrame)

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 546546 [details] [diff] [review]
reftests for Bug 670466 and Bug 549468

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

::: modules/libpr0n/test/reftest/encoders-lossless/encoder.html
@@ +14,5 @@
> +  // Example: 
> +  // encoder.html?img=escape(reference_image.png)
> +  //             &mime=escape(image/ico)
> +  //             &options=escape(-moz-parse-options:bpp=24;png=no)
> +  var oGetVars = {};

See below for a more complete discussion on variable prefix; I think just calling this getVars would be OK, though.

@@ +31,5 @@
> +    }
> +    return(sValue);
> +  }
> +  if (window.location.search.length > 1) {
> +    var iCouple, aCouples = window.location.search.substr(1).split("&");

I'm not sure what the prefix on iCouple and aCouples are supposed to mean here, but "a" in general is used for parameters to functions (I know, I know...). In this case you could just have couples and couple, I think.

(Aside: you could declare iCouple inside the for loop.)

@@ +32,5 @@
> +    return(sValue);
> +  }
> +  if (window.location.search.length > 1) {
> +    var iCouple, aCouples = window.location.search.substr(1).split("&");
> +    for (var iCouplId = 0; iCouplId < aCouples.length; iCouplId++) {

Loop index could just be "i" here.

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: Patch for working BMP encoder

Patch the same as last submitted except:
- Fixed build error on linux with extra semicolon after running through try server.

Comment 22

[Brian R. Bondy [:bbondy]]

Attachment: reftests for Bug 670466 and Bug 549468 w/ review comments and an additional fix

Same as the last reftest patch that was r+ but with the review changes implemented.  I'm not sure either what those prefixes stood for in that context as I just took the get params code from the link you gave me.  I fixed it though to remove the prefixes.

I also fixed an intermittent failure that I noticed very rarely happens when I tried to test all of these patches on the try servers.   The problem was that the canvas was being created after the img.src was set, and the onload would try to use the canvas.  Rarely that handler was being called early and using the canvas before it was created.

Comment 23

[Brian R. Bondy [:bbondy]]

Attachment: imglib part of patch for working BMP encoder

This one wasn't reviewed yet, but I:
- re-based it for Bug 600556
- fixed formatting based on feedback from your other reviews
- Split the patch into 2

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: content/html part of patch for working BMP encoder

I will wait for a review on the content/html part of this patch from a module owner/peer in that section until the imglib part of the patch passes review.

Comment 25

[Brian R. Bondy [:bbondy]]

Comment on attachment 548058 [details]
reftests for Bug 670466 and Bug 549468 w/ review comments and an additional fix

Going to update this reftest patch to use the new mime types for icons and going to move it to Bug 549468 (ico encoder) instead of here.

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: imglib part of patch for working BMP encoder

Couple more changes on this patch after finishing the ICO review:
- removed monitor that you mentioned in ICO encoder is not needed
- Cleaned up parsing options like you asked for int he ICO encoder

Comment 27

[Joe Drew (not getting mail)]

Comment on attachment 548643 [details] [diff] [review]
imglib part of patch for working BMP encoder

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

::: modules/libpr0n/encoders/bmp/nsBMPEncoder.cpp
@@ +124,5 @@
> +// Calculates the number of padding bytes that are needed per row of image data
> +static inline PRUint32
> +PaddingBytes(PRUint32 aBPP, PRUint32 aWidth)
> +{
> +  PRUint32 rowSize = aWidth * aBPP / 8;

You could use BytesPerPixel(aBPP) here ;)

@@ +139,5 @@
> +                                             PRUint32 aInputFormat,
> +                                             const nsAString& aOutputOptions)
> +{
> +  // can't initialize more than once
> +  if (mImageBufferStart != nsnull || mImageBufferCurr != nsnull) {

You don't need to use != nsnull here (and throughout); just testing the pointer bare is fine.

@@ +196,5 @@
> +  // write each row: if we add more input formats, we may want to
> +  // generalize the conversions
> +  if (aInputFormat == INPUT_FORMAT_HOSTARGB) {
> +    // BMP requires RGBA with post-multiplied alpha, so we need to convert
> +    PRUint8* row = new PRUint8[mBMPInfoHeader.width * BytesPerPixel(mBMPInfoHeader.bpp)];

All your allocations should be fallible (and tested for), because these are user-controlled parameters. See https://developer.mozilla.org/en/Infallible_memory_allocation

Also, this row could be allocated outside the input format blocks, and could use nsAutoArrayPtr (RAII) so you don't have to worry about control flow.

::: modules/libpr0n/encoders/bmp/nsBMPEncoder.h
@@ +105,5 @@
> +  PRUint32 mImageBufferSize;
> +  // Keeps track of the number of bytes in the image buffer which are encoded
> +  PRUint32 mImageBufferUsed;
> +  // Keeps track of the number of bytes in the image buffer which are read
> +  PRUint32 mImageBufferReadPoint;

Do we need this, given we have mImageBufferCurr?

::: modules/libpr0n/src/Endian.h
@@ +42,5 @@
> +#if defined WORDS_BIGENDIAN || defined IS_BIG_ENDIAN
> +// We must ensure that the entity is unsigned
> +// otherwise, if it is signed/negative, the MSB will be
> +// propagated when we shift
> +#define LITTLE_TO_NATIVE16(x) (((((PRUint16) x) & 0xFF) << 8) | \

Because you're using PR* types, you should #include "prtypes.h" in here.

@@ +47,5 @@
> +  (((PRUint16) x) >> 8))
> +#define LITTLE_TO_NATIVE32(x) (((((PRUint32) x) & 0xFF) << 24) | \
> +  (((((PRUint32) x) >> 8) & 0xFF) << 16) | \
> +  (((((PRUint32) x) >> 16) & 0xFF) << 8) | \
> +  (((PRUint32) x) >> 24))

This should all be correctly indented.

Comment 28

[Brian R. Bondy [:bbondy]]

> Also, this row could be allocated outside the input format blocks, and could use nsAutoArrayPtr

I'm not sure if the freeing would match the allocating function.
nsAutoArrayPtr uses delete[] but the data should be allocated in a fallibly fashion using moz_alloc.

Comment 29

[Brian R. Bondy [:bbondy]]

*moz_malloc

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: imglib part of patch for working BMP encoder

Implemented review comments.

You may notice a couple of unchanged lines as well showing as changes, I fixed a problem I was having with inconsistent newlines to use Unix LF.

Comment 31

[Joe Drew (not getting mail)]

(In reply to comment #28)
> > Also, this row could be allocated outside the input format blocks, and could use nsAutoArrayPtr
> 
> I'm not sure if the freeing would match the allocating function.
> nsAutoArrayPtr uses delete[] but the data should be allocated in a fallibly
> fashion using moz_alloc.

You can use |new (fallible_t())| for fallible new.

Comment 32

[Joe Drew (not getting mail)]

Comment on attachment 550722 [details] [diff] [review]
imglib part of patch for working BMP encoder

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

Use |new (fallible_t())| and nsAutoArrayPtr as mentioned in comment 27, and r=me.

Comment 33

[Brian R. Bondy [:bbondy]]

Attachment: imglib part of patch for working BMP encoder

Implemented fallible allocation via new and nsAutoArrayPtr

Comment 34

[Joe Drew (not getting mail)]

Comment on attachment 550915 [details] [diff] [review]
imglib part of patch for working BMP encoder

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

The only comments I have are the same as for bug 549468.

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: content/html part of patch for working BMP encoder

Comment 36

[Brian R. Bondy [:bbondy]]

Attachment: imglib part of patch for working BMP encoder

Implemented review comments

Comment 37

[Brian R. Bondy [:bbondy]]

Attachment: imglib part of patch for working BMP encoder

Changed from const fallible to static fallible.

Comment 38

[Brian R. Bondy [:bbondy]]

Attachment: imglib part of patch for working BMP encoder

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 553578 [details] [diff] [review]
content/html part of patch for working BMP encoder

I don't know what this patch is trying to do, but
at least it leaks memory.
paramBuf should be freed at some point.

Perhaps you could use GetAsACString, not GetAsStringWithSize

Who uses -moz-parse-options and for what?
Do we have tests for it?

Comment 40

[Brian R. Bondy [:bbondy]]

Thanks Olli I'll fix that. 

> I don't know what this patch is trying to do

The purpose of the patch is to add an extra optional parameter to the canvas.ToDataURL call so that we can easily test image encoders using reftests.
For example it allows us to test bitmaps with all of the different bit depths and compression settings.

The spec says that this argument must be ignored by user agents if it is not a recognized option, so I prefixed it with a string that is specific to us. 
"Other arguments must be ignored and must not cause the user agent to raise an exception. A future version of this specification will probably define other parameters to be passed to these methods to allow authors to more carefully control compression settings, image metadata, etc."

Reference: http://www.w3.org/TR/html5/the-canvas-element.html#dom-canvas-todataurl


> Who uses -moz-parse-options and for what?
> Do we have tests for it?

We use it to test our encoders via reftests as of this patch.

Comment 41

[Brian R. Bondy [:bbondy]]

By the way if you are curious the ref tests can be found in the sister bug: Bug 549468

Comment 42

[Brian R. Bondy [:bbondy]]

Attachment: content/html part of patch for working BMP encoder

Implemented review comments.

Comment 43

[Joe Drew (not getting mail)]

Comment on attachment 553629 [details] [diff] [review]
imglib part of patch for working BMP encoder

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

previous r+ = I don't need to review this again if all you do is address comments :)

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 553745 [details] [diff] [review]
content/html part of patch for working BMP encoder

># HG changeset patch
># Parent e9ad27daf6613bada4dd688f018701b281812ec5
># User Brian R. Bondy <[e-mail]>
>Bug 670466 - Add basic support for .bmp encoder
>
>diff --git a/content/html/content/src/nsHTMLCanvasElement.cpp b/content/html/content/src/nsHTMLCanvasElement.cpp
>--- a/content/html/content/src/nsHTMLCanvasElement.cpp
>+++ b/content/html/content/src/nsHTMLCanvasElement.cpp
>@@ -308,19 +308,50 @@ nsHTMLCanvasElement::ToDataURLImpl(const
>       if (NS_SUCCEEDED(aEncoderOptions->GetAsDouble(&quality)) &&
>           quality >= 0.0 && quality <= 1.0) {
>         params.AppendLiteral("quality=");
>         params.AppendInt(NS_lround(quality * 100.0));
>       }
>     }
>   }
> 
>+  // If we haven't parsed the params check for proprietary options.
>+  // The proprietary option -moz-parse-options will take a image lib encoder
>+  // parse options string as is and pass it to the encoder.
>+  PRBool usingCustomParseOptions = PR_FALSE;
>+  if (params.Length() == 0) {
>+      static const PRUnichar mozParseOptions[] = L"-moz-parse-options:";
NS_NAMED_LITERAL_STRING(mozParseOptions, "-moz-parse-options:");

>+      nsAutoString paramString;
>+      if (NS_SUCCEEDED(aEncoderOptions->GetAsAString(paramString)) && 
>+          paramString.Length() > mozParseOptionsCharCount) {
>+        const nsDependentSubstring& start = Substring(paramString, 0, 
>+                                                      mozParseOptionsCharCount);
>+        if (start.Equals(mozParseOptions)) {
if (StringBeginsWith(paramString, mozParseOptions)) ....


>+          nsDependentSubstring parseOptions = Substring(paramString, 
>+                                                        mozParseOptionsCharCount, 
>+                                                        paramString.Length() - mozParseOptionsCharCount);
>+          params.Append(parseOptions);
>+          usingCustomParseOptions = PR_TRUE;
>+        }
>+      }
>+  }
You're using both 4-spaces and 2-spaces indentation. Should be 2-spaces

Sorry, I should have given these comments already in the first time.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: content/html part of patch for working BMP encoder v3

Implemented review comments.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 554501 [details] [diff] [review]
content/html part of patch for working BMP encoder v3


>+  // If there are unrecognized custom parse options, we should fall back to 
>+  // the default values for the encoder without any options at all.
>+  if (rv == NS_ERROR_INVALID_ARG && usingCustomParseOptions) {
>+    fallbackToPNG = false;
>+    nsAutoString noParams;
>+    rv = ExtractData(type, noParams, getter_AddRefs(stream), fallbackToPNG);
I think there isn't need for noParams.
You could use EmptyString()

Comment 47

[Brian R. Bondy [:bbondy]]

Attachment: content/html part of patch for working BMP encoder v3

Marked as r+ since it was already marked r+ by smaug, this was just to change an empty string object to EmptyString().

Comment 48

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/373bd41d9e28 # skip reftest on Android
http://hg.mozilla.org/mozilla-central/rev/f097100df8aa # reftests
http://hg.mozilla.org/mozilla-central/rev/cefa6f2d2dfb
http://hg.mozilla.org/mozilla-central/rev/52f2d362cbac