Bug 37304 – WebKit does not support jpeg qulaity in canvas.toDataURL

• Component: New Bugs
• Status: RESOLVED FIXED

[Leo Yang]

HTML5 draft spec says implementation shall support quality parameter in canvas.toDataURL(type, quality, ...) when the type is image/jpeg. WebKit does not support it now.

[Leo Yang]

Created attachment 53127 [details]
Proposed patch

A proposed patch (including test cases) to the bug.

[WebKit Review Bot]

Attachment 53127 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1694139

[Leo Yang]

Created attachment 53143 [details]
Update patch to fix build issue on chromium

[WebKit Review Bot]

Attachment 53143 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1674233

[Leo Yang]

Created attachment 53148 [details]
Proposed patch - Fix a typo in the previous patch

[Leo Yang]

Created attachment 53155 [details]
Proposed patch - try to satisfy build bot

[George Staikos]

Comment on attachment 53155 [details]
Proposed patch - try to satisfy build bot

Generally looks good, but fails on mac:
WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMHTMLCanvasElement.mm: In function 'NSString* -[DOMHTMLCanvasElement toDataURL:](DOMHTMLCanvasElement*, objc_selector*, NSString*)':
WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMHTMLCanvasElement.mm:73: error: no matching function for call to 'WebCore::HTMLCanvasElement::toDataURL(NSString*&, WebCore::ExceptionCode&)'
WebKit/WebCore/dom/CanvasSurface.h:62: note: candidates are: WebCore::String WebCore::CanvasSurface::toDataURL(const WebCore::String&, double, WebCore::ExceptionCode&)

[George Staikos]

Oliver, how do you think the ObjC binding should be done here?  Is there a concern about backwards compatibility?

[Leo Yang]

Created attachment 53408 [details]
Fixed build issue for mac

[Dirk Schulze]

Maybe this bug gets overlapped by bug 37117?

[George Staikos]

I don't think so - platforms can do this 37117 their own way.

[Sam Weinig]

Comment on attachment 53408 [details]
Fixed build issue for mac

This looks good and I would like to get this landed.  My one suggestion would be to put the test in platform independent location (if possible) and just skip it on platforms where this is not implemented (and file bugs for those platforms to implement the feature). You may also want to test that the quality argument does not effect non-jpep MIME types (if possible).

r=me.

[Sam Weinig]

