Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2294993002:
Add color xform support to SkWebpCodec (Closed)
DescriptionAdd color xform support to SkWebpCodec
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2294993002
Committed: https://skia.googlesource.com/skia/+/828ed1777da74692d0c8a5834017929f5aedcc6b
Committed: https://skia.googlesource.com/skia/+/e99883f33b6443c243eab3cac5a64677a9edfcc7
Patch Set 1 #
Total comments: 8
Patch Set 2 : Response to comments #Patch Set 3 : Try v6 #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 31 (21 generated)
Description was changed from ========== Add color xform support to SkWebpCodec BUG=skia: ========== to ========== Add color xform support to SkWebpCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2294993002 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
lgtm https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkCodecPriv.h... src/codec/SkCodecPriv.h:334: static inline SkAlphaType color_xform_alpha_type(SkAlphaType dstAlphaType, SkAlphaType srcAlphaType) This name seems extra specific, and yet it doesn't tell you what it does. I don't have a better name off the top of my head though. https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkWebpCodec.cpp File src/codec/SkWebpCodec.cpp (right): https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:282: // to decode BGRA and apply the color xform to a BGRA buffer. Why don't we today? https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:290: // buffer to F16. libwebp does not support a row-by-row decode. It seems like this comment mostly applies to the below case, too? https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:318: case VP8_STATUS_SUSPENDED: { Nit: do you need the braces?
https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkCodecPriv.h... src/codec/SkCodecPriv.h:334: static inline SkAlphaType color_xform_alpha_type(SkAlphaType dstAlphaType, SkAlphaType srcAlphaType) On 2016/09/07 20:32:58, scroggo_BACK_FROM_PATERNITY wrote: > This name seems extra specific, and yet it doesn't tell you what it does. I > don't have a better name off the top of my head though. Going with "select_alpha_xform". https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkWebpCodec.cpp File src/codec/SkWebpCodec.cpp (right): https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:282: // to decode BGRA and apply the color xform to a BGRA buffer. On 2016/09/07 20:32:58, scroggo_BACK_FROM_PATERNITY wrote: > Why don't we today? I haven't implemented BGRA as an input yet :) https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:290: // buffer to F16. libwebp does not support a row-by-row decode. On 2016/09/07 20:32:58, scroggo_BACK_FROM_PATERNITY wrote: > It seems like this comment mostly applies to the below case, too? Yeah, I'll move it up. It's just that it's awful in the F16 case because we need to allocate an extra buffer. https://codereview.chromium.org/2294993002/diff/60001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:318: case VP8_STATUS_SUSPENDED: { On 2016/09/07 20:32:58, scroggo_BACK_FROM_PATERNITY wrote: > Nit: do you need the braces? No, removed.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/2294993002/#ps80001 (title: "Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add color xform support to SkWebpCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2294993002 ========== to ========== Add color xform support to SkWebpCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2294993002 Committed: https://skia.googlesource.com/skia/+/828ed1777da74692d0c8a5834017929f5aedcc6b ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001) as https://skia.googlesource.com/skia/+/828ed1777da74692d0c8a5834017929f5aedcc6b
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:80001) has been created in https://codereview.chromium.org/2318253004/ by msarett@google.com. The reason for reverting is: Windows bots hate it when I upload new images..
Message was sent while issue was closed.
msarett@google.com changed reviewers: + borenet@google.com
Message was sent while issue was closed.
Message was sent while issue was closed.
Description was changed from ========== Add color xform support to SkWebpCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2294993002 Committed: https://skia.googlesource.com/skia/+/828ed1777da74692d0c8a5834017929f5aedcc6b ========== to ========== Add color xform support to SkWebpCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2294993002 Committed: https://skia.googlesource.com/skia/+/828ed1777da74692d0c8a5834017929f5aedcc6b ==========
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/08 01:54:16, msarett wrote: Think I have this working now - by deleting the .cipd directory from my local copy of the images. I think the .cipd tries to be efficient by referencing previous versions of the images (rather than copying them every time). But for some reason, I don't think this works on Windows.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/2294993002/#ps100001 (title: "Try v6")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add color xform support to SkWebpCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2294993002 Committed: https://skia.googlesource.com/skia/+/828ed1777da74692d0c8a5834017929f5aedcc6b ========== to ========== Add color xform support to SkWebpCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2294993002 Committed: https://skia.googlesource.com/skia/+/828ed1777da74692d0c8a5834017929f5aedcc6b Committed: https://skia.googlesource.com/skia/+/e99883f33b6443c243eab3cac5a64677a9edfcc7 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://skia.googlesource.com/skia/+/e99883f33b6443c243eab3cac5a64677a9edfcc7 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
