Last Comment Bug 891884 - toBlob should support the quality parameter as toDataURL does
: toBlob should support the quality parameter as toDataURL does
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: 22 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla25
Assigned To: Dale Harvey (:daleharvey)
:
Mentors:
: 822190 (view as bug list)
Depends on:
Blocks: 889765 889902
  Show dependency treegraph
 
Reported: 2013-07-10 07:35 PDT by Julien Wajsberg [:julienw]
Modified: 2013-08-15 08:58 PDT (History)
11 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
leo+
wontfix
wontfix
fixed
fixed
wontfix
wontfix
fixed


Attachments
wip patch v1 (10.76 KB, patch)
2013-07-11 10:38 PDT, Julien Wajsberg [:julienw]
no flags Details | Diff | Splinter Review
example web page to demonstrate this works (101.43 KB, text/html)
2013-07-12 00:19 PDT, Julien Wajsberg [:julienw]
no flags Details
example web page to demonstrate this works (101.59 KB, text/html)
2013-07-12 00:36 PDT, Julien Wajsberg [:julienw]
no flags Details
wip patch v2 (278.75 KB, patch)
2013-07-12 01:16 PDT, Julien Wajsberg [:julienw]
no flags Details | Diff | Splinter Review
wip patch v3 (379.08 KB, patch)
2013-07-12 07:31 PDT, Julien Wajsberg [:julienw]
no flags Details | Diff | Splinter Review
patch v1 (987.11 KB, patch)
2013-07-12 08:22 PDT, Julien Wajsberg [:julienw]
joe: review+
Details | Diff | Splinter Review
patch v2 (334 bytes, patch)
2013-07-12 09:28 PDT, Julien Wajsberg [:julienw]
felash: review+
Details | Diff | Splinter Review
patch v3 (255.31 KB, patch)
2013-07-13 04:09 PDT, Julien Wajsberg [:julienw]
felash: review+
mrbkap: superreview+
Details | Diff | Splinter Review
b2g18 patch v1 (252.51 KB, patch)
2013-07-13 07:03 PDT, Julien Wajsberg [:julienw]
joe: review+
Details | Diff | Splinter Review
patch v4 (92.67 KB, patch)
2013-07-17 21:27 PDT, Dale Harvey (:daleharvey)
dale: review+
dale: superreview+
Details | Diff | Splinter Review
b2g18 patch v1 (91.68 KB, patch)
2013-07-18 02:42 PDT, Dale Harvey (:daleharvey)
dale: review+
Details | Diff | Splinter Review
patch v5 (256.04 KB, patch)
2013-07-18 09:31 PDT, Dale Harvey (:daleharvey)
dale: review+
dale: superreview+
Details | Diff | Splinter Review
891884.b2g18.patch (252.96 KB, patch)
2013-07-22 19:56 PDT, Dale Harvey (:daleharvey)
dale: review+
Details | Diff | Splinter Review
891884.patch (256.44 KB, patch)
2013-07-24 04:24 PDT, Dale Harvey (:daleharvey)
dale: review+
dale: superreview+
Details | Diff | Splinter Review
891884.b2g18.patch (253.36 KB, patch)
2013-07-24 04:35 PDT, Dale Harvey (:daleharvey)
dale: review+
dale: superreview+
Details | Diff | Splinter Review

Description Julien Wajsberg [:julienw] 2013-07-10 07:35:39 PDT
as I understand from my reading of [1], we don't support the 3rd argument jpeg quality for toBlob whereas we do for toDataURL.

The spec [2] says that we should support it in both functions.

We badly needs this in Bug 889765 for Firefox OS... My understanding is that it's not so much work, we only need to parse the incoming arguments in the same way that we do for data URL.

[1] https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLCanvasElement.cpp#579

[2] http://www.w3.org/TR/html5/embedded-content-0.html#dom-canvas-toblob

Feel free to resolve WFM if I understood wrongly.

Asking for leo because bug 889765 is leo+.
Comment 1 John Hammink 2013-07-10 08:43:20 PDT
How badly is this needed?  Is there a workaround? Adding another function argument at this point brings some risk.
Comment 2 Julien Wajsberg [:julienw] 2013-07-10 08:49:57 PDT
for leo we could try to resize the image a little more, but I'd really want to use the quality instead.

I'd like to use it when generating the thumbnails in bug 889902 as well, which would probably save us some memory. And you know we need this :)
Comment 3 Julien Wajsberg [:julienw] 2013-07-11 02:53:35 PDT
*** Bug 822190 has been marked as a duplicate of this bug. ***
Comment 4 Julien Wajsberg [:julienw] 2013-07-11 10:38:19 PDT
Created attachment 774093 [details] [diff] [review]
wip patch v1

Here is a WIP patch.

