Last Comment Bug 704558 - Accept encoder output options in encodeImage and encodeScaledImage
: Accept encoder output options in encodeImage and encodeScaledImage
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla11
Assigned To: Tim Abraldes [:TimAbraldes] [:tabraldes]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 702691
  Show dependency treegraph
 
Reported: 2011-11-22 11:30 PST by Tim Abraldes [:TimAbraldes] [:tabraldes]
Modified: 2011-12-17 09:31 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 - Concept (6.99 KB, patch)
2011-11-22 11:38 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
joe: feedback+
Details | Diff | Splinter Review
ImageLib & Widget Patch v1 - Adds ImageLib tests, makes outputOptions optional, and avoids excessive string conversion (11.46 KB, patch)
2011-11-29 20:50 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
netzen: review+
Details | Diff | Splinter Review
Patch with extra tests for imagelib (12.60 KB, patch)
2011-11-30 11:38 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
netzen: review+
Details | Diff | Splinter Review
Toolkit patch: Modifies consumer of encodeImage/encodeScaledImage to use the new parameter (1.06 KB, patch)
2011-11-30 11:41 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
robert.strong.bugs: review+
Details | Diff | Splinter Review
ImageLib & Widget Patch v2 - Combines patch v1 and patch with extra test cases (18.08 KB, patch)
2011-11-30 17:01 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
timabraldes: review+
Details | Diff | Splinter Review
ImageLib & Widget Patch v3 - Changes test cases to use different format options (all bmp and bpp; no png) (9.52 KB, patch)
2011-12-01 14:00 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
netzen: review-
Details | Diff | Splinter Review
ImageLib & Widget Patch v4 - Include new reference images (17.23 KB, patch)
2011-12-02 10:11 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
netzen: review+
Details | Diff | Splinter Review
ImageLib & Widget Patch v5 - Formatting (17.23 KB, patch)
2011-12-02 11:13 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
timabraldes: review+
Details | Diff | Splinter Review
Toolkit patch v2 - Correct reviewer info (1.05 KB, patch)
2011-12-02 11:52 PST, Tim Abraldes [:TimAbraldes] [:tabraldes]
timabraldes: review+
Details | Diff | Splinter Review

Description Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-11-22 11:30:06 PST
My specific use case is that I intend to pass "format=bmp" to our ICO encoder in a call to `encodeImage`.

I envision being able to do that from js like so (where imgContainer is an image container containing the desired icon image):

      let imgTools = Cc["@mozilla.org/image/tools;1"]
                     .createInstance(Ci.imgITools);
      imgTools.encodeImage(imgContainer.value,
                           "image/vnd.microsoft.icon",
                           "format=bmp");
Comment 1 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-11-22 11:38:16 PST
Created attachment 576215 [details] [diff] [review]
Patch v1 - Concept

This is what I'm thinking, modulo some nsAString/nsACString differences.  Brian, do you mind taking a look and letting me know what you think?  I won't be touching this again until next week so no rush.
Comment 2 Brian R. Bondy [:bbondy] 2011-11-22 11:44:47 PST
Yup, I'll take a look later this week or early next week.
Comment 3 Brian R. Bondy [:bbondy] 2011-11-27 09:07:28 PST
Comment on attachment 576215 [details] [diff] [review]
Patch v1 - Concept

This looks good to me, but I have one concern.  That there are other products and add-ons using this imgITools function already that this would break.  I'm wondering if it would be better to add a new function with the outputOptions parameter instead and have the old functions call into that function for their implementation.

Please check with Joe Drew to see if he'd prefer that way or the current way.

Also I think you'll have to break up this patch. I can review once you get feedback from Joe Drew for imagelib and widget but I'm not sure if I am allowed to r+ the toolkit/ change even know it is just adding a blank parameter to the existing function. 

