WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
38492
CG implementation needed for compression quality in canvas.toDataURL
https://bugs.webkit.org/show_bug.cgi?id=38492
Summary
CG implementation needed for compression quality in canvas.toDataURL
Matthew Delaney
Reported
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.
Attachments
Patch implements CG support for quality param in toDataURL canvas feature.
(6.40 KB, patch)
2010-06-01 12:50 PDT
,
Matthew Delaney
no flags
Details
Formatted Diff
Diff
Patch
(14.10 KB, patch)
2010-06-02 21:37 PDT
,
Matthew Delaney
no flags
Details
Formatted Diff
Diff
Patch
(15.71 KB, patch)
2010-06-03 17:34 PDT
,
Matthew Delaney
no flags
Details
Formatted Diff
Diff
Patch
(15.75 KB, patch)
2010-06-03 18:09 PDT
,
Matthew Delaney
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Matthew Delaney
Comment 1
2010-06-01 12:50:07 PDT
Created
attachment 57583
[details]
Patch implements CG support for quality param in toDataURL canvas feature.
WebKit Review Bot
Comment 2
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.
Darin Adler
Comment 3
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.
Matthew Delaney
Comment 4
2010-06-02 21:37:53 PDT
Created
attachment 57732
[details]
Patch
WebKit Review Bot
Comment 5
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
Matthew Delaney
Comment 6
2010-06-03 17:34:19 PDT
Created
attachment 57838
[details]
Patch
Darin Adler
Comment 7
2010-06-03 17:59:41 PDT
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
Comment 8
2010-06-03 18:04:42 PDT
Opps, good call. Uploading a new patch with that change now.
Matthew Delaney
Comment 9
2010-06-03 18:09:02 PDT
Created
attachment 57839
[details]
Patch
WebKit Commit Bot
Comment 10
2010-06-04 04:52:53 PDT
Comment on
attachment 57839
[details]
Patch Clearing flags on attachment: 57839 Committed
r60675
: <
http://trac.webkit.org/changeset/60675
>
WebKit Commit Bot
Comment 11
2010-06-04 04:52:59 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug