sschallerl Reporter
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 FirePHP/0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3

According to the HTML5-spec, canvas.toDataURL method

    image/jpeg: The second argument, if it is a number in the range 0.0 to 1.0 inclusive, must be treated as the desired quality level. If it is not a number or is outside that range, the user agent must use its default value, as if the argument had been omitted.

This doesn't work, instead a security exception 1000 is thrown.

Also WhatWG-HTML5 spec
Other arguments must be ignored and must not cause the user agent to raise an exception.

WhatWG Canvas.toDataURL spec: 
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-canvas-todataurl

Corresponding test in Philip Taylors Canvas test suite:
http://philip.html5.org/tests/canvas/suite/tests/index.toDataURL.jpeg.quality.html

My own test case (works in opera!):
http://stefan.schallerl.com/canvas-todataurl-fail/

Reproducible: Always

Steps to Reproduce:
1. call mycanvas.toDataURL("image/jpeg", 0.5)

Actual Results:  
An exception is thrown:

[Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM
_SECURITY_ERR)"  location: "http://stefan.schallerl.com/canvas-todataurl-fail/ L
ine: 33"]

Expected Results:  
A base64 encoded image/jpeg data url with 50% quality

currently fails in chrome (returns only png) and safari (ignores 2nd paramter), but works as expected in opera 10.53/build 3374

also tried it in a current nightly build:
Version=3.7a5pre
BuildID=20100506040636

no difference

	sschallerl Reporter
	Comment 1 • 14 years ago

this bug is related (but not completely similar) to

Bug 401795 - Canvas toDataURL does not accept excess arguments
https://bugzilla.mozilla.org/show_bug.cgi?id=401795

	Davide C.
	Comment 2 • 14 years ago

Confirming that:

      mycanvas.toDataURL("image/jpeg", 0.5)

systematically FAILS in Firefox 3.6.12 on Mac OSX 10.6.4 too.

FYI, it works correctly on Chrome 7, which accepts and interprets the quality paramter as expected.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 3 • 14 years ago

Fun, this changed in the spec -- I think it was unspecified/underspecified before.  We accept a string such as "quality=50" to get 50% quality.

	Davide C.
	Comment 4 • 14 years ago

Hi, thanks for information. I gave it a try but apparently it's still not working:

Security error" code: "1000
[Break on this error] var canvasData = canvas.toDataURL("image/jpeg", "quality=50"); 

The only way it seems to work is by not specifying the second parameter at all, i.e. just ...toDataURL("image/jpeg");

Any suggestions? Thanks.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 5 • 14 years ago

Hmm, for some reason we apply extra security checks for the 2-arg form; this could be a leftover from when it was a nonstandard thing.  Regardless, this is valid bug, we should implement the API that's described in the spec.

	Davide C.
	Comment 6 • 14 years ago

Sounds good, thank you guys for fixing it as soon as possible.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 7 • 14 years ago

This is bug 401795, AFAICT.

	sschallerl Reporter
	Comment 8 • 14 years ago

@Ms2ger: the problem is not exactly the same. bug 401795 is about ignoring excess arguments. this bug is about the missing behaviour!

mycanvas.toDataURL("image/jpeg", 0.5)

according to the spec the second argument (in this case 0.5) is the compression factor:

image/jpeg: The second argument, if it is a number in the range 0.0 to 1.0 inclusive, must be treated as the desired quality level. If it is not a number or is outside that range, the user agent must use its default value, as if the argument had been omitted.

this bug addresses not only that excess parameters aren't ignored (and furthermore, in this case it shouldn't even be ignored), but that the second parameter doesn't affect the desired compression rate of the image.

	Davide C.
	Comment 9 • 14 years ago

sschallerl is right. This is a violation of the specifications (reported above).

I hope it can be fixed at least in Firefox 4 (the Beta version still has the same problem as of today). It's a pity that only Chrome is implementing it correctly.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 10 • 14 years ago

This won't make it into Firefox 4. We've been in feature freeze for months now. Maybe in Fx5, which will be released 3 months later, but someone needs to step up and write a patch for that to happen.

	sschallerl Reporter
	Comment 11 • 14 years ago

