Bug 38492

Summary: CG implementation needed for compression quality in canvas.toDataURL
Product: WebKit Reporter: Matthew Delaney <mdelaney7>
Component: New BugsAssignee: Matthew Delaney <mdelaney7>
Severity: Enhancement CC: commit-queue, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on: 37304    
Bug Blocks:    
Description Flags
Patch implements CG support for quality param in toDataURL canvas feature.
Patch none

Description Matthew Delaney 2010-05-03 15:18:23 PDT
CG implementation is needed to incorporate compression quality for jpeg in canvas.toDataURL

HTML5 draft spec says implementation shall support quality parameter in canvas.toDataURL(type, quality, ...) when the type is image/jpeg. Support for this is needed by CG.
Comment 1 Matthew Delaney 2010-06-01 12:50:07 PDT
Created attachment 57583 [details]
Patch implements CG support for quality param in toDataURL canvas feature.
Comment 2 WebKit Review Bot 2010-06-01 14:19:20 PDT
Attachment 57583 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2010-06-01 15:15:43 PDT
Comment on attachment 57583 [details]
Patch implements CG support for quality param in toDataURL canvas feature.

Looks good.

> +	Fix for 38492. Added CG implementation to support quality parameter in canvas toDataURL().

This has a tab character.

The reference to the bug should be a bug URL not just the bug number.

> +
> +        * bindings/js/JSHTMLCanvasElementCustom.cpp:
> +        (WebCore::JSHTMLCanvasElement::toDataURL):
> +        * dom/CanvasSurface.h:
> +        (WebCore::CanvasSurface::toDataURL):
> +        * platform/graphics/ImageBuffer.h:
> +        * platform/graphics/cg/ImageBufferCG.cpp:
> +        (WebCore::jpegUTI):
> +        (WebCore::utiFromMIMEType):
> +        (WebCore::ImageBuffer::toDataURL):

Each file and function should have a short comment explaining the change.

> +#define DEFAULT_QUALITY -1.0

This should be a C++ constant, not a macro. Also, if the name is at the WebCore namespace level (top level), then it needs to be a specific-enough name to stand on its own.

    const double defaultImageCompressionQuality = -1;

I also am surprised that the default quality is -1. The old default was 1. Why the change? The change log should explain this. If the idea is that an explicit quality is optional, I think we should find an explicit way of doing that instead of defining a magic constant. We call that "in-band signaling" and usually try to avoid it. One way to do it would be to make the image quality argument be a "const double*" instead of just double and then we could use a 0 to mean "not specified".

If you do want to use in-band signaling, I think that NAN is probably a better value to mean "no quality specified". One advantage of that is that JavaScript code will automatically supply a NAN for any value that is not a number, without any custom bindings.

Did you add code to handle this default quality concept to the other platforms? Or do they already work as-is?

>          void putUnmultipliedImageData(ImageData*, const IntRect& sourceRect, const IntPoint& destPoint);
>          void putPremultipliedImageData(ImageData*, const IntRect& sourceRect, const IntPoint& destPoint);
> -        String toDataURL(const String& mimeType, double quality = 1.0) const;
> +
> +        String toDataURL(const String& mimeType, double quality = DEFAULT_QUALITY) const;

Why are you adding an extra blank line? I suggest omitting that.

> -    CGImageDestinationAddImage(destination.get(), image.get(), 0);
> +    RetainPtr<CFDictionaryRef> dict = 0;

I suggest naming this either "properties" or "imageProperties". The name "dict" isn't as good because it's an abbreviation, not a word, and also naming the data structure is not as useful as naming its purpose.

> +    if (CFEqual(uti.get(), jpegUTI()) && quality >= 0.0 && quality <= 1.0) {
> +
> +        // Apply the compression quality to the image destination.
> +        RetainPtr<CFNumberRef> compressionQuality(AdoptCF, CFNumberCreate(kCFAllocatorDefault, kCFNumberDoubleType, &quality));

You should get rid of that extra newline above.

> +        const void* keys[] = { kCGImageDestinationLossyCompressionQuality };
> +        const void* values[] = { compressionQuality.get() };
> +        dict.adoptCF(CFDictionaryCreate(0, keys, values, 1, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

I suggest you explicitly use the size "1" for these arrays instead of omitting the size. But also, if there's only a single key and value you don't actually need an array. This works:

    const void* key = kCGImageDestinationLossyCompressionQuality;
    const void* value = compressionQuality.get();
    imageProperties.adoptCF(CFDictionaryCreate(0, &key, &value, 1, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

I think those "&" before kCFTypeDictionaryKeyCallBacks and kCFTypeDictionaryValueCallBacks are optional and you should omit them.

I'm going to say review- because I think that you should deal with at least one of the bugs above.

Also, is there any test for this? A bug fix should add a test. A new test, or an existing test that was disabled on Mac before, or an existing test with new success results that before had failure results.
Comment 4 Matthew Delaney 2010-06-02 21:37:53 PDT
Created attachment 57732 [details]
Comment 5 WebKit Review Bot 2010-06-02 22:00:22 PDT
Attachment 57732 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3004009
Comment 6 Matthew Delaney 2010-06-03 17:34:19 PDT
Created attachment 57838 [details]
Comment 7 Darin Adler 2010-06-03 17:59:41 PDT
Comment on attachment 57838 [details]

In the Qt version you don't do a range check on the quality value passed in. But in the CG version you do. And you removed the checking from the custom binding for toDataURL. This means that the Qt version won't have range checking any more, but it did before.

review- because you should not regress Qt in this fashion. Otherwise, r=me
Comment 8 Matthew Delaney 2010-06-03 18:04:42 PDT
Opps, good call. Uploading a new patch with that change now.
Comment 9 Matthew Delaney 2010-06-03 18:09:02 PDT
Created attachment 57839 [details]
Comment 10 WebKit Commit Bot 2010-06-04 04:52:53 PDT
Comment on attachment 57839 [details]

Clearing flags on attachment: 57839

Committed r60675: <http://trac.webkit.org/changeset/60675>
Comment 11 WebKit Commit Bot 2010-06-04 04:52:59 PDT
All reviewed patches have been landed.  Closing bug.