Please r=me once you get the feedback from Joe.
Comment 4 Joe Drew (not getting mail) 2011-11-28 12:57:29 PST
Comment on attachment 576215 [details] [diff] [review]
Patch v1 - Concept

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

Make outputOptions optional, I think. Also note that you will need to add tests to imglib for this :)
Comment 5 Joe Drew (not getting mail) 2011-11-29 08:17:15 PST
I'm not concerned about changes to imgITools because binary addons always need to recompile for every new version of Firefox.
Comment 6 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-11-29 11:35:32 PST
Thanks Joe and Brian for the feedback!

I'll proceed as follows:
  1) Split the patch into reviewer-specific patches
  2) Make outputOptions optional
  3) Add tests to imglib
Comment 7 Brian R. Bondy [:bbondy] 2011-11-29 11:38:03 PST
Sounds good.  For #3 by the way you can add them to 
mozilla/image/test/unit/test_imgtools.js
Comment 8 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-11-29 20:50:48 PST
Created attachment 577847 [details] [diff] [review]
ImageLib & Widget Patch v1 - Adds ImageLib tests, makes outputOptions optional, and avoids excessive string conversion

Summary of changes:
   - Added 1 test case each for encodeImage and encodeScaledImage
   - Made outputOptions optional
   - I changed outputOptions from nsACString to nsAString since we're converting to nsAString anyway before using it

I'm running this through tryserver but I thought I'd make it available for review as well.  Let me know what you think!
Comment 9 Mozilla RelEng Bot 2011-11-30 00:50:25 PST
Try run for de4f53ec5ca1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=de4f53ec5ca1
Results (out of 178 total builds):
    success: 154
    warnings: 20
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-de4f53ec5ca1
Comment 10 Brian R. Bondy [:bbondy] 2011-11-30 08:46:29 PST
Comment on attachment 577847 [details] [diff] [review]
ImageLib & Widget Patch v1 - Adds ImageLib tests, makes outputOptions optional, and avoids excessive string conversion

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

Good work, just one suggestion for 2 new xpcshell tests.
r=? me with the 2 new xpcshell tests.

::: image/test/unit/test_imgtools.js
@@ +348,5 @@
> +
> +// we'll reuse the container from the previous test
> +istream = imgTools.encodeImage(container,
> +                               "image/vnd.microsoft.icon",
> +                               "format=bmp");

Although our current default is PNG format, in case this changes we should have another reftet the same as this one for format=png.

@@ +370,5 @@
> +istream = imgTools.encodeScaledImage(container,
> +                                     "image/vnd.microsoft.icon",
> +                                     16,
> +                                     16,
> +                                     "format=bmp");

Ditto.
Comment 11 Brian R. Bondy [:bbondy] 2011-11-30 08:47:49 PST
*reftet  -> xpcshell
Comment 12 Brian R. Bondy [:bbondy] 2011-11-30 11:00:18 PST
Try results looked ok btw.
Comment 13 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-11-30 11:38:27 PST
Created attachment 578025 [details] [diff] [review]
Patch with extra tests for imagelib
Comment 14 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-11-30 11:41:05 PST
Created attachment 578029 [details] [diff] [review]
Toolkit patch: Modifies consumer of encodeImage/encodeScaledImage to use the new parameter

Robert, you're listed as a peer for toolkit.  Let me know if I should r="someone else"
Comment 15 Robert Strong [:rstrong] (use needinfo to contact me) 2011-11-30 11:48:25 PST
Comment on attachment 578029 [details] [diff] [review]
Toolkit patch: Modifies consumer of encodeImage/encodeScaledImage to use the new parameter

Tim / Brian, would it make sense to specify outputOptions for this case? If you think so please add it and just carry forward my r+
Comment 16 Brian R. Bondy [:bbondy] 2011-11-30 11:57:33 PST
> Tim / Brian, would it make sense to specify outputOptions for this case?