@davide: i'm not even sure if chrome is implementing it correctly. if you look at my test case, http://stefan.schallerl.com/canvas-todataurl-fail/ , chrome returns image/png even though we asked for image/jpeg.

	sschallerl Reporter
	Comment 12 • 14 years ago

afaik opera 11.01/win32 is the only browser that implements it correctly (at least for image/jpeg).

	Davide C.
	Comment 13 • 14 years ago

@sschalleri: 
1) I've just tried your link above and on MacOS 10.6.6/Chrome 8 it works correctly (not surprised as I've been using it since months already). In particular, here's the output I see:
-------------------------
dataURL1: 1 parameter:  canvas.toDataURL('image/jpeg') WIN
data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/4QBAR....  
dataURL2: 2 parameters: canvas.toDataURL('image/jpeg', 0.1)
data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/4QBAR ....
-------------------------
Which, if the test is reliable, shows that jpeg is returned correctly, with the second one being much smaller (and poorer quality) as expected because of the 0.1 factor.

2) regardless of who's implementing it right, I guess here the point is that Firefox is violating the specs and needs to fix it sooner or later.

	Brian O'Keefe [:bokeefe] Assignee
	Comment 14 • 13 years ago

This patch fixes ToDataURL by doing the following:
* From untrusted callers:
  - For the image/jpeg MIME type, parse the second parameter as a float
  - For any other MIME type, ignore the second parameter
  - For all MIME types, ignore the rest of the parameters (which also fixes bug 401795)
* From trusted callers:
  - Don't touch any of the "extra" parameters (essentially, do exactly what it did before)

I also enabled the mochitests, since they're passing locally. The linked testcase also works.

Requesting feedback? because this is my first patch, and the string manipulations look pretty bad, but I couldn't see a better way to do it.

	Nickolay_Ponomarev
	Comment 15 • 13 years ago

Comment on attachment 525210 [details] [diff] [review]
Add quality parameter for image/jpeg

Brian, check out https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch if you haven't seen it yet. Feedback/review requests without a requestee often fall through cracks, so I'm setting one for you.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 13 years ago

Thanks for the patch!

+  if (optional_argc >= 2 && !nsContentUtils::IsCallerTrustedForRead()) {

I think trusted callers should get standard Web APIs. So I suggest dropping this IsCallerTrustedForRead check, and simply assume that trusted callers are going to set the quality parameter per spec. I can't find any callers passing "quality=" in our tree, and if there are any in the wild they'll just get the default quality, they won't completely break.

+    nsContentUtils::ASCIIToLower(aType, type);
+
+    if (NS_LITERAL_STRING("image/jpeg").Equals(type)) {

You can call aType.LowerCaseEqualsASCII and avoid the "type" temporary.

+        nsAutoString qualityParameter(NS_LITERAL_STRING("quality="));
+        qualityParameter.AppendInt(qualityInteger);

You could use AppendLiteral and AppendInt directly to fixedParameters to avoid the qualityParameter temporary.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 17 • 13 years ago

Mid-aired, my comment:

This doesn't really match the spec... I'd make toDataURL take a jsval in
dom/interfaces/html/nsIDOMHTMLCanvasElement.idl and then do something like

nsAutoString params;
if (type.EqualsLiteral("image/jpeg") && JSVAL_IS_NUMBER(aParams)) {
  JSContext *cx = nsContentUtils::GetCurrentJSContext();
  double quality = 0.0;
  if (!JS_ValueToNumber(cx, aParams, &quality) {
    return NS_ERROR_FAILURE;
  }
  params.AppendLiteral("quality=");
  params.AppendInt(PRInt32(quality * 100.0));
} else if (nsContentUtils::IsCallerTrustedForRead() &&
JSVAL_IS_STRING(aParams)) {
  // See
http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBDatabase.cpp#604
}

in ToDataURLImpl, after the lowercasing.

	noel gordon
	Comment 18 • 13 years ago

Nice to see this happening.  Maybe round up the quality value?
 https://bugs.webkit.org/attachment.cgi?id=75000&action=prettypatch

Consider fixing ToDataURLImpl() to composite onto black per the spec.
 http://www.w3.org/Bugs/Public/show_bug.cgi?id=11431
 https://bugs.webkit.org/show_bug.cgi?id=51237

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 19 • 13 years ago

(In reply to comment #18)
> Nice to see this happening.  Maybe round up the quality value?
>  https://bugs.webkit.org/attachment.cgi?id=75000&action=prettypatch

Webkit rounds, it doesn't round up. We should probably do the same.

> Consider fixing ToDataURLImpl() to composite onto black per the spec.
>  http://www.w3.org/Bugs/Public/show_bug.cgi?id=11431
>  https://bugs.webkit.org/show_bug.cgi?id=51237

Not in this bug. File a separate bug if necessary.

	noel gordon
	Comment 20 • 13 years ago

> Webkit rounds, it doesn't round up. We should probably do the same.

OK good.  File, bug 650720 for the composite on black issue.

	Brian O'Keefe [:bokeefe] Assignee
	Comment 21 • 13 years ago

Thanks for all the feedback. I updated the patch to address the above comments:

(In reply to comment #17)
> This doesn't really match the spec... I'd make toDataURL take a jsval in
> dom/interfaces/html/nsIDOMHTMLCanvasElement.idl and then do something like
> [code]
> in ToDataURLImpl, after the lowercasing.

I changed the toDataURL declaration to take a jsval for the quality parameter. I also changed toDataURLImpl to take a jsval as well.  I left toDataURLAs alone, so it converts from the DOMString to a jsval now.
I moved the quality parameter conversions into toDataURLImpl accordingly.

(In reply to comment #16)
> I think trusted callers should get standard Web APIs.

That sounds reasonable enough. I changed it so that all callers go through the new quality conversion if the first parameter is a number, but trusted callers will still do the old processing if it's a string.

Lingering question: Should I leave the old call style around? I left it mostly to support toDataURLAs (which doesn't actually seem used anywhere). If I should take it out, should I:
* Leave toDataURLAs, which still takes the "option=value" syntax (and put the conversion back into toDataURL, or a helper function)
* Remove toDataURLAs, so there's no way to pass arbitrary options for any callers

(In reply to comment #19)
>> Nice to see this happening.  Maybe round up the quality value?
>>  https://bugs.webkit.org/attachment.cgi?id=75000&action=prettypatch
> Webkit rounds, it doesn't round up. We should probably do the same.

I added the rounding. If there's a function I should call (instead of adding 0.5), I'll update the patch to call that instead.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 13 years ago

+  JSString* opts = ::JS_NewUCStringCopyN(cx, reinterpret_cast<const jschar*> (
+                                      PromiseFlatString(aEncoderOptions).get()),
+                                         aEncoderOptions.Length());

Something has to destroy this string, but I'm not sure what does.

In fact I think we should just remove toDataURLAs, then that question goes away :-).

+      params.AppendInt((PRInt32)(quality * 100.0 + 0.5));

More readable to use NS_lroundf(quality * 100.0).

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 23 • 13 years ago

Comment on attachment 526692 [details] [diff] [review]
Add quality parameter for image/jpeg

>diff --git a/content/html/content/src/nsHTMLCanvasElement.cpp b/content/html/content/src/nsHTMLCanvasElement.cpp
>--- a/content/html/content/src/nsHTMLCanvasElement.cpp
>+++ b/content/html/content/src/nsHTMLCanvasElement.cpp
> NS_IMETHODIMP
> nsHTMLCanvasElement::ToDataURLAs(const nsAString& aMimeType,
>                                  const nsAString& aEncoderOptions,
>                                  nsAString& aDataURL)
> {
>-  return ToDataURLImpl(aMimeType, aEncoderOptions, aDataURL);
>+  JSContext* cx = nsContentUtils::GetCurrentJSContext();
>+  JSString* opts = ::JS_NewUCStringCopyN(cx, reinterpret_cast<const jschar*> (
>+                                      PromiseFlatString(aEncoderOptions).get()),
>+                                         aEncoderOptions.Length());

I wish this wasn't so ugly...

> nsresult
> nsHTMLCanvasElement::ToDataURLImpl(const nsAString& aMimeType,
>-                                   const nsAString& aEncoderOptions,
>+                                   const jsval& aEncoderOptions,
>                                    nsAString& aDataURL)
> {
>   bool fallbackToPNG = false;
> 
>   nsAutoString type;
>   nsContentUtils::ASCIIToLower(aMimeType, type);
> 
>+  JSContext* cx = nsContentUtils::GetCurrentJSContext();
>+  nsAutoString params;
>+  
>+  if (type.EqualsLiteral("image/jpeg") && JSVAL_IS_NUMBER(aEncoderOptions)) {
>+    // Quality parameter is only valid for the image/jpeg MIME type
>+
>+    double quality = 0.0;
>+
>+    // Quality must be between 0.0 and 1.0, inclusive

This comment wants to be a few lines below.

>+    if (!JS_ValueToNumber(cx, aEncoderOptions, &quality)) {
>+      return NS_ERROR_FAILURE;
>+    }

(Do we need to throw here?)

>+    if (quality >= 0 && quality <= 1.0) {
>+      params.AppendLiteral("quality=");
>+      params.AppendInt((PRInt32)(quality * 100.0 + 0.5));
>+    }
>+  } else if (nsContentUtils::IsCallerTrustedForRead() &&
>+             JSVAL_IS_STRING(aEncoderOptions)) {
>+    JSString* jsEncoderOptions = JS_ValueToString(cx, aEncoderOptions);
>+    
>+    size_t optionsLength;
>+    const jschar* optionsChars = ::JS_GetStringCharsZAndLength(cx,
>+                                                               jsEncoderOptions,
>+                                                               &optionsLength);
>+    
>+    params = nsDependentString(optionsChars, optionsLength);
>+  }
>+

More ugliness :(

Thanks for taking this on! It looks good to me, but I'd like bz to have a look and notice all the things I missed :)

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 13 years ago

Since Ms2ger nicely asked for my feedback...  I think the jsval thing is wrong (in particular, it makes it impossible to call this from C++), and in particular as a result of that approach the patch here does NOT implement what the spec says.  The spec says:

  The second argument, if it is a number in the range 0.0 to 1.0 inclusive, must
  be treated as the desired quality level. If it is not a number or is outside
  that range, the user agent must use its default value, as if the argument had
  been omitted.

That's somewhat pretty weird thing from a JS perspective, since it treats "0.5" and 0.5 differently, but the attached patch most definitely does NOT treat "0.5" and 0.5 differently.  So if we're trying to implement what the spec says the attached patch is wrong.

Before I say anything else about what I think the code should look like, I'd like to understand what behavior we're trying to implement.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 13 years ago

Er, wait.  I missed the JSVAL_IS_NUMBER check there.  So we do implement what the spec says, after all.

In which case, I think we should be using an nsIVariant here, not a jsval, and just check for the data type being VTYPE_INT32 or VTYPE_DOUBLE (and a relevant string type in the IsCallerTrustedForRead case).

That would incidentally fix the GC hazard the attached patch has in the IsCallerTrustedForRead case: jsEncoderOptions could be GCed, generally speaking, between the JS_GetStringCharsZAndLength call and the nsDependentString constructor running, at which point the nsDependentString would get garbage data.

Past that, I agree with roc regarding ToDataURLAs if it's unused.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 13 years ago

And I hate to bring up nsIVariant like that, by the way, but that's the only thing that we have other than jsval that can handle |any| arguments, and working correctly with jsval is _hard_ in some ways.  Harder than with nsIVariant.

	Brian O'Keefe [:bokeefe] Assignee
	Comment 27 • 13 years ago

> More readable to use NS_lroundf(quality * 100.0). 
I had a feeling there was an easier way to do that. 

> In fact I think we should just remove toDataURLAs 
Since roc and bz are both in favor of removing toDataURLAs, I did. 

> In which case, I think we should be using an nsIVariant here, ... 
> check for the data type being VTYPE_INT32 or VTYPE_DOUBLE 
That looks nicer than all the jsval stuff. I made the check cover any of the number-ish formats. 

I took out the IsCallerTrustedForRead part, since it was really only used by ToDataURLAs now that ToDataURL is matching the spec. There weren't any callers using the (string, string) form (correctly) other than in the tests. I fixed up that test so it's testing the right thing now. I changed the "incorrect" calls too - they were all passing the empty string as the second parameter, so I took it out entirely from those calls.

I also added a bunch of extra tests specifically testing the quality parameter.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 28 • 13 years ago

Comment on attachment 532427 [details] [diff] [review]
Add quality parameter to ToDataURL for image/jpeg

>+  nsAutoString params;
>+  
>+  PRUint16 vartype;
>+  nsresult rv = aEncoderOptions->GetDataType(&vartype);
>+  
>+  if (type.EqualsLiteral("image/jpeg") && NS_SUCCEEDED(rv) &&
>+      vartype <= nsIDataType::VTYPE_DOUBLE) {
>+    // Quality parameter is only valid for the image/jpeg MIME type
>+
>+    double quality;
>+    rv = aEncoderOptions->GetAsDouble(&quality);
>+
>+    // Quality must be between 0.0 and 1.0, inclusive
>+    if (NS_SUCCEEDED(rv) && quality >= 0.0 && quality <= 1.0) {
>+      params.AppendLiteral("quality=");
>+      params.AppendInt(NS_lround(quality * 100.0));
>+    }
>+  }

How about

// Quality parameter is only valid for the image/jpeg MIME type
if (type.EqualsLiteral("image/jpeg")) {
  PRUint16 vartype;
  if (NS_SUCCEEDED(aEncoderOptions->GetDataType(&vartype)) &&
      vartype <= nsIDataType::VTYPE_DOUBLE) {
    double quality;
    // Quality must be between 0.0 and 1.0, inclusive
    if (NS_SUCCEEDED(aEncoderOptions->GetAsDouble(&quality)) &&
        quality >= 0.0 && quality <= 1.0) {
      params.AppendLiteral("quality=");
      params.AppendInt(NS_lround(quality * 100.0));
    }
  }
}

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 29 • 13 years ago

Comment on attachment 532427 [details] [diff] [review]
Add quality parameter to ToDataURL for image/jpeg

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

this looks fine; what Ms2ger suggested is slightly better.

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 13 years ago

Comment on attachment 532427 [details] [diff] [review]
Add quality parameter to ToDataURL for image/jpeg

One nit: you made options an optional arg in the idl, so aEncoderOptions can be null, I would think.  But your code dereferences it in all cases.  I'm surprised this passed tests...

	Brian O'Keefe [:bokeefe] Assignee
	Comment 31 • 13 years ago

This version implements Ms2ger's suggestion for rearranging the parameter checks, and fixes bz's nit.

> One nit: you made options an optional arg in the idl, so aEncoderOptions can be
> null, I would think.  But your code dereferences it in all cases.  I'm
> surprised this passed tests...

I see what you mean.  I was assuming it would only get called with the special NullVariant class, not an actual null.  But even if that's the case, checking for null can't hurt.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 13 years ago

Comment on attachment 534937 [details] [diff] [review]
Add quality parameter to ToDataURL for image/jpeg

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

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 13 years ago

Comment on attachment 534937 [details] [diff] [review]
Add quality parameter to ToDataURL for image/jpeg

r=me

	Dão Gottwald [:dao]
	Comment 34 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/769b00d49ce7

	Eric Shepherd [:sheppy]
	Comment 36 • 13 years ago

louisremi updated the documentation a while ago:

https://developer.mozilla.org/en/DOM/HTMLCanvasElement

	Simona Badau, Desktop QA
	Comment 37 • 13 years ago

Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Verified issue on Win XP, Win 7, Ubuntu 11.04 and Mac OS X 10.6 using the test cases attached in the Description.

Setting resolution to VERIFIED FIXED.

	Kathleen :Brade
	Comment 38 • 13 years ago

This bug caused a loss of functionality for add-ons.  Filed as bug 723931.