code compile in the content/html directory but I haven't tested it yet and my full build has not finished yet.
Comment 5 Julien Wajsberg [:julienw] 2013-07-12 00:19:31 PDT
Created attachment 774502 [details]
example web page to demonstrate this works

load this file with and without this patch and you'll see the difference. Especially look at the "quality = 0" line.
Comment 6 Julien Wajsberg [:julienw] 2013-07-12 00:36:56 PDT
Created attachment 774511 [details]
example web page to demonstrate this works

the first one had a bug. qualities are < 1 :)
Comment 7 Julien Wajsberg [:julienw] 2013-07-12 01:16:41 PDT
Created attachment 774515 [details] [diff] [review]
wip patch v2

asking feedback because my reftests don't work yet
Comment 8 Julien Wajsberg [:julienw] 2013-07-12 02:50:18 PDT
Comment on attachment 774515 [details] [diff] [review]
wip patch v2

reworking the reftests, I had some explanation from :padenot.
Comment 9 Julien Wajsberg [:julienw] 2013-07-12 07:31:05 PDT
Created attachment 774642 [details] [diff] [review]
wip patch v3

reftests are passing, but they crashing when running only the new reftests. The don't crash when running with others...
Comment 10 Julien Wajsberg [:julienw] 2013-07-12 08:22:35 PDT
Created attachment 774672 [details] [diff] [review]
patch v1

try is https://tbpl.mozilla.org/?tree=Try&rev=f2138e09e6b8
Comment 11 Joe Drew (not getting mail) 2013-07-12 08:36:10 PDT
Comment on attachment 774672 [details] [diff] [review]
patch v1

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

Blake, can you sr the idl/webidl changes, since we're changing the web-accessible API?

::: content/html/content/reftests/toblob-todataurl/images/LICENSE
@@ +1,1 @@
> +this file comes from http://en.wikipedia.org/wiki/File:Example.png

I don't think you can do this. For one thing, it's not actually a license!

