Accept encoder output options in encodeImage and encodeScaledImage

RESOLVED FIXED in mozilla11

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla11
x86
Windows 7
addon-compat, dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

17.23 KB, patch
TimAbraldes
: review+
Details | Diff | Splinter Review
1.05 KB, patch
TimAbraldes
: review+
Details | Diff | Splinter Review
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");
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.
Attachment #576215 - Flags: feedback?(netzen)
Yup, I'll take a look later this week or early next week.
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.
Attachment #576215 - Flags: feedback?(netzen) → feedback?(joe)
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 :)
Attachment #576215 - Flags: feedback?(joe) → feedback+
I'm not concerned about changes to imgITools because binary addons always need to recompile for every new version of Firefox.
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
Sounds good.  For #3 by the way you can add them to 
mozilla/image/test/unit/test_imgtools.js
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!
Attachment #576215 - Attachment is obsolete: true
Attachment #577847 - Flags: review?(netzen)

Comment 9

6 years ago
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
Keywords: addon-compat, dev-doc-needed
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.
Attachment #577847 - Flags: review?(netzen) → review+
*reftet  -> xpcshell
Try results looked ok btw.
Created attachment 578025 [details] [diff] [review]
Patch with extra tests for imagelib
Attachment #578025 - Flags: review?(netzen)
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"
Attachment #578029 - Flags: review?(robert.bugzilla)
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+
Attachment #578029 - Flags: review?(robert.bugzilla) → review+
> 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 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.
Attachment #578025 - Flags: review?(netzen) → review+
Created attachment 578137 [details] [diff] [review]
ImageLib & Widget Patch v2 - Combines patch v1 and patch with extra test cases

qfold complete :)
Attachment #577847 - Attachment is obsolete: true
Attachment #578025 - Attachment is obsolete: true
Attachment #578137 - Flags: review+
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

6 years ago
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
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
Hrm strange, I ran the new tests on my machine as well, I'm using win7 x64 (but an x86 build)
suggested on irc to use bmp and bpp to ensure it's working correctly.
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?
Attachment #578137 - Attachment is obsolete: true
Attachment #578384 - Flags: review?(netzen)

Comment 25

6 years ago
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

6 years ago
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
>  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 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.
Attachment #578384 - Flags: review?(netzen) → review-
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!
Attachment #578384 - Attachment is obsolete: true
Attachment #578623 - Flags: review?(netzen)
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.
Attachment #578623 - Flags: review?(netzen) → review+
Created attachment 578653 [details] [diff] [review]
ImageLib & Widget Patch v5 - Formatting

I must be under-caffeinated
Attachment #578623 - Attachment is obsolete: true
Attachment #578653 - Flags: review+
Looks good, thanks for fixing that up! Enjoy your coffees :)
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...
Whiteboard: checkin-needed
Created attachment 578674 [details] [diff] [review]
Toolkit patch v2 - Correct reviewer info

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

Fixing
Attachment #578029 - Attachment is obsolete: true
Attachment #578674 - Flags: review+
(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.
Thanks for posting!
Is there anything special I need to do to make sure the patches for this bug get into m-i?
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.
Ah of course, I'm reading about that on the newgroups now.  Thanks Brian!  Sorry for the unnecessary ping :D
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.
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ff70e251a0f4
http://hg.mozilla.org/integration/mozilla-inbound/rev/4726e2ddf570
Target Milestone: --- → mozilla11
Whiteboard: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff70e251a0f4
https://hg.mozilla.org/mozilla-central/rev/4726e2ddf570
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.