Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1501303002:
SkPixelSerializer: support indexed pixels (Closed)
|
Created:
5 years ago by hal.canary Modified:
5 years ago CC:
mtklein, reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPixelSerializer: support indexed pixels
By taking a SkPixmap, SkPixelSerializer::encode() can now handle colortables.
Committed: https://skia.googlesource.com/skia/+/6b28017781b47ebd9c9c3f3a52561f05702d5e8a
Patch Set 1 #Patch Set 2 : 2015-12-06 (Sunday) 17:21:55 EST #
Total comments: 3
Patch Set 3 : 2015-12-07 (Monday) 10:42:27 EST #Patch Set 4 : 2015-12-07 (Monday) 10:44:44 EST #
Total comments: 6
Patch Set 5 : 2015-12-07 (Monday) 15:24:44 EST #
Messages
Total messages: 33 (13 generated)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501303002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501303002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501303002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501303002/20001
halcanary@google.com changed reviewers: + reed@google.com
PTAL Motivation: I'm using SkPixelSerializer in SkRecord.
halcanary@google.com changed reviewers: + scroggo@google.com
Adding Leon for codec review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks like a few things are going on here: - some convenience methods are being added (for pixmaps) - a 2nd virtual added to PixelSerializer - using SkImageEncoder as a default factory for a serializer Are they all required to be part of the same cl? https://codereview.chromium.org/1501303002/diff/20001/src/images/SkImageEncod... File src/images/SkImageEncoder.cpp (right): https://codereview.chromium.org/1501303002/diff/20001/src/images/SkImageEncod... src/images/SkImageEncoder.cpp:62: bm.setImmutable(); why is this needed?
On 2015/12/07 at 15:01:28, reed wrote: > Looks like a few things are going on here: > > - some convenience methods are being added (for pixmaps) > - a 2nd virtual added to PixelSerializer > - using SkImageEncoder as a default factory for a serializer > > Are they all required to be part of the same cl? I'll break this up into two CLs.
https://codereview.chromium.org/1501303002/diff/20001/src/images/SkImageEncod... File src/images/SkImageEncoder.cpp (right): https://codereview.chromium.org/1501303002/diff/20001/src/images/SkImageEncod... src/images/SkImageEncoder.cpp:62: bm.setImmutable(); On 2015/12/07 at 15:01:28, reed1 wrote: > why is this needed? I think it's a good idea to call setImmutable() to absolutely ensure that the const_cast is okay.
https://codereview.chromium.org/1501303002/diff/20001/src/images/SkImageEncod... File src/images/SkImageEncoder.cpp (right): https://codereview.chromium.org/1501303002/diff/20001/src/images/SkImageEncod... src/images/SkImageEncoder.cpp:62: bm.setImmutable(); On 2015/12/07 15:15:36, Hal Canary wrote: > On 2015/12/07 at 15:01:28, reed1 wrote: > > why is this needed? > > I think it's a good idea to call setImmutable() to absolutely ensure that the > const_cast is okay. I don't disagree, but I think its extraneous, since we're not enforcing it via an assert, and we don't behave differently w/ or w/o it. My nit-request is to not call it.
Description was changed from ========== SkPixelSerializer: support indexed pixels Add SkImageEncoder::EncodeData(const SkPixmap&, ...) function. Add SkImageEncoder::CreatePixelSerializer() to return a PixelSerializer that calls into SkImageEncoder::EncodeData. SkImage::encode() make use of SkImageEncoder::CreatePixelSerializer. ========== to ========== SkPixelSerializer: support indexed pixels ==========
On 2015/12/07 at 15:28:29, reed wrote: > https://codereview.chromium.org/1501303002/diff/20001/src/images/SkImageEncod... > File src/images/SkImageEncoder.cpp (right): > > https://codereview.chromium.org/1501303002/diff/20001/src/images/SkImageEncod... > src/images/SkImageEncoder.cpp:62: bm.setImmutable(); > On 2015/12/07 15:15:36, Hal Canary wrote: > > On 2015/12/07 at 15:01:28, reed1 wrote: > > > why is this needed? > > > > I think it's a good idea to call setImmutable() to absolutely ensure that the > > const_cast is okay. > > I don't disagree, but I think its extraneous, since we're not enforcing it via an assert, and we don't behave differently w/ or w/o it. My nit-request is to not call it. done
On 2015/12/07 at 15:15:31, Hal Canary wrote: > On 2015/12/07 at 15:01:28, reed wrote: > > Looks like a few things are going on here: > > > > - some convenience methods are being added (for pixmaps) > > - a 2nd virtual added to PixelSerializer > > - using SkImageEncoder as a default factory for a serializer > > > > Are they all required to be part of the same cl? > > > I'll break this up into two CLs. Done. PTAL at Patchset 3.
lgtm https://codereview.chromium.org/1501303002/diff/60001/include/core/SkPixelSer... File include/core/SkPixelSerializer.h (right): https://codereview.chromium.org/1501303002/diff/60001/include/core/SkPixelSer... include/core/SkPixelSerializer.h:35: SkData* encodePixels(const SkImageInfo& info, const void* pixels, size_t rowBytes) { Does anyone call this other than SkWriteBuffer? Or can we go ahead and remove it? https://codereview.chromium.org/1501303002/diff/60001/include/core/SkPixelSer... include/core/SkPixelSerializer.h:60: // NOTE: onEncodePixels() is deprecated and removed in a later CL. Does Chrome override this? Should we use a macro to conditionally remove it? https://codereview.chromium.org/1501303002/diff/60001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/1501303002/diff/60001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:190: return bm.installPixels(pixmap.info(), nit: I think this would be easier to follow if you separate it out into multiple statements. e.g. if (!bm.installPixels(...)) return nullptr; return SkImageEncoder::EncodeData(...); Maybe that's just a matter of preference though.
api lgtm
https://codereview.chromium.org/1501303002/diff/60001/include/core/SkPixelSer... File include/core/SkPixelSerializer.h (right): https://codereview.chromium.org/1501303002/diff/60001/include/core/SkPixelSer... include/core/SkPixelSerializer.h:35: SkData* encodePixels(const SkImageInfo& info, const void* pixels, size_t rowBytes) { On 2015/12/07 at 15:57:07, scroggo wrote: > Does anyone call this other than SkWriteBuffer? Or can we go ahead and remove it? done https://codereview.chromium.org/1501303002/diff/60001/include/core/SkPixelSer... include/core/SkPixelSerializer.h:60: // NOTE: onEncodePixels() is deprecated and removed in a later CL. On 2015/12/07 at 15:57:07, scroggo wrote: > Does Chrome override this? Should we use a macro to conditionally remove it? Chrome and only Chrome overrides this. Macros are usually necessary, but this fallback plan doesn't need them. https://codereview.chromium.org/1501303002/diff/60001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/1501303002/diff/60001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:190: return bm.installPixels(pixmap.info(), On 2015/12/07 at 15:57:07, scroggo wrote: > nit: I think this would be easier to follow if you separate it out into multiple statements. e.g. > > if (!bm.installPixels(...)) > return nullptr; > > return SkImageEncoder::EncodeData(...); > > Maybe that's just a matter of preference though. done
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501303002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501303002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SkPixelSerializer: support indexed pixels ========== to ========== SkPixelSerializer: support indexed pixels By taking a SkPixmap, SkPixelSerializer::encode() can now handle colortables. ==========
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1501303002/#ps80001 (title: "2015-12-07 (Monday) 15:24:44 EST")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501303002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501303002/80001
Message was sent while issue was closed.
Description was changed from ========== SkPixelSerializer: support indexed pixels By taking a SkPixmap, SkPixelSerializer::encode() can now handle colortables. ========== to ========== SkPixelSerializer: support indexed pixels By taking a SkPixmap, SkPixelSerializer::encode() can now handle colortables. Committed: https://skia.googlesource.com/skia/+/6b28017781b47ebd9c9c3f3a52561f05702d5e8a ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/6b28017781b47ebd9c9c3f3a52561f05702d5e8a |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