Better to just create your own, or copy something that we already have eg in image/test/reftest.

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +475,5 @@
> +HTMLCanvasElement::ParseParams(JSContext* aCx,
> +                               const nsAString& aType,
> +                               const JS::Value& aEncoderOptions,
> +                               nsAString& aParams,
> +                               bool& usingCustomParseOptions) {

{ on new line

@@ +484,5 @@
> +      double quality = aEncoderOptions.toNumber();
> +      // Quality must be between 0.0 and 1.0, inclusive
> +      if (quality >= 0.0 && quality <= 1.0) {
> +        aParams.AppendLiteral("quality=");
> +        aParams.AppendInt(NS_lround(quality * 100.0));

Oh man. I realize you only moved this code, but it's pretty crappy that we're doing this with string manipulation. I'd have much rather we fixed Imagelib to not need this stuff.

::: dom/interfaces/html/nsIDOMHTMLCanvasElement.idl
@@ +74,3 @@
>    void toBlob(in nsIFileCallback callback,
> +              [optional] in DOMString type,
> +              [optional] in jsval params);

I think you need to rev the nsIDOMHTMLCanvasElement uuid.
Comment 12 Julien Wajsberg [:julienw] 2013-07-12 09:28:32 PDT
Created attachment 774710 [details] [diff] [review]
patch v2

addressed comments from Joe. Thanks for the quick review !

I simplified how the reftests are done, now all html files share the same js files which makes it easier to edit. I did my own png file as well !

new try is https://tbpl.mozilla.org/?tree=Try&rev=d9938306af81
Comment 13 Julien Wajsberg [:julienw] 2013-07-12 09:29:35 PDT
will add a new patch with r= sr= once everything is cleared.
Comment 14 Julien Wajsberg [:julienw] 2013-07-12 09:32:15 PDT
(In reply to John Hammink from comment #1)
> How badly is this needed?  Is there a workaround? Adding another function
> argument at this point brings some risk.

Nothing in Gaia uses this yet afaik, so imho there is no risk here.

Joe, do you think it would be a good idea to add a reftest without a quality param too ?
Comment 15 Joe Drew (not getting mail) 2013-07-12 10:23:25 PDT
Yes, sure!
Comment 16 Blake Kaplan (:mrbkap) 2013-07-12 11:19:16 PDT
Comment on attachment 774710 [details] [diff] [review]
patch v2

>try: -b do -p linux,linux64,macosx64,win32,android,android-armv6,ics_armv7a_gecko -u reftest,reftest-no-accel,crashtest,mochitests -t none

I don't think this is the patch you wanted it to be :)
Comment 17 Julien Wajsberg [:julienw] 2013-07-13 03:37:33 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #16)
> Comment on attachment 774710 [details] [diff] [review]
> patch v2
> 
> >try: -b do -p linux,linux64,macosx64,win32,android,android-armv6,ics_armv7a_gecko -u reftest,reftest-no-accel,crashtest,mochitests -t none
> 
> I don't think this is the patch you wanted it to be :)

indeed ;)

patch v3 coming in some minutes
Comment 18 Julien Wajsberg [:julienw] 2013-07-13 04:09:14 PDT
Created attachment 775123 [details] [diff] [review]
patch v3

added reftests for:
* q=.92
* q=undefined
* no quality param

carrying r=joedrew

thanks !

new try : https://tbpl.mozilla.org/?tree=Try&rev=af9aa10d2e02
Comment 19 Julien Wajsberg [:julienw] 2013-07-13 07:03:37 PDT
Created attachment 775136 [details] [diff] [review]
b2g18 patch v1

Because b2g18 doesn't have webidl, I made an adhoc patch for it.

same reftests are working :)
I didn't updated the rev on the idl because strangely the idl already haves the necessary parameters on this branch.
 
try is https://tbpl.mozilla.org/?tree=Try&rev=7e8fda814820
Comment 20 Julien Wajsberg [:julienw] 2013-07-13 07:08:56 PDT
my previous try had some leftover patches, here is the "real" b2g18 try: https://tbpl.mozilla.org/?tree=Try&rev=fe71b198ba1e
Comment 21 Julien Wajsberg [:julienw] 2013-07-14 09:32:48 PDT
Dale was ok with finishing this.

Dale, I'd really like to have this in b2g18 :)
Comment 22 Joe Drew (not getting mail) 2013-07-15 05:57:47 PDT
Comment on attachment 775136 [details] [diff] [review]
b2g18 patch v1

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

Looks OK to me. No meaningful differences from the m-c difference, right?
Comment 23 Joe Drew (not getting mail) 2013-07-15 05:57:59 PDT
er, m-c patch.
Comment 24 Blake Kaplan (:mrbkap) 2013-07-15 12:05:17 PDT
Comment on attachment 775123 [details] [diff] [review]
patch v3

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

sr=mrbkap with the reference changed into a pointer and the nits addressed.

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +475,5 @@
> +HTMLCanvasElement::ParseParams(JSContext* aCx,
> +                               const nsAString& aType,
> +                               const JS::Value& aEncoderOptions,
> +                               nsAString& aParams,
> +                               bool& usingCustomParseOptions)

Gecko style is to use a pointer instead of a reference for out params like this.

@@ +477,5 @@
> +                               const JS::Value& aEncoderOptions,
> +                               nsAString& aParams,
> +                               bool& usingCustomParseOptions)
> +{
> +

Nit: extra blank line here.

@@ +573,5 @@
>  HTMLCanvasElement::ToBlob(nsIFileCallback* aCallback,
> +                          const nsAString& aType,
> +                          const JS::Value& aEncoderOptions,
> +                          JSContext* aCx
> +                          )

Nit: the ) should go next to the x on the same line as aCx.
Comment 25 bhavana bajaj [:bajaj] 2013-07-16 16:50:08 PDT
Unclear if this is really needed ? Can you please help suggesting the lowest risk option here between leo+ this bug or doing the workaround as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=889765#c18
Comment 26 Wayne Chang [:wchang] 2013-07-17 02:03:07 PDT
Triage - leo+ as required by 889765, will speak to schung with regards to comment 25 and alter nomination if the status changes.
Comment 27 Dale Harvey (:daleharvey) 2013-07-17 21:27:03 PDT
Created attachment 777579 [details] [diff] [review]
patch v4

Patch with nits addressed, Carrying r=joedrew, sr=mrbkap

Did another try run just to be sure nothing broke with the changes https://tbpl.mozilla.org/?tree=Try&rev=b323a4a68efa
Comment 28 Dale Harvey (:daleharvey) 2013-07-18 02:41:12 PDT
Merged to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/eeab86551b26
Comment 29 Dale Harvey (:daleharvey) 2013-07-18 02:42:42 PDT
Created attachment 777676 [details] [diff] [review]
b2g18 patch v1

Patch for b2g18 with nits addressed, Carrying r=joedrew, sr=mrbkap
Comment 30 Dale Harvey (:daleharvey) 2013-07-18 02:50:52 PDT
Merged to b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/b50d763029d2
Comment 31 Ed Morley [:emorley] 2013-07-18 04:19:37 PDT
Bugs aren't marked fixed until the inbound landing has merged to mozilla-central.

Also, this is failing reftests. Did you test locally/on try? :-)

https://tbpl.mozilla.org/php/getParsedLog.php?id=25427445&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25427332&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25427314&tree=Mozilla-Inbound

Backed out:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/76ecd043c64b
Comment 32 Ed Morley [:emorley] 2013-07-18 04:20:48 PDT
Just seen the try run in comment 27 - there were no reftests requested there.

Please also wait until the inbound landing is green before landing on b2g18 if possible.

