toBlob should support the quality parameter as toDataURL does

RESOLVED FIXED in Firefox 25, Firefox OS v1.1hd

Status

()

Core
Canvas: 2D
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: julienw, Assigned: daleharvey)

Tracking

({dev-doc-complete})

22 Branch
mozilla25
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(3 attachments, 12 obsolete attachments)

101.59 KB, text/html
Details
256.44 KB, patch
daleharvey
: review+
daleharvey
: superreview+
Details | Diff | Splinter Review
253.36 KB, patch
daleharvey
: review+
daleharvey
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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+.
(Reporter)

Updated

4 years ago
Blocks: 889902

Comment 1

4 years ago
How badly is this needed?  Is there a workaround? Adding another function argument at this point brings some risk.
Flags: needinfo?(felash)
(Reporter)

Comment 2

4 years ago
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 :)
Flags: needinfo?(felash)
(Reporter)

Updated

4 years ago
Duplicate of this bug: 822190
(Reporter)

Comment 4

4 years ago
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.
(Reporter)

Updated

4 years ago
Assignee: nobody → felash
(Reporter)

Comment 5

4 years ago
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.
(Reporter)

Comment 6

4 years ago
Created attachment 774511 [details]
example web page to demonstrate this works

the first one had a bug. qualities are < 1 :)
Attachment #774502 - Attachment is obsolete: true
(Reporter)

Comment 7

4 years ago
Created attachment 774515 [details] [diff] [review]
wip patch v2

asking feedback because my reftests don't work yet
Attachment #774093 - Attachment is obsolete: true
Attachment #774515 - Flags: feedback?(joe)
(Reporter)

Comment 8

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

reworking the reftests, I had some explanation from :padenot.
Attachment #774515 - Flags: feedback?(joe)
(Reporter)

Comment 9

4 years ago
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...
Attachment #774515 - Attachment is obsolete: true
(Reporter)

Comment 10

4 years ago
Created attachment 774672 [details] [diff] [review]
patch v1

try is https://tbpl.mozilla.org/?tree=Try&rev=f2138e09e6b8
Attachment #774642 - Attachment is obsolete: true
Attachment #774672 - Flags: review?(joe)
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.
Attachment #774672 - Flags: superreview?(mrbkap)
Attachment #774672 - Flags: review?(joe)
Attachment #774672 - Flags: review+
(Reporter)

Comment 12

4 years ago
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
Attachment #774672 - Attachment is obsolete: true
Attachment #774672 - Flags: superreview?(mrbkap)
Attachment #774710 - Flags: superreview?(mrbkap)
Attachment #774710 - Flags: review+
(Reporter)

Comment 13

4 years ago
will add a new patch with r= sr= once everything is cleared.
(Reporter)

Comment 14

4 years ago
(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 ?
Yes, sure!
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 :)
Attachment #774710 - Flags: superreview?(mrbkap)
(Reporter)

Comment 17

4 years ago
(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
(Reporter)

Comment 18

4 years ago
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
Attachment #774710 - Attachment is obsolete: true
Attachment #775123 - Flags: superreview?(mrbkap)
Attachment #775123 - Flags: review+
(Reporter)

Comment 19

4 years ago
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
Attachment #775136 - Flags: review?(joe)
(Reporter)

Comment 20

4 years ago
my previous try had some leftover patches, here is the "real" b2g18 try: https://tbpl.mozilla.org/?tree=Try&rev=fe71b198ba1e
(Reporter)

Comment 21

4 years ago
Dale was ok with finishing this.

Dale, I'd really like to have this in b2g18 :)
Assignee: felash → dale
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?
Attachment #775136 - Flags: review?(joe) → review+
er, m-c patch.
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.
Attachment #775123 - Flags: superreview?(mrbkap) → superreview+
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
Triage - leo+ as required by 889765, will speak to schung with regards to comment 25 and alter nomination if the status changes.
blocking-b2g: leo? → leo+
(Assignee)

Comment 27

4 years ago
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
Attachment #775123 - Attachment is obsolete: true
Attachment #777579 - Flags: superreview+
Attachment #777579 - Flags: review+
(Assignee)

Comment 28

4 years ago
Merged to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/eeab86551b26
(Assignee)

Comment 29

4 years ago
Created attachment 777676 [details] [diff] [review]
b2g18 patch v1

Patch for b2g18 with nits addressed, Carrying r=joedrew, sr=mrbkap
Attachment #775136 - Attachment is obsolete: true
Attachment #777676 - Flags: review+
(Assignee)

Comment 30

4 years ago
Merged to b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/b50d763029d2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 :-)
(Assignee)

Comment 33

4 years ago
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
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?
(Assignee)

Comment 35

4 years ago
ugh yeh I lost the binaries when applying the patch, will reup a new one to try, sorry for the churn
(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.
(Assignee)

Comment 37

4 years ago
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
(Assignee)

Comment 38

4 years ago
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
Attachment #777579 - Attachment is obsolete: true
Attachment #777878 - Flags: superreview+
Attachment #777878 - Flags: review+
(Assignee)

Comment 39

4 years ago
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.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 40

4 years ago
Account got fixed, merged to birch

https://hg.mozilla.org/projects/birch/rev/1bbc7f3de64c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1bbc7f3de64c
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 42

4 years ago
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 :)
Attachment #777676 - Attachment is obsolete: true
Attachment #779578 - Flags: review+
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/
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 45

4 years ago
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)
Attachment #777878 - Attachment is obsolete: true
Attachment #780328 - Flags: superreview+
Attachment #780328 - Flags: review+
(Assignee)

Comment 46

4 years ago
https://hg.mozilla.org/projects/birch/rev/e831e823710d
(Assignee)

Comment 47

4 years ago
Created attachment 780335 [details] [diff] [review]
891884.b2g18.patch

b2g18 patch with above changes
Attachment #779578 - Attachment is obsolete: true
Attachment #780335 - Flags: superreview+
Attachment #780335 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e831e823710d
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
https://hg.mozilla.org/releases/mozilla-b2g18/rev/692a38cdde6b
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
status-firefox25: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/692a38cdde6b
status-b2g-v1.1hd: affected → fixed
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.