Bug 38492 – CG implementation needed for compression quality in canvas.toDataURL

• Component: New Bugs
• Status: RESOLVED FIXED

[Matthew Delaney]

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.

[Matthew Delaney]

Created attachment 57583 [details]
Patch implements CG support for quality param in toDataURL canvas feature.

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Matthew Delaney]

Created attachment 57732 [details]
Patch

[WebKit Review Bot]

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

[Matthew Delaney]

Created attachment 57838 [details]
Patch

[Darin Adler]

Comment on attachment 57838 [details]
Patch

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

[Matthew Delaney]

Opps, good call. Uploading a new patch with that change now.

[Matthew Delaney]

Created attachment 57839 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 57839 [details]
Patch

Clearing flags on attachment: 57839

Committed r60675: <http://trac.webkit.org/changeset/60675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.