Thank you :-)
Comment 33 Dale Harvey (:daleharvey) 2013-07-18 04:39:46 PDT
ah apologies, I did another try run

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

Which looked good, will figure out what went wrong there
Comment 34 Ed Morley [:emorley] 2013-07-18 04:43:54 PDT
b2g18 backout:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7f6f4bc1a621

(In reply to Dale Harvey (:daleharvey) from comment #33)
> ah apologies, I did another try run
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b323a4a68efa
> 
> Which looked good, will figure out what went wrong there

There are no reftests requested :-)

(try: -b o -p all -u mochitests -t none)

http://trychooser.pub.build.mozilla.org/ might help?
Comment 35 Dale Harvey (:daleharvey) 2013-07-18 09:01:19 PDT
ugh yeh I lost the binaries when applying the patch, will reup a new one to try, sorry for the churn
Comment 36 Ryan VanderMeulen [:RyanVM] 2013-07-18 09:04:24 PDT
(In reply to Dale Harvey (:daleharvey) from comment #28)
> Merged to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/eeab86551b26

1) B2G landings should landing on birch, not inbound.

(In reply to Dale Harvey (:daleharvey) from comment #30)
> Merged to b2g18:
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/b50d763029d2

2) Was there any reason this had to land on b2g18 prior to making it over to m-c, which is our normal procedure? In fact, that procedure is in place for exactly the reasons that played out here - avoiding multiple-branch bustage & backouts.
Comment 37 Dale Harvey (:daleharvey) 2013-07-18 09:15:49 PDT
No reasons, it was my mistake, I thought 1. DOM and non b2g specific bugs landed on inbound and 2. that inbound and b2g could happen in parallel.

Wont happen again (was my first push) apologies
Comment 38 Dale Harvey (:daleharvey) 2013-07-18 09:31:33 PDT
Created attachment 777878 [details] [diff] [review]
patch v5

New patch with binaries, carrying review, pushed to try with reftests enabled

ttps://tbpl.mozilla.org/?tree=Try&rev=c26f0706e493
Comment 39 Dale Harvey (:daleharvey) 2013-07-22 06:39:47 PDT
Try pass is green, My account is broken (filed a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=896447) so flagging checkin needed, will reupload b2g18 patch.
Comment 40 Dale Harvey (:daleharvey) 2013-07-22 14:54:58 PDT
Account got fixed, merged to birch

https://hg.mozilla.org/projects/birch/rev/1bbc7f3de64c
Comment 41 Ryan VanderMeulen [:RyanVM] 2013-07-22 19:01:47 PDT
https://hg.mozilla.org/mozilla-central/rev/1bbc7f3de64c
Comment 42 Dale Harvey (:daleharvey) 2013-07-22 19:56:38 PDT
Created attachment 779578 [details] [diff] [review]
891884.b2g18.patch

Updated b2g18 patch (with test files included), carrying review

try doesnt work with b2g18 but the patch builds locally and is very close to the central patch.

Will leave for sheriffs to uplift so I dont get it wrong again :)
Comment 43 Carsten Book [:Tomcat] 2013-07-23 02:15:25 PDT
we had to back this out https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4bf993a58a because causing reftest failures on android like https://tbpl.mozilla.org/php/getParsedLog.php?id=25598464&tree=Mozilla-Inbound


for the try run, the Try run was performed, but requested
"reftest" which doesn't catch android, since they are reftest-N (where N
is 1-4), so i guess this was the reason that this reftest failures didn't show up/
Comment 44 Ed Morley [:emorley] 2013-07-23 02:16:48 PDT
Birch also doesn't run android reftests (other than jsreftests), which is why the failures didn't show up before the merge from birch -> m-c.
Comment 45 Dale Harvey (:daleharvey) 2013-07-24 04:24:25 PDT
Created attachment 780328 [details] [diff] [review]
891884.patch

Added fuzzy-if statements for android reftests, error was due to read-back data being different than what we wrote (all other android reftest do similiar)

Pushed to try, single fail was a typo https://tbpl.mozilla.org/?tree=Try&rev=228690e2135b (other fail was an unrelated randomorange)
Comment 46 Dale Harvey (:daleharvey) 2013-07-24 04:25:34 PDT
https://hg.mozilla.org/projects/birch/rev/e831e823710d
Comment 47 Dale Harvey (:daleharvey) 2013-07-24 04:35:14 PDT
Created attachment 780335 [details] [diff] [review]
891884.b2g18.patch

b2g18 patch with above changes
Comment 48 Ryan VanderMeulen [:RyanVM] 2013-07-24 15:30:32 PDT
https://hg.mozilla.org/mozilla-central/rev/e831e823710d
Comment 49 Ryan VanderMeulen [:RyanVM] 2013-07-25 07:06:41 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/692a38cdde6b

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