Bug 37304 - WebKit does not support jpeg qulaity in canvas.toDataURL
: WebKit does not support jpeg qulaity in canvas.toDataURL
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 38494 38492 38495
  Show dependency treegraph
 
Reported: 2010-04-08 20:17 PDT by Leo Yang
Modified: 2010-05-31 18:24 PDT (History)
10 users (show)

See Also:


Attachments
Proposed patch (34.33 KB, patch)
2010-04-11 19:30 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Update patch to fix build issue on chromium (36.12 KB, patch)
2010-04-11 20:40 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Proposed patch - Fix a typo in the previous patch (36.14 KB, patch)
2010-04-11 22:32 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Proposed patch - try to satisfy build bot (36.14 KB, patch)
2010-04-12 00:03 PDT, Leo Yang
staikos: review-
Details | Formatted Diff | Diff
Fixed build issue for mac (36.55 KB, patch)
2010-04-14 22:06 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Make the patch upstream (36.67 KB, patch)
2010-04-25 21:23 PDT, Leo Yang
darin: review-
Details | Formatted Diff | Diff
Patch -- follow some comments from Dirk Schulze (36.42 KB, patch)
2010-04-28 00:04 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
patch (37.67 KB, patch)
2010-05-11 03:36 PDT, Leo Yang
staikos: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2010-04-08 20:17:56 PDT
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.
Comment 1 Leo Yang 2010-04-11 19:30:42 PDT
Created attachment 53127 [details]
Proposed patch

A proposed patch (including test cases) to the bug.
Comment 2 WebKit Review Bot 2010-04-11 19:42:32 PDT
Attachment 53127 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1694139
Comment 3 Leo Yang 2010-04-11 20:40:32 PDT
Created attachment 53143 [details]
Update patch to fix build issue on chromium
Comment 4 WebKit Review Bot 2010-04-11 20:53:00 PDT
Attachment 53143 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1674233
Comment 5 Leo Yang 2010-04-11 22:32:05 PDT
Created attachment 53148 [details]
Proposed patch - Fix a typo in the previous patch
Comment 6 Leo Yang 2010-04-12 00:03:44 PDT
Created attachment 53155 [details]
Proposed patch - try to satisfy build bot
Comment 7 George Staikos 2010-04-14 18:29:17 PDT
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&)
Comment 8 George Staikos 2010-04-14 19:47:23 PDT
Oliver, how do you think the ObjC binding should be done here?  Is there a concern about backwards compatibility?
Comment 9 Leo Yang 2010-04-14 22:06:57 PDT
Created attachment 53408 [details]
Fixed build issue for mac
Comment 10 Dirk Schulze 2010-04-23 23:33:46 PDT
Maybe this bug gets overlapped by bug 37117?
Comment 11 George Staikos 2010-04-24 06:04:20 PDT
I don't think so - platforms can do this 37117 their own way.
Comment 12 Sam Weinig 2010-04-24 12:50:23 PDT
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.
Comment 13 Sam Weinig 2010-04-24 12:56:17 PDT
(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.
Comment 14 George Staikos 2010-04-24 13:38:27 PDT
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.
Comment 15 Sam Weinig 2010-04-24 16:07:19 PDT
Yeah, that seems fine.
Comment 16 WebKit Commit Bot 2010-04-24 18:32:12 PDT
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
Comment 17 Leo Yang 2010-04-25 21:23:16 PDT
Created attachment 54253 [details]
Make the patch upstream
Comment 18 Darin Adler 2010-04-26 09:55:07 PDT
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)".
Comment 19 Leo Yang 2010-04-26 19:35:00 PDT
(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.
Comment 20 Leo Yang 2010-04-28 00:04:50 PDT
Created attachment 54529 [details]
Patch -- follow some comments from Dirk Schulze
Comment 21 Eric Seidel 2010-05-02 19:15:03 PDT
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.
Comment 22 Leo Yang 2010-05-11 03:36:11 PDT
Created attachment 55686 [details]
patch

1. Follow some review comments.

2. Describe the purpose of each test cases.
Comment 23 David Levin 2010-05-11 10:36:34 PDT
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?
Comment 24 Leo Yang 2010-05-11 19:01:13 PDT
(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.
Comment 25 George Staikos 2010-05-20 02:50:06 PDT
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.
Comment 26 Sam Weinig 2010-05-31 15:28:10 PDT
Comment on attachment 55686 [details]
patch

Since this won't apply cleanly anymore, I am going to land this instead of the bot.
Comment 27 Sam Weinig 2010-05-31 18:24:28 PDT
Landed in r60458 with some fixes.