Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1302423004:
Support lossy and lossless <canvas>.toDataURL for webp (Closed)
|
Created:
5 years, 3 months ago by Noel Gordon Modified:
4 years, 7 months ago CC:
blink-reviews, Rik, danakj, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, jzern, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, vikasa Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSupport lossy and lossless <canvas>.toDataURL for webp
Deem "image/webp" to be an image format that supports alpha for the
purposes of the HTML <canvas> spec. We no longer need to implement
the composite-over-black requirement of the <canvas> spec since the
image importer now includes the alpha channel always. If the image
has no alpha, the libwebp encoder will notice and remove the alpha
channel from the output (so the encoded image size is minimized).
Add "image/webp.ll" (lossless webp) support. The toDataURL API user
may optionally use the toDataURL quality argument to control encode
speed. The default quality balances encoding speed with compression
rate similar to the toDataURL PNG encoder.
Note "image/webp.ll" is not a mimeType: it is an encoding selector.
The toDataURL return result must contain the mimeType "image/webp".
Layout tests added in this CL test this requirement.
For "image/webp" at maximum quality, maintain sharpness (to match
the toDataURL JPEG encoder) by using a better quality encoder. This
is an internal detail of the user agent, one that might be changed
to use libwebp::WebPPictureSmartARGBToYUVA() in future.
Change in behavior: add new tests, update existing tests.
fast/canvas/canvas-toBlob-webp-lossless.html
fast/canvas/canvas-toDataURL-webp-alpha.html
fast/canvas/canvas-toDataURL-webp-lossless.html
fast/canvas/canvas-toDataURL-webp-maximum-quality.html
fast/canvas/canvas-toDataURL-webp.html
fast/canvas/toDataURL-supportedTypes.html
This change include ideas and contributions from skal@, urvang@ and
acterhd@
BUG=523098
Patch Set 1 #
Total comments: 11
Patch Set 2 : #Patch Set 3 : Add new tests. #Patch Set 4 : Add new tests, set expectations. #Patch Set 5 : #
Total comments: 2
Patch Set 6 : Patch for landing. #Messages
Total messages: 77 (13 generated)
https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:137: // terms of encoding speed and quality. _and_ filesize.
https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:152: bool WEBPImageEncoder::encode(const SkBitmap& bitmap, int quality, Vector<unsigned char>* output, WEBPImageEncoder::Mode mode) FIXME: since SkBitmap is being removed from chrome, we should remove this encode() variant. That means only the ImageDataBuffer encode variant is required, and they ImageDataBuffer always contains unpremultiplied RGBA data. That fact could be used to greatly simplify this encoder, and the other encoders (PNG, JPEG).
https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:138: config.quality = 75; FIXME: use WEBPImageEncoder::DefaultCompressionQuality here?
> FIXME: since SkBitmap is being removed from chrome, ...
Quick test: comment out that encode variant in the {.h,.cpp} and recompile
chrome, seems to work.
https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:152: bool WEBPImageEncoder::encode(const SkBitmap& bitmap, int quality, Vector<unsigned char>* output, WEBPImageEncoder::Mode mode) On 2015/08/26 23:43:14, noel gordon wrote: > FIXME: since SkBitmap is being removed from chrome, we should remove this > encode() variant. https://code.google.com/p/chromium/issues/detail?id=449197
On 2015/08/27 12:43:01, noel gordon wrote: > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:152: bool > WEBPImageEncoder::encode(const SkBitmap& bitmap, int quality, Vector<unsigned > char>* output, WEBPImageEncoder::Mode mode) > On 2015/08/26 23:43:14, noel gordon wrote: > > FIXME: since SkBitmap is being removed from chrome, we should remove this > > encode() variant. > > https://code.google.com/p/chromium/issues/detail?id=449197 Pick up from my source code. My code more valid in compatibility plane. Also used correct names as "+lossless" and "+alpha". I also used your idea with lossless compression. But I don't know what will is better. https://codereview.chromium.org/1315803002/
acterhd@gmail.com changed reviewers: + acterhd@gmail.com
I think your idea, based in my, are not good. I'm recently updated code. Saved backward compatibility. https://codereview.chromium.org/1302423004/diff/1/Source/platform/MIMETypeReg... File Source/platform/MIMETypeRegistry.cpp (right): https://codereview.chromium.org/1302423004/diff/1/Source/platform/MIMETypeReg... Source/platform/MIMETypeRegistry.cpp:90: if (equalIgnoringCase(mimeType, "image/webp") || equalIgnoringCase(mimeType, "image/webplossless")) Better use "image/webp+lossless". https://codereview.chromium.org/1302423004/diff/1/Source/platform/graphics/Im... File Source/platform/graphics/ImageBuffer.cpp (right): https://codereview.chromium.org/1302423004/diff/1/Source/platform/graphics/Im... Source/platform/graphics/ImageBuffer.cpp:384: I'm used ternary operator for replace mimes. https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:115: I'm used backward compatibility with lossy without alpha channel. https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:131: } else if (quality > 99) { Bad idea, particularly for video (WebM) encoders with quality = 100.
urvang@chromium.org changed reviewers: + urvang@chromium.org
https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:129: config.method = (quality * 6) / 100; Large method values are pretty impractical: e.g. q=100 & m=6 is simply impractical for canvas.toDataURL use case. I would keep method from 0 to 3. And FYI: quality=100 & m=3 is still pretty good compression wise. (even q=10, m = 0 in fact gives decent compression). https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:140: config.lossless = 1; We have a separate setting for lossless, so quality > 99 should NOT mean lossless. We should use losssy with WebPPictureSmartARGBToYUVA() instead, which would be reasonably sharp and yet smaller in size and a bit faster than lossless. In fact, Pascal suggested using it for all high qualities for lossy (q>=90 say) https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:152: bool WEBPImageEncoder::encode(const SkBitmap& bitmap, int quality, Vector<unsigned char>* output, WEBPImageEncoder::Mode mode) On 2015/08/27 12:43:00, noel gordon wrote: > On 2015/08/26 23:43:14, noel gordon wrote: > > FIXME: since SkBitmap is being removed from chrome, we should remove this > > encode() variant. > > https://code.google.com/p/chromium/issues/detail?id=449197 Nice! That explains why I never hit this encoder in tests. Let's remove this encode() variant first then!
On 2015/08/27 17:18:24, urvang wrote: > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:129: config.method = > (quality * 6) / 100; > Large method values are pretty impractical: e.g. q=100 & m=6 is simply > impractical for canvas.toDataURL use case. > I would keep method from 0 to 3. > And FYI: quality=100 & m=3 is still pretty good compression wise. > (even q=10, m = 0 in fact gives decent compression). > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:140: config.lossless = > 1; > We have a separate setting for lossless, so quality > 99 should NOT mean > lossless. > We should use losssy with WebPPictureSmartARGBToYUVA() instead, which would be > reasonably sharp and yet smaller in size and a bit faster than lossless. > In fact, Pascal suggested using it for all high qualities for lossy (q>=90 say) > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:152: bool > WEBPImageEncoder::encode(const SkBitmap& bitmap, int quality, Vector<unsigned > char>* output, WEBPImageEncoder::Mode mode) > On 2015/08/27 12:43:00, noel gordon wrote: > > On 2015/08/26 23:43:14, noel gordon wrote: > > > FIXME: since SkBitmap is being removed from chrome, we should remove this > > > encode() variant. > > > > https://code.google.com/p/chromium/issues/detail?id=449197 > > Nice! That explains why I never hit this encoder in tests. > Let's remove this encode() variant first then! urvang, review my patch please.
Okay, noel. Just copy-paste my code to your patch, and I'm delete my variant.
On 2015/08/28 11:05:10, acterhd wrote: > Okay, noel. Just copy-paste my code to your patch, and I'm delete my variant. No, keep working on your variant, I'll get popcorn.
On 2015/08/27 17:18:24, urvang wrote: > > On 2015/08/26 23:43:14, noel gordon wrote: > > > FIXME: since SkBitmap is being removed from chrome, we should remove this > > > encode() variant. > > Nice! That explains why I never hit this encoder in tests. Yes, it's removal makes things simpler. Removed it today, cc-ed you on the patch as an FYI.
On 2015/08/28 11:20:37, noel gordon wrote: > On 2015/08/28 11:05:10, acterhd wrote: > > Okay, noel. Just copy-paste my code to your patch, and I'm delete my variant. > > No, keep working on your variant, I'll get popcorn. Why you not lgtm me? I'm restored compatibility.
On 2015/08/28 11:25:53, acterhd wrote: > On 2015/08/28 11:20:37, noel gordon wrote: > > On 2015/08/28 11:05:10, acterhd wrote: > > > Okay, noel. Just copy-paste my code to your patch, and I'm delete my > variant. > > > > No, keep working on your variant, I'll get popcorn. > > Why you not lgtm me? I'm restored compatibility. lgtm
On 2015/08/28 11:26:27, acterhd wrote: > lgtm Read https://www.chromium.org/developers/contributing-code to work how to contribute code, write tests, and what "lgtm" is used for.
On 2015/08/28 11:53:58, noel gordon wrote: > On 2015/08/28 11:26:27, acterhd wrote: > > lgtm > > Read https://www.chromium.org/developers/contributing-code to work how to > contribute code, write tests, and what "lgtm" is used for. I'm write test. Wait 1 hour! I can't do it faster.
On 2015/08/28 12:02:05, acterhd wrote: > On 2015/08/28 11:53:58, noel gordon wrote: > > On 2015/08/28 11:26:27, acterhd wrote: > > > lgtm > > > > Read https://www.chromium.org/developers/contributing-code to work how to > > contribute code, write tests, and what "lgtm" is used for. > > I'm write test. Wait 1 hour! I can't do it faster. Already wrote!
On 2015/08/28 12:39:29, acterhd wrote: > On 2015/08/28 12:02:05, acterhd wrote: > > On 2015/08/28 11:53:58, noel gordon wrote: > > > On 2015/08/28 11:26:27, acterhd wrote: > > > > lgtm > > > > > > Read https://www.chromium.org/developers/contributing-code to work how to > > > contribute code, write tests, and what "lgtm" is used for. > > > > I'm write test. Wait 1 hour! I can't do it faster. > > Already wrote! noel, I wanted take off "not lgtm" or "lgtm" status. I happened, I did not know. I did not mean that.
On 2015/08/28 12:42:40, acterhd wrote: > On 2015/08/28 12:39:29, acterhd wrote: > > On 2015/08/28 12:02:05, acterhd wrote: > > > On 2015/08/28 11:53:58, noel gordon wrote: > > > > On 2015/08/28 11:26:27, acterhd wrote: > > > > > lgtm > > > > > > > > Read https://www.chromium.org/developers/contributing-code to work how to > > > > contribute code, write tests, and what "lgtm" is used for. > > > > > > I'm write test. Wait 1 hour! I can't do it faster. > > > > Already wrote! > > noel, I wanted take off "not lgtm" or "lgtm" status. I happened, I did not know. > I did not mean that. I don't know how to control it!
On 2015/08/28 12:43:17, acterhd wrote: > On 2015/08/28 12:42:40, acterhd wrote: > > On 2015/08/28 12:39:29, acterhd wrote: > > > On 2015/08/28 12:02:05, acterhd wrote: > > > > On 2015/08/28 11:53:58, noel gordon wrote: > > > > > On 2015/08/28 11:26:27, acterhd wrote: > > > > > > lgtm > > > > > > > > > > Read https://www.chromium.org/developers/contributing-code to work how > to > > > > > contribute code, write tests, and what "lgtm" is used for. > > > > > > > > I'm write test. Wait 1 hour! I can't do it faster. > > > > > > Already wrote! > > > > noel, I wanted take off "not lgtm" or "lgtm" status. I happened, I did not > know. > > I did not mean that. > > I don't know how to control it! F... How to cancel?!
On 2015/08/28 12:44:04, acterhd wrote: > On 2015/08/28 12:43:17, acterhd wrote: > > On 2015/08/28 12:42:40, acterhd wrote: > > > On 2015/08/28 12:39:29, acterhd wrote: > > > > On 2015/08/28 12:02:05, acterhd wrote: > > > > > On 2015/08/28 11:53:58, noel gordon wrote: > > > > > > On 2015/08/28 11:26:27, acterhd wrote: > > > > > > > lgtm > > > > > > > > > > > > Read https://www.chromium.org/developers/contributing-code to work how > > to > > > > > > contribute code, write tests, and what "lgtm" is used for. > > > > > > > > > > I'm write test. Wait 1 hour! I can't do it faster. > > > > > > > > Already wrote! > > > > > > noel, I wanted take off "not lgtm" or "lgtm" status. I happened, I did not > > know. > > > I did not mean that. > > > > I don't know how to control it! > F... How to cancel?! cancel lgtm
On 2015/08/28 13:08:59, acterhd wrote: > On 2015/08/28 12:44:04, acterhd wrote: > > On 2015/08/28 12:43:17, acterhd wrote: > > > On 2015/08/28 12:42:40, acterhd wrote: > > > > On 2015/08/28 12:39:29, acterhd wrote: > > > > > On 2015/08/28 12:02:05, acterhd wrote: > > > > > > On 2015/08/28 11:53:58, noel gordon wrote: > > > > > > > On 2015/08/28 11:26:27, acterhd wrote: > > > > > > > > lgtm > > > > > > > > > > > > > > Read https://www.chromium.org/developers/contributing-code to work > how > > > to > > > > > > > contribute code, write tests, and what "lgtm" is used for. > > > > > > > > > > > > I'm write test. Wait 1 hour! I can't do it faster. > > > > > > > > > > Already wrote! > > > > > > > > noel, I wanted take off "not lgtm" or "lgtm" status. I happened, I did not > > > know. > > > > I did not mean that. > > > > > > I don't know how to control it! > > F... How to cancel?! > > cancel lgtm TBR
On 2015/08/28 12:44:04, acterhd wrote: > On 2015/08/28 12:43:17, acterhd wrote: > > On 2015/08/28 12:42:40, acterhd wrote: > > > On 2015/08/28 12:39:29, acterhd wrote: > > > > On 2015/08/28 12:02:05, acterhd wrote: > > > > > On 2015/08/28 11:53:58, noel gordon wrote: > > > > > > On 2015/08/28 11:26:27, acterhd wrote: > > > > > > > lgtm > > > > > > > > > > > > Read https://www.chromium.org/developers/contributing-code to work how > > to > > > > > > contribute code, write tests, and what "lgtm" is used for. > > > > > > > > > > I'm write test. Wait 1 hour! I can't do it faster. > > > > > > > > Already wrote! > > > > > > noel, I wanted take off "not lgtm" or "lgtm" status. I happened, I did not > > know. > > > I did not mean that. > > > > I don't know how to control it! The way you are currently going about things is uncontrolled. You should read https://www.chromium.org/developers/contributing-code to work how to contribute code to chromium, write tests, upload patches, get approval, and learn what "lgtm" is used for. > F... How to cancel?! You cannot cancel it or delete it. It is not polite to add "lgtm" or "not lgtm" to any patch unless someone 1) asks you to approve a patch, 2) you have chromium commit access, and 3) you possess the right to approve the patch in that area of the code base. The patch in this issue has not been sent for review. It is a WIP (work in progress) used to explore different implementation options.
On 2015/08/28 13:14:56, noel gordon wrote: > On 2015/08/28 12:44:04, acterhd wrote: > > On 2015/08/28 12:43:17, acterhd wrote: > > > On 2015/08/28 12:42:40, acterhd wrote: > > > > On 2015/08/28 12:39:29, acterhd wrote: > > > > > On 2015/08/28 12:02:05, acterhd wrote: > > > > > > On 2015/08/28 11:53:58, noel gordon wrote: > > > > > > > On 2015/08/28 11:26:27, acterhd wrote: > > > > > > > > lgtm > > > > > > > > > > > > > > Read https://www.chromium.org/developers/contributing-code to work > how > > > to > > > > > > > contribute code, write tests, and what "lgtm" is used for. > > > > > > > > > > > > I'm write test. Wait 1 hour! I can't do it faster. > > > > > > > > > > Already wrote! > > > > > > > > noel, I wanted take off "not lgtm" or "lgtm" status. I happened, I did not > > > know. > > > > I did not mean that. > > > > > > I don't know how to control it! > > The way you are currently going about things is uncontrolled. > > You should read https://www.chromium.org/developers/contributing-code to work > how to > contribute code to chromium, write tests, upload patches, get approval, and > learn what "lgtm" is used for. > > > F... How to cancel?! > > You cannot cancel it or delete it. > > It is not polite to add "lgtm" or "not lgtm" to any patch unless someone 1) asks > you to approve a patch, 2) you have chromium commit access, and 3) you possess > the right to approve the patch in that area of the code base. > > The patch in this issue has not been sent for review. It is a WIP (work in > progress) used to explore different implementation options. I removed my issue. I better re-think about publication again.
On 2015/08/28 13:23:07, acterhd wrote: > I removed my issue. I better re-think about publication again. You did not need to remove your issue. But yes, I suggest you re-think your approach.
On 2015/08/28 13:29:58, noel gordon wrote: > On 2015/08/28 13:23:07, acterhd wrote: > > I removed my issue. I better re-think about publication again. > > You did not need to remove your issue. But yes, I suggest you re-think your > approach. I created again. https://codereview.chromium.org/1306103006
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302423004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302423004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302423004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/08/27 17:18:24, urvang wrote: > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:129: config.method = > (quality * 6) / 100; > I would keep method from 0 to 3. > And FYI: quality=100 & m=3 is still pretty good compression wise. > (even q=10, m = 0 in fact gives decent compression). Ok made it m=2. > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:140: config.lossless = > 1; > We have a separate setting for lossless, so quality > 99 should NOT mean > lossless. > We should use losssy with WebPPictureSmartARGBToYUVA() instead, which would be > reasonably sharp and yet smaller in size and a bit faster than lossless. > In fact, Pascal suggested using it for all high qualities for lossy (q>=90 say) WebPPictureSmartARGBToYUVA is not in chrome, so turned it down to lossless m=0, q=75 and can at least write tests (added).
On 2015/09/01 13:24:39, noel gordon wrote: > On 2015/08/27 17:18:24, urvang wrote: > > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > > File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): > > > > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:129: config.method = > > (quality * 6) / 100; > > I would keep method from 0 to 3. > > And FYI: quality=100 & m=3 is still pretty good compression wise. > > (even q=10, m = 0 in fact gives decent compression). > > Ok made it m=2. > > > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:140: config.lossless > = > > 1; > > We have a separate setting for lossless, so quality > 99 should NOT mean > > lossless. > > We should use losssy with WebPPictureSmartARGBToYUVA() instead, which would be > > reasonably sharp and yet smaller in size and a bit faster than lossless. > > In fact, Pascal suggested using it for all high qualities for lossy (q>=90 > say) > > WebPPictureSmartARGBToYUVA is not in chrome, so turned it down to lossless m=0, > q=75 and can at least write tests (added). Why you not accept my patch? https://codereview.chromium.org/1304363010/
On 2015/09/01 13:24:39, noel gordon wrote: > On 2015/08/27 17:18:24, urvang wrote: > > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > > File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): > > > > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:129: config.method = > > (quality * 6) / 100; > > I would keep method from 0 to 3. > > And FYI: quality=100 & m=3 is still pretty good compression wise. > > (even q=10, m = 0 in fact gives decent compression). > > Ok made it m=2. > > > > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:140: config.lossless > = > > 1; > > We have a separate setting for lossless, so quality > 99 should NOT mean > > lossless. > > We should use losssy with WebPPictureSmartARGBToYUVA() instead, which would be > > reasonably sharp and yet smaller in size and a bit faster than lossless. > > In fact, Pascal suggested using it for all high qualities for lossy (q>=90 > say) > > WebPPictureSmartARGBToYUVA is not in chrome, so turned it down to lossless m=0, > q=75 and can at least write tests (added). Ah, looks like these are under #ifdef in Chrome still: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb..., because they are in HEAD, but not the latest released version of libwebp: https://chromium.googlesource.com/webm/libwebp/+/0.4.3/src/webp/encode.h So, we can add that later.
lgtm https://codereview.chromium.org/1302423004/diff/100001/LayoutTests/fast/canva... File LayoutTests/fast/canvas/toDataURL-supportedTypes.html (left): https://codereview.chromium.org/1302423004/diff/100001/LayoutTests/fast/canva... LayoutTests/fast/canvas/toDataURL-supportedTypes.html:28: "image/gif", Curious: How was "image/gif" working before?
https://codereview.chromium.org/1302423004/diff/100001/LayoutTests/fast/canva... File LayoutTests/fast/canvas/toDataURL-supportedTypes.html (left): https://codereview.chromium.org/1302423004/diff/100001/LayoutTests/fast/canva... LayoutTests/fast/canvas/toDataURL-supportedTypes.html:28: "image/gif", On 2015/09/01 19:54:08, urvang wrote: > Curious: How was "image/gif" working before? The existing test expectations report: Given MIMEType: image/gif Used MIMEType: image/png MIME types DIFFER. so no change in behavior with this CL. Unsupported image/ types must be encoded as PNG per the <canvas> spec.
On 2015/09/01 19:47:57, urvang wrote: > On 2015/09/01 13:24:39, noel gordon wrote: > https://codereview.chromium.org/1302423004/diff/1/Source/platform/image-encod... > > > Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:140: > config.lossless > > = > > > 1; > > > We have a separate setting for lossless, so quality > 99 should NOT mean > > > lossless. > > > We should use losssy with WebPPictureSmartARGBToYUVA() instead, which would > be > > > reasonably sharp and yet smaller in size and a bit faster than lossless. > > > In fact, Pascal suggested using it for all high qualities for lossy (q>=90 > > say) > > > > WebPPictureSmartARGBToYUVA is not in chrome, so turned it down to lossless > m=0, > > q=75 and can at least write tests (added). > > Ah, looks like these are under #ifdef in Chrome still: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb..., > because they are in HEAD, but not the latest released version of libwebp: > https://chromium.googlesource.com/webm/libwebp/+/0.4.3/src/webp/encode.h Yeap, we don't have it yet. > So, we can add that later. Ok.
Also as discussed, changed the encoder selector to be "image/webp.ll" everywhere.
noel@chromium.org changed reviewers: - acterhd@gmail.com
On 2015/09/01 19:54:08, urvang wrote: > lgtm Updated changelog, tests passing locally for me. PTAL.
pdr@chromium.org changed reviewers: + acterhd@gmail.com, chrishtr@chromium.org, pdr@chromium.org
Why are we doing this through the type argument instead of using additional quality control arguments? The spec seems to be explicitly designed to be extensible: http://www.w3.org/html/wg/drafts/html/master/semantics.html#canvas-serialisat...
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302423004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/02 03:54:53, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. noel, why not do like in my patch? - save MIMETypeRegister unchanged - save lossy webp for backward compatibility - add two MIME options instead one (without change registry) - deprecate imagebuffer.todataurl function ???
On 2015/09/02 06:06:37, acterhd wrote: > On 2015/09/02 03:54:53, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > noel, why not do like in my patch? > - save MIMETypeRegister unchanged > - save lossy webp for backward compatibility > - add two MIME options instead one (without change registry) > - deprecate imagebuffer.todataurl function > ??? I added less layout tests, and well passed.
On 2015/09/02 06:08:51, acterhd wrote: > > noel, why not do like in my patch? > > - save MIMETypeRegister unchanged the registry was changed. > > - save lossy webp for backward compatibility this is not necessary. > > - add two MIME options instead one (without change registry) webp is lossy or lossess: there is no need for a separate "alpha" type. > > - deprecate imagebuffer.todataurl function unrelated change and not the subject of the bug. > I added less layout tests, and well passed. the tests do not cover the changes you made. acterhd@ there is process required to make Blink changes. I have sent you links to the documents that explain this. You should read and understand those documents, and follow the process. The current bug requires spec work, and understanding of other changes going on in Blink, and higher-level design work. This is not something I recommend you try and pursue as your initial contribution to Blink. crbug.com has many bugs that are much simpler than this bug. For example, a simple bug like adding a test might be good place to start. We all have to start somewhere. The best place to start is at the beginning. Solving a simple bug will help you understand how our process works, and also why it works.
On 2015/09/02 13:15:26, noel gordon wrote: > On 2015/09/02 06:08:51, acterhd wrote: > > > > noel, why not do like in my patch? > > > - save MIMETypeRegister unchanged > the registry was changed. > > > - save lossy webp for backward compatibility > this is not necessary. > > > - add two MIME options instead one (without change registry) > webp is lossy or lossess: there is no need for a separate "alpha" type. > > > - deprecate imagebuffer.todataurl function > unrelated change and not the subject of the bug. > > I added less layout tests, and well passed. > the tests do not cover the changes you made. > > acterhd@ there is process required to make Blink changes. I have sent you links > to the documents that explain this. You should read and understand those > documents, and follow the process. The current bug requires spec work, and > understanding of other changes going on in Blink, and higher-level design work. > This is not something I recommend you try and pursue as your initial > contribution to Blink. http://crbug.com has many bugs that are much simpler than this > bug. For example, a simple bug like adding a test might be good place to start. > We all have to start somewhere. The best place to start is at the beginning. > Solving a simple bug will help you understand how our process works, and also > why it works. noel, I has bug fix only with 1-2 line, but you don't believe.. This is has been with -webkit-canvas(). But this CSS property was deprecated. And I want use WebP lossless encoder only with one target - reconstruct APNG animation, cause less damage to DOM as possible...
On 2015/09/02 13:23:56, acterhd wrote: > On 2015/09/02 13:15:26, noel gordon wrote: > > On 2015/09/02 06:08:51, acterhd wrote: > > > > > > noel, why not do like in my patch? > > > > - save MIMETypeRegister unchanged > > the registry was changed. > > > > - save lossy webp for backward compatibility > > this is not necessary. > > > > - add two MIME options instead one (without change registry) > > webp is lossy or lossess: there is no need for a separate "alpha" type. > > > > - deprecate imagebuffer.todataurl function > > unrelated change and not the subject of the bug. > > > I added less layout tests, and well passed. > > the tests do not cover the changes you made. > > > > acterhd@ there is process required to make Blink changes. I have sent you > links > > to the documents that explain this. You should read and understand those > > documents, and follow the process. The current bug requires spec work, and > > understanding of other changes going on in Blink, and higher-level design > work. > > This is not something I recommend you try and pursue as your initial > > contribution to Blink. http://crbug.com has many bugs that are much simpler > than this > > bug. For example, a simple bug like adding a test might be good place to > start. > > We all have to start somewhere. The best place to start is at the beginning. > > > Solving a simple bug will help you understand how our process works, and also > > why it works. > > noel, I has bug fix only with 1-2 line, but you don't believe.. This is has been > with -webkit-canvas(). But this CSS property was deprecated. And I want use WebP > lossless encoder only with one target - reconstruct APNG animation, cause less > damage to DOM as possible... But did not made layout tests.
I removed own patch. Good luck, noel!
On 2015/09/03 12:14:32, acterhd wrote: > I removed own patch. Good luck, noel! And thanks for trying. I have attributed your contribution in the change description. I know you want to use lossless webp for your development work. We need some way to allow web developer access the lossless feature of the webp encoder. The WIP patch in this issue is one possible way. Other ways might involve spec changes, a process that takes time to resolve.
On 2015/09/02 02:22:34, pdr wrote: > Why are we doing this through the type argument instead of using additional > quality control arguments? Additional quality control arguments? No such thing in the spec ... > The spec seems to be explicitly designed to be extensible: > http://www.w3.org/html/wg/drafts/html/master/semantics.html#canvas-serialisat... other than a statement that "Other [toDataURL] arguments must be ignored and must not cause the user agent to throw an exception. A future version of this specification will probably define other parameters to be passed to these methods to allow authors to more carefully control compression settings, image metadata, etc." ... if that is what you meant by "extensible"? Seems undefined at present. re: using mimeTypes as in the CL: "image/webp" is one (though it's not registered at IANA). "image/webp.ll" is one if you squint, and maybe could be registered for the toDataURL toBlob use-cases. Using a "mimeType" seems acceptable per the spec. aside: @skal, no IANA register mimes for webp?
> aside: @skal, no IANA register mimes for webp? mostly: lack of time to write a proper RFC.
On 2015/09/07 at 02:39:22, noel wrote: > On 2015/09/02 02:22:34, pdr wrote: > > Why are we doing this through the type argument instead of using additional > > quality control arguments? > > Additional quality control arguments? No such thing in the spec ... > > > The spec seems to be explicitly designed to be extensible: > > http://www.w3.org/html/wg/drafts/html/master/semantics.html#canvas-serialisat... > > other than a statement that "Other [toDataURL] arguments must be ignored and must not cause the user agent to throw an exception. A future version of this specification will probably define other parameters to be passed to these methods to allow authors to more carefully control compression settings, image metadata, etc." > > ... if that is what you meant by "extensible"? Seems undefined at present. I don't think you're giving the spec a fair chance here. This patch is exactly about "more carefully controlling compression settings", no?
Compression settings? The webp encoder has many such settings [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... and here I don't we're about exposing these webp-specifics at all.
On 2015/11/30 at 10:02:30, noel wrote: > Compression settings? The webp encoder has many such settings [1] > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > and here I don't we're about exposing these webp-specifics at all. Can you rephrase the last sentence? Are you still in favor of controlling settings with a mimetype?
On 2015/11/30 17:17:55, pdr wrote: > On 2015/11/30 at 10:02:30, noel wrote: > > Compression settings? The webp encoder has many such settings [1] > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > > > and here I don't we're about exposing these webp-specifics at all. > > Can you rephrase the last sentence? > > Are you still in favor of controlling settings with a mimetype? Philip, do you have a suggestion about how to do this differently? Should we put up a spec proposal for an encoderDictionary (to be passed after encoderOptions) that would take into account various options? What should they be? It seems counterintuitive that a quality level less than 1.0 would have any effect on the lossless version of webp. Should a 1.0 value for encoderOptions just imply the lossless webp variant? Let's figure out how to unblock this work.
On 2015/12/08 at 23:11:37, kbr wrote: > On 2015/11/30 17:17:55, pdr wrote: > > On 2015/11/30 at 10:02:30, noel wrote: > > > Compression settings? The webp encoder has many such settings [1] > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > > > > > and here I don't we're about exposing these webp-specifics at all. > > > > Can you rephrase the last sentence? > > > > Are you still in favor of controlling settings with a mimetype? > > Philip, do you have a suggestion about how to do this differently? Should we put up a spec proposal for an encoderDictionary (to be passed after encoderOptions) that would take into account various options? What should they be? > > It seems counterintuitive that a quality level less than 1.0 would have any effect on the lossless version of webp. Should a 1.0 value for encoderOptions just imply the lossless webp variant? > > Let's figure out how to unblock this work. Yeah, a dictionary sounds great. How about adding "webp" to this list: http://www.w3.org/html/wg/drafts/html/master/semantics.html#canvas-serialisat... And for the second argument, spec your dictionary with the various webp encoder options. Would that work?
Oops, for your second question "Should a 1.0 value for encoderOptions just imply the lossless webp variant?": that sounds reasonable to me. Another option would be to have a lossless flag in the dictionary as we do in the c++ code. Some of the options may apply to both lossy and lossless so the explicit flag could be useful in disambiguating those situations.
On 2015/11/30 17:17:55, pdr wrote: > On 2015/11/30 at 10:02:30, noel wrote: > > Compression settings? The webp encoder has many such settings [1] > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > > > and here I don't we're about exposing these webp-specifics at all. > > Can you rephrase the last sentence? Apologies, and of course. ... and here I don't _think_ we intend to expose all those webp-specific params ... config->quality = quality; config->target_size = 0; config->target_PSNR = 0.; config->method = 4; config->sns_strength = 50; config->filter_strength = 60; config->filter_sharpness = 0; config->filter_type = 1; config->partitions = 0; config->segments = 4; config->pass = 1; config->show_compressed = 0; config->preprocessing = 0; config->autofilter = 0; config->partition_limit = 0; config->alpha_compression = 1; config->alpha_filtering = 1; config->alpha_quality = 100; config->lossless = 0; config->image_hint = WEBP_HINT_DEFAULT; config->emulate_jpeg_size = 0; config->thread_level = 0; config->low_memory = 0; That'd lock-in the webp folks to supporting them on the web. When you mentioned "Compression settings", that list is the one that springs to (my) mind. Large list, and imagine specing it all. Perhaps by "Compression settings", you meant something else? > Are you still in favor of controlling settings with a mimetype? I could go that way unless there's a better suggestion: it saves specing anything and conforms and allows developers to control the lossless "quality" ...
On 2015/12/08 23:11:37, Ken Russell wrote: > On 2015/11/30 17:17:55, pdr wrote: > > On 2015/11/30 at 10:02:30, noel wrote: > > > Compression settings? The webp encoder has many such settings [1] > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > > > > > and here I don't we're about exposing these webp-specifics at all. > > > > Can you rephrase the last sentence? > > > > Are you still in favor of controlling settings with a mimetype? > > Philip, do you have a suggestion about how to do this differently? Should we put > up a spec proposal for an encoderDictionary (to be passed after encoderOptions) > that would take into account various options? What should they be? > > It seems counterintuitive that a quality level less than 1.0 would have any > effect on the lossless version of webp. Should a 1.0 value for encoderOptions > just imply the lossless webp variant? Counterintuitive, but let me try and explain: refer to https://code.google.com/p/chromium/issues/detail?id=179289 image/png is lossless too, but developers want control over its "lossless quality" - how crushed it is, aka how long it takes, aka the speed - and we proposed to whatwg (see the bug for outcome). Seems like a reasonable developer request to me based on their examples, and I figure lossless webp like png has the same issue. If instead "image/webp", quality=1.0 is co-opted to mean create a lossless webp as you suggest, then the browser must internally choose how long it takes. We pick a default internal quality like we do for png. Developers are unhappy that they cannot control how long it takes. Currently developers get different results cross-browser. "image/png": Firefox (they favour crush and take longer), vs Chrome where we favor speed and less crush. Safari matched Mozilla here in the past, but later changed to match Chrome. Developers find that situation painful to deal with and want better control. The proposal of this patch is to select lossless webp via a mime-type, and use the optional quality argument to allow developers to have that control. A suitable default quality is used if none is given. All matches spec.
On 2015/12/08 23:11:37, Ken Russell wrote:
> Philip, do you have a suggestion about how to do this differently? Should we
put
> up a spec proposal for an encoderDictionary (to be passed after
encoderOptions)
> that would take into account various options? What should they be?
You could use a dictionary sure: so to options. Maybe { lossless : true,
quality: 0.5 } for webp, not really sure about png.
toDataURL("image/webp", { lossless : true, quality: 0.5 }); // not speced
toDataURL("image/png", 0.5); // not spec-ed
This patch proposal is in the spirit of the spec:
toDataURL("image/webpll", 0.5);
whereas a dict will require spec-level text as we known.
On 2015/12/08 23:17:18, pdr wrote: > How about adding "webp" to this list: > http://www.w3.org/html/wg/drafts/html/master/semantics.html#canvas-serialisat... Two browser vendors would need to agree to that, no?
On 2015/12/08 23:49:19, pdr wrote: > Oops, for your second question "Should a 1.0 value for encoderOptions just imply > the lossless webp variant?": that sounds reasonable to me. https://code.google.com/p/chromium/issues/detail?id=179289 for developer POV on that. What dict option would placate them? I imagined a float would do /shrug.
> a dict will require spec-level text as we known. meant, "as we know", kaibia. |