(In reply to comment #12)
> (From update of attachment 53408 [details])
> This looks good and I would like to get this landed.  My one suggestion would
> be to put the test in platform independent location (if possible) and just skip
> it on platforms where this is not implemented (and file bugs for those
> platforms to implement the feature). You may also want to test that the quality
> argument does not effect non-jpep MIME types (if possible).
> 
> r=me.

One additional thing that occurred to me was that we may want an illegal value for the default value instead of 1.0.  For CG, for instance, the default compression for JPEG would not be 1.0, so we would want to know if no quality argument was passed.  Does that seem reasonable.  I suggest a value of -1.0 as the default.

[George Staikos]

Should it be committed and the follow up with the additional comments?  Seems reasonable...  If so then I'll add it to the commit queue.

[Sam Weinig]

Yeah, that seems fine.

[WebKit Commit Bot]

Comment on attachment 53408 [details]
Fixed build issue for mac

Rejecting patch 53408 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Sam Weinig', u'--force']" exit_code: 1
Last 500 characters of output:
platform/qt/fast/canvas/toDataURL-jpeg-quality-basic.html
patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg-quality-notnumber-expected.txt
patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg-quality-notnumber.html
patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg-quality-outsiderange-expected.txt
patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg-quality-outsiderange.html
patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg.js

Full output: http://webkit-commit-queue.appspot.com/results/1828089

[Leo Yang]

Created attachment 54253 [details]
Make the patch upstream

[Darin Adler]

Comment on attachment 54253 [details]
Make the patch upstream

> +#include <wtf/MathExtras.h>

No need for MathExtras.h (see below).

> +    double quality = 1.0;
> +    if (args.size() > 1) {
> +        JSValue v = args.at(1);
> +        if (v.isNumber())
> +            quality = v.toNumber(exec);
> +        if (quality > 1.0 || quality < 0.0 || !isfinite(quality))
> +            quality = 1.0;
> +    }

There is a simpler, cleaner way to write this.

    double quality = args.at(1).toNumber(exec);
    if (!(quality >= 0 && quality < 1))
        quality = 1;

You don't need all those if statements. This correctly handles NAN, infinity, missing arguments, and the like. A NAN will return false from any comparison.

> +#include "PlatformString.h"
>  
>  #include <wtf/OwnPtr.h>
>  #include <wtf/Noncopyable.h>
> @@ -41,7 +43,6 @@ class FloatSize;
>  class GraphicsContext;
>  class ImageBuffer;
>  class IntPoint;
> -class String;

There is no need to include the header here in CanvasSurface.h. You can pass a const String& through to another const String& without including the definition of the String class.

> -        DOMString toDataURL(in [ConvertUndefinedOrNullToNullString] DOMString type)
> +        // HTML5 draft spec: DOMString toDataURL(DOMString type, ...);
> +        [Custom] DOMString toDataURL(in [ConvertUndefinedOrNullToNullString] DOMString type)
>              raises(DOMException);

I don't understand this comment. You should leave it out unless it has something specific to say.

> -    if (!m_data.m_pixmap.save(&buffer, mimeType.substring(sizeof "image").utf8().data()))
> +    if (!m_data.m_pixmap.save(&buffer, mimeType.substring(sizeof "image").utf8().data(), quality*100+0.5))

This is not the correct way to scale a [0,1] floating point value to a [0,100] integer value. The correct way is "quality * nextafter(100, 0)".

[Leo Yang]

(In reply to comment #18)
> (From update of attachment 54253 [details])
> > +#include <wtf/MathExtras.h>
> 
> No need for MathExtras.h (see below).
> 
> > +    double quality = 1.0;
> > +    if (args.size() > 1) {
> > +        JSValue v = args.at(1);
> > +        if (v.isNumber())
> > +            quality = v.toNumber(exec);
> > +        if (quality > 1.0 || quality < 0.0 || !isfinite(quality))
> > +            quality = 1.0;
> > +    }
> 
> There is a simpler, cleaner way to write this.
> 
>     double quality = args.at(1).toNumber(exec);
>     if (!(quality >= 0 && quality < 1))
>         quality = 1;
> 
> You don't need all those if statements. This correctly handles NAN, infinity,
> missing arguments, and the like. A NAN will return false from any comparison.
> 
HTML 5 draft spec reuires quality must be with number type. So the simpler way can not handle one case in which quality is a numeric like string (e.g. String('0.01'), per the spec, it should be converted to 100% quality)

> > +#include "PlatformString.h"
> >  
> >  #include <wtf/OwnPtr.h>
> >  #include <wtf/Noncopyable.h>
> > @@ -41,7 +43,6 @@ class FloatSize;
> >  class GraphicsContext;
> >  class ImageBuffer;
> >  class IntPoint;
> > -class String;
> 
> There is no need to include the header here in CanvasSurface.h. You can pass a
> const String& through to another const String& without including the definition
> of the String class.
I had thought it need not to include PlatformString.h, but CanvasSurface.cpp doesn't compile by gcc-4.3.3-Ubuntu. I think the reson is that inline function return non-reference type. So If I inlcude PlatformString.h in CavasSurface.cpp before line #include "CanvasSurface.h", it's a bit violation of code style.

> 
> > -        DOMString toDataURL(in [ConvertUndefinedOrNullToNullString] DOMString type)
> > +        // HTML5 draft spec: DOMString toDataURL(DOMString type, ...);
> > +        [Custom] DOMString toDataURL(in [ConvertUndefinedOrNullToNullString] DOMString type)
> >              raises(DOMException);
> 
> I don't understand this comment. You should leave it out unless it has
> something specific to say.
> 
OK. Will remove it

> > -    if (!m_data.m_pixmap.save(&buffer, mimeType.substring(sizeof "image").utf8().data()))
> > +    if (!m_data.m_pixmap.save(&buffer, mimeType.substring(sizeof "image").utf8().data(), quality*100+0.5))
> 
> This is not the correct way to scale a [0,1] floating point value to a [0,100]
> integer value. The correct way is "quality * nextafter(100, 0)".
quality * nextafter(100, 0) is not bad. But quality * 100 + 0.5 should be faster than the previous one, and the two ways has same precsion. Regard as correctness, if quality = 0.9995, the result is 99 if it adopts the previous way, but I think 100 is more resonable.

[Leo Yang]

Created attachment 54529 [details]
Patch -- follow some comments from Dirk Schulze

[Eric Seidel (no email)]

Comment on attachment 53408 [details]
Fixed build issue for mac

Cleared Sam Weinig's review+ from obsolete attachment 53408 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Leo Yang]

Created attachment 55686 [details]
patch

1. Follow some review comments.

2. Describe the purpose of each test cases.

[David Levin]

Could this be done without custom bindings?

Just take an optional double and then do the equivalent of

  if (!(0.0 <= quality && quality <= 1.0))
      quality = 1.0;

in CanvasSurface::toDataURL?

[Leo Yang]

(In reply to comment #23)
> Could this be done without custom bindings?
> 
> Just take an optional double and then do the equivalent of
> 
>   if (!(0.0 <= quality && quality <= 1.0))
>       quality = 1.0;
> 
> in CanvasSurface::toDataURL?
It can't. Just as I said in comment #19, we need to check the real type of quality parameter.

[George Staikos]

Comment on attachment 55686 [details]
patch

Looks like everything has been addressed and it is well tested.  Also appears to be properly ported everywhere. Looks good to me.

[Sam Weinig]

Comment on attachment 55686 [details]
patch

Since this won't apply cleanly anymore, I am going to land this instead of the bot.

[Sam Weinig]

Landed in r60458 with some fixes.