Nope, should be EmptyString(), so it's all good for that patch.
Comment 17 Brian R. Bondy [:bbondy] 2011-11-30 16:42:00 PST
Comment on attachment 578025 [details] [diff] [review]
Patch with extra tests for imagelib

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

Looks good, I had to do a double take on both of the PNG ICOs being of size 8214 :).
You can qfold the 2 patches I reviewed together if you want before you push.
Comment 18 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-11-30 17:01:54 PST
Created attachment 578137 [details] [diff] [review]
ImageLib & Widget Patch v2 - Combines patch v1 and patch with extra test cases

qfold complete :)
Comment 19 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-11-30 20:36:31 PST
Built this locally (clobber build) with the qfolded patch and the toolkit patch.  XPCShell tests still work.  Running through try one more time just in case, but otherwise it sounds like this is ready for checkin
Comment 20 Mozilla RelEng Bot 2011-12-01 00:50:23 PST
Try run for 69ce91f7b8a0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=69ce91f7b8a0
Results (out of 190 total builds):
    success: 161
    warnings: 25
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-69ce91f7b8a0
Comment 21 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-01 10:04:26 PST
I am certainly glad I ran that through try.  Every build except "Win debug" and "WinXP debug" failed with this message:
    FAILED in test #13 -- test encoding an unscaled ICO with format options (png): arrays differ at index 2537

Somehow the "Win debug" and "WinXP debug" builds succeed though
Comment 22 Brian R. Bondy [:bbondy] 2011-12-01 10:16:51 PST
Hrm strange, I ran the new tests on my machine as well, I'm using win7 x64 (but an x86 build)
Comment 23 Brian R. Bondy [:bbondy] 2011-12-01 10:44:02 PST
suggested on irc to use bmp and bpp to ensure it's working correctly.
Comment 24 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-01 14:00:24 PST
Created attachment 578384 [details] [diff] [review]
ImageLib & Widget Patch v3 - Changes test cases to use different format options (all bmp and bpp; no png)

Spoke with bbondy and rs about this.  In the interest of getting this functionality into m-c quickly, I'm modifying the test cases to use different format options (no png), and separately I'll track down why png ICO encoding is different on Win Debug builds.

Brian - this is currently running through try.  If everything looks OK there could you review this patch again?
Comment 25 Mozilla RelEng Bot 2011-12-01 18:30:33 PST
Try run for 07119f6bccda is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=07119f6bccda
Results (out of 190 total builds):
    exception: 1
    success: 152
    warnings: 32
    failure: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-07119f6bccda
Comment 26 Mozilla RelEng Bot 2011-12-01 21:30:30 PST
Try run for 29f78624c640 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=29f78624c640
Results (out of 172 total builds):
    exception: 2
    success: 147
    warnings: 19
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tabraldes@mozilla.com-29f78624c640
Comment 27 Brian R. Bondy [:bbondy] 2011-12-01 22:32:23 PST
>  and separately I'll track down why png ICO encoding is different 
> on Win Debug builds.

Thanks I think that would be helpful.
The easiest way is probably to try to use a release build or a build using another platform.

If you happen to not reproduce on your own machine, one thing you can try with the old patch that is busted on non Win debug (in the context of another bug) is to put in the log a base64 encoded data URI ( http://en.wikipedia.org/wiki/Data_URI_scheme ) for each of the 2 images.
Then we can investigate both images from try's Windows Opt vs Windows Debug to see (visually at first) exactly how they differ.

I'm not sure if there's an easier way, but you can use do_print to output the 2 URIs and you can use btoa to get the base64 encoded string.  Test locally you should be able to paste the URI in your browser's URL bar and see the image.
Comment 28 Brian R. Bondy [:bbondy] 2011-12-01 22:51:46 PST
Comment on attachment 578384 [details] [diff] [review]
ImageLib & Widget Patch v3 - Changes test cases to use different format options (all bmp and bpp; no png)

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

Sorry, I was hoping to r+ this but it's missing the reference icon images.
I tried to run the test locally and it failed with missing icons.  (not just a display thing in splinter review).

For some reason the patch pushed to try does have the icons though.
r=me wit the new patch with the images.

::: image/test/unit/test_imgtools.js
@@ +345,5 @@
> +/* ========== 11 ========== */
> +testnum++;
> +testdesc = "test encoding an unscaled ICO with format options "
> +           + "(format=bmp;bpp=32)";
> +

nit: put plus on the previous line and the 2 strings aligned.  Ditto the other 3 testdesc = lines added.
Comment 29 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-02 10:11:16 PST
Created attachment 578623 [details] [diff] [review]
ImageLib & Widget Patch v4 - Include new reference images

Oops!  I had noticed the issue in tryserver so I pushed an updated patch there but I forgot to update the one here.  Thanks for testing/reviewing this Brian!
Comment 30 Brian R. Bondy [:bbondy] 2011-12-02 10:44:12 PST
Comment on attachment 578623 [details] [diff] [review]
ImageLib & Widget Patch v4 - Include new reference images

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

The nits from the previous review are still not implemented from this patch, but after you do those it's good to go to mozilla-inbound.
Comment 31 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-02 11:13:38 PST
Created attachment 578653 [details] [diff] [review]
ImageLib & Widget Patch v5 - Formatting

I must be under-caffeinated
Comment 32 Brian R. Bondy [:bbondy] 2011-12-02 11:15:07 PST
Looks good, thanks for fixing that up! Enjoy your coffees :)
Comment 33 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-02 11:42:45 PST
Tested locally and seems to work; adding checkin-needed

To the committer:
Please check in both patches (only checking in 1 will cause bustage).

Now acquiring caffeine...
Comment 34 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-02 11:52:48 PST
Created attachment 578674 [details] [diff] [review]
Toolkit patch v2 - Correct reviewer info

Just noticed the path had "r=???" instead of "r=rs"

Fixing
Comment 35 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-12 17:06:34 PST
(In reply to Brian R. Bondy [:bbondy] from comment #27)
> >  and separately I'll track down why png ICO encoding is different 
> > on Win Debug builds.
> 
> Thanks I think that would be helpful.
> The easiest way is probably to try to use a release build or a build using
> another platform.
> 
> If you happen to not reproduce on your own machine, one thing you can try
> with the old patch that is busted on non Win debug (in the context of
> another bug) is to put in the log a base64 encoded data URI (
> http://en.wikipedia.org/wiki/Data_URI_scheme ) for each of the 2 images.
> Then we can investigate both images from try's Windows Opt vs Windows Debug
> to see (visually at first) exactly how they differ.
> 
> I'm not sure if there's an easier way, but you can use do_print to output
> the 2 URIs and you can use btoa to get the base64 encoded string.  Test
> locally you should be able to paste the URI in your browser's URL bar and
> see the image.

I've filed bug 710079 about WinDebug builds producing .ico files that are different from other builds.
Comment 36 Brian R. Bondy [:bbondy] 2011-12-12 18:12:12 PST
Thanks for posting!
Comment 37 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-14 10:40:02 PST
Is there anything special I need to do to make sure the patches for this bug get into m-i?
Comment 38 Brian R. Bondy [:bbondy] 2011-12-14 10:47:16 PST
I can push these out for you Tim but the tree is currently closed without special approval. Please ping me again when it's open and I'll push them.
Comment 39 Tim Abraldes [:TimAbraldes] [:tabraldes] 2011-12-14 10:50:51 PST
Ah of course, I'm reading about that on the newgroups now.  Thanks Brian!  Sorry for the unnecessary ping :D
Comment 40 Brian R. Bondy [:bbondy] 2011-12-14 10:52:12 PST
np glad you pinged me as I didn't know I should push it out.  Pls give me a reminder on IRC and I'll get it pushed right away once open.

Note You need to log in before you can comment on or make changes to this bug.