Bug 37304 - WebKit does not support jpeg qulaity in canvas.toDataURL
: WebKit does not support jpeg qulaity in canvas.toDataURL
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 38492 38494 38495
  Show dependency treegraph
 
Reported: 2010-04-08 20:17 PST by
Modified: 2010-05-31 18:24 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-08 20:17:56 PST
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 From 2010-04-11 19:30:42 PST -------
Created an attachment (id=53127) [details]
Proposed patch

A proposed patch (including test cases) to the bug.
------- Comment #2 From 2010-04-11 19:42:32 PST -------
Attachment 53127 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1694139
------- Comment #3 From 2010-04-11 20:40:32 PST -------
Created an attachment (id=53143) [details]
Update patch to fix build issue on chromium
------- Comment #4 From 2010-04-11 20:53:00 PST -------
Attachment 53143 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1674233
------- Comment #5 From 2010-04-11 22:32:05 PST -------
Created an attachment (id=53148) [details]
Proposed patch - Fix a typo in the previous patch
------- Comment #6 From 2010-04-12 00:03:44 PST -------
Created an attachment (id=53155) [details]
Proposed patch - try to satisfy build bot
------- Comment #7 From 2010-04-14 18:29:17 PST -------
(From update of attachment 53155 [details])
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 From 2010-04-14 19:47:23 PST -------
Oliver, how do you think the ObjC binding should be done here?  Is there a concern about backwards compatibility?
------- Comment #9 From 2010-04-14 22:06:57 PST -------
Created an attachment (id=53408) [details]
Fixed build issue for mac
------- Comment #10 From 2010-04-23 23:33:46 PST -------
Maybe this bug gets overlapped by bug 37117?
------- Comment #11 From 2010-04-24 06:04:20 PST -------
I don't think so - platforms can do this 37117 their own way.
------- Comment #12 From 2010-04-24 12:50:23 PST -------
(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.
------- Comment #13 From 2010-04-24 12:56:17 PST -------
(In reply to comment #12)
> (From update of attachment 53408 [details] [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 From 2010-04-24 13:38:27 PST -------
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 From 2010-04-24 16:07:19 PST -------
Yeah, that seems fine.
------- Comment #16 From 2010-04-24 18:32:12 PST -------
(From update of attachment 53408 [details])
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 From 2010-04-25 21:23:16 PST -------
Created an attachment (id=54253) [details]
Make the patch upstream
------- Comment #18 From 2010-04-26 09:55:07 PST -------
(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.

> +#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 From 2010-04-26 19:35:00 PST -------
(In reply to comment #18)
> (From update of attachment 54253 [details] [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 From 2010-04-28 00:04:50 PST -------
Created an attachment (id=54529) [details]
Patch -- follow some comments from Dirk Schulze
------- Comment #21 From 2010-05-02 19:15:03 PST -------
(From update of attachment 53408 [details])
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 From 2010-05-11 03:36:11 PST -------
Created an attachment (id=55686) [details]
patch

1. Follow some review comments.

2. Describe the purpose of each test cases.
------- Comment #23 From 2010-05-11 10:36:34 PST -------
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 From 2010-05-11 19:01:13 PST -------
(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 From 2010-05-20 02:50:06 PST -------
(From update of attachment 55686 [details])
Looks like everything has been addressed and it is well tested.  Also appears to be properly ported everywhere. Looks good to me.
------- Comment #26 From 2010-05-31 15:28:10 PST -------
(From update of attachment 55686 [details])
Since this won't apply cleanly anymore, I am going to land this instead of the bot.
------- Comment #27 From 2010-05-31 18:24:28 PST -------
Landed in r60458 with some fixes.