891884 - toBlob should support the quality parameter as toDataURL does

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Julien Wajsberg [:julienw]]

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]

How badly is this needed?  Is there a workaround? Adding another function argument at this point brings some risk.

Comment 2

[Julien Wajsberg [:julienw]]

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 4

[Julien Wajsberg [:julienw]]

Attachment: 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]]

Attachment: 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]]

Attachment: example web page to demonstrate this works

the first one had a bug. qualities are < 1 :)

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: wip patch v2

asking feedback because my reftests don't work yet

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 774515 [details] [diff] [review]
wip patch v2

reworking the reftests, I had some explanation from :padenot.

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: 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]]

Attachment: patch v1

try is https://tbpl.mozilla.org/?tree=Try&rev=f2138e09e6b8

Comment 11

[Joe Drew (not getting mail)]

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]]

Attachment: 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]]

will add a new patch with r= sr= once everything is cleared.

Comment 14

[Julien Wajsberg [:julienw]]

(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)]

Yes, sure!

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

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]]

(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]]

Attachment: 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]]

Attachment: 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]]

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]]

Dale was ok with finishing this.

Dale, I'd really like to have this in b2g18 :)

Comment 22

[Joe Drew (not getting mail)]

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)]

er, m-c patch.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

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]]

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]]

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)]

Attachment: 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)]

Merged to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/eeab86551b26

Comment 29

[Dale Harvey (:daleharvey)]

Attachment: b2g18 patch v1

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

Comment 30

[Dale Harvey (:daleharvey)]

Merged to b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/b50d763029d2

Comment 31

[Ed Morley [:emorley]]

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]]

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)]

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]]

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)]

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]]

(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)]

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)]

Attachment: 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)]

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)]

Account got fixed, merged to birch

https://hg.mozilla.org/projects/birch/rev/1bbc7f3de64c

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1bbc7f3de64c

Comment 42

[Dale Harvey (:daleharvey)]

Attachment: 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]]

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]]

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)]

Attachment: 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)]

https://hg.mozilla.org/projects/birch/rev/e831e823710d

Comment 47

[Dale Harvey (:daleharvey)]

Attachment: 891884.b2g18.patch

b2g18 patch with above changes

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e831e823710d

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/692a38cdde6b

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/692a38cdde6b

Comment 51

[Jean-Yves Perrier [:teoli]]

I added a note in :
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25