Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2085653003:
Enable flattening and unflattening of SkColorSpace (Closed)
DescriptionEnable flattening and unflattening of SkColorSpace
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2085653003
Committed: https://skia.googlesource.com/skia/+/111a42d9cebf0bb8844c5d24f67fac57cc619d29
Patch Set 1 #
Total comments: 3
Patch Set 2 : Serialize without SkFlattenable #
Total comments: 6
Patch Set 3 : Response to comments #
Total comments: 2
Patch Set 4 : Add ColorSpaceHeader #
Total comments: 8
Patch Set 5 : Save 12 floats instead of 16 #
Total comments: 6
Patch Set 6 : Return null on error #
Total comments: 2
Patch Set 7 : Rebase #Patch Set 8 : Fix debug build #Patch Set 9 : Fix unit tests #
Depends on Patchset: Messages
Total messages: 54 (22 generated)
Description was changed from ========== Enable flattening and unflattening of SkColorSpace BUG=skia: ========== to ========== Enable flattening and unflattening of SkColorSpace BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2085653003 ==========
msarett@google.com changed reviewers: + brianosman@google.com, reed@google.com
https://codereview.chromium.org/2085653003/diff/1/include/core/SkColorSpace.h File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2085653003/diff/1/include/core/SkColorSpace.h... include/core/SkColorSpace.h:14: class SK_API SkColorSpace : public SkFlattenable { We can flatten/unflatten without making SkColorSpace an SkFlattenable. I think this is fine, but I'm indifferent. Didn't really want a public CreateProc()... https://codereview.chromium.org/2085653003/diff/1/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/1/src/core/SkColorSpace.cpp#n... src/core/SkColorSpace.cpp:32: , fProfileData(nullptr) The decision here is to only hold onto the profile data if we'll need it to flatten the SkColorSpace object... https://codereview.chromium.org/2085653003/diff/1/src/core/SkColorSpace_Base.h File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2085653003/diff/1/src/core/SkColorSpace_Base.... src/core/SkColorSpace_Base.h:158: sk_sp<SkData> writeToICC() const; I'm not sure about the future of this API. Any thoughts about making it (more) private? I keep breaking it in different ways, and I'm not sure it has a use case anymore.
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/patch-status/2085653003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
My first thought was to not be a flattenable either, for simplicity and since we don't support 3rd-party subclassing at all (which is some of the cost/value of the Flattenable infrastructure). What about: SkData* serialize() const; static sk_sp<SkColorSpace> Deserialize(const void* data, size_t length);
Patchset #2 (id:20001) has been deleted
On 2016/06/21 01:40:30, reed1 wrote: > My first thought was to not be a flattenable either, for simplicity and since we > don't support 3rd-party subclassing at all (which is some of the cost/value of > the Flattenable infrastructure). > > What about: > > SkData* serialize() const; > > static sk_sp<SkColorSpace> Deserialize(const void* data, size_t length); SGTM. I've uploaded a new version that matches this API.
If the caller needs to know about SkData anyway (for serialize), is it too much to ask that we also use SkData for Deserialize? I know it potentially adds an extra step for the caller (wrapping their data), depending on how they actually stored/sent the blob, but I like symmetry. https://codereview.chromium.org/2085653003/diff/40001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/40001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1260: write_matrix((float*) dataPtr, fToXYZD50); Could just write: fToXYZD50.asColMajorf((float*)dataPtr); ... and remove write_matrix entirely. https://codereview.chromium.org/2085653003/diff/40001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1336: read_matrix(&toXYZ, (const float*) data); Similar to above: toXYZ.setColMajorf((const float*) data); ... and remove read_matrix.
I'm fine if we also offer a Deserialize that takes SkData. Requiring it seems odd, since we won't actually "ref" it, but will just parse it, thus forcing the caller to make malloc'd a SkData (and possibly copied the data) just to give us a ptr.
/self wonders if we should offer SkImageInfo.serialize as well, just for fun :) https://codereview.chromium.org/2085653003/diff/40001/include/core/SkColorSpa... File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2085653003/diff/40001/include/core/SkColorSpa... include/core/SkColorSpace.h:83: SkData* serialize() const; sk_sp<SkData>
"If the caller needs to know about SkData anyway (for serialize), is it too much to ask that we also use SkData for Deserialize? I know it potentially adds an extra step for the caller (wrapping their data), depending on how they actually stored/sent the blob, but I like symmetry." I don't disagree, but it is strange to pass an SkData that we won't ref. I'd rather keep as is - I do like the symmetry between this function and NewFromICC(const void*, size_t). https://codereview.chromium.org/2085653003/diff/40001/include/core/SkColorSpa... File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2085653003/diff/40001/include/core/SkColorSpa... include/core/SkColorSpace.h:83: SkData* serialize() const; On 2016/06/21 14:19:19, reed1 wrote: > sk_sp<SkData> Done. https://codereview.chromium.org/2085653003/diff/40001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/40001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1260: write_matrix((float*) dataPtr, fToXYZD50); On 2016/06/21 13:38:19, Brian Osman wrote: > Could just write: > fToXYZD50.asColMajorf((float*)dataPtr); > ... and remove write_matrix entirely. Ahh yes of course. https://codereview.chromium.org/2085653003/diff/40001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1336: read_matrix(&toXYZ, (const float*) data); On 2016/06/21 13:38:19, Brian Osman wrote: > Similar to above: > toXYZ.setColMajorf((const float*) data); > ... and remove read_matrix. Yes.
lgtm
https://codereview.chromium.org/2085653003/diff/60001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/60001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1211: /////////////////////////////////////////////////////////////////////////////////////////////////// Some thoughts. I think we should be more explicit about sizes and the memory layout of the serialized buffer. sizeof(enum) can possibly change (depending on compiler settings and the value ranges for our enums), and I also think we should explicitly encode the size of the profile-data (when present) rather than rely on implicitly knowing it from the total size... think of the case where we may have our contents embedded in a larger stream of data... I guess that last thought implies that our Deserialize might want to return the number of bytes actually processed (think if Deserialize took a stream ...) e.g. First 4 bytes encode the type of encoding... struct Header { uint8_t fVersion; uint8_t fNamed; // or 0xFF if this guy is not named uint8_t fGammaNamed; // or 0xFF if this guy is not a named gamma uint8_t fFlags; // 0 for now, maybe 1 bit means an ICC Profile follows: 4-byte length + data (padded to 4 bytes?) };
PTAL https://codereview.chromium.org/2085653003/diff/60001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/60001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1211: /////////////////////////////////////////////////////////////////////////////////////////////////// On 2016/06/21 14:58:34, reed1 wrote: > Some thoughts. > > I think we should be more explicit about sizes and the memory layout of the > serialized buffer. sizeof(enum) can possibly change (depending on compiler > settings and the value ranges for our enums), and I also think we should > explicitly encode the size of the profile-data (when present) rather than rely > on implicitly knowing it from the total size... think of the case where we may > have our contents embedded in a larger stream of data... > > I guess that last thought implies that our Deserialize might want to return the > number of bytes actually processed (think if Deserialize took a stream ...) > > e.g. > First 4 bytes encode the type of encoding... > > struct Header { > uint8_t fVersion; > uint8_t fNamed; // or 0xFF if this guy is not named > uint8_t fGammaNamed; // or 0xFF if this guy is not a named gamma > uint8_t fFlags; // 0 for now, maybe 1 bit means an ICC Profile follows: > 4-byte length + data (padded to 4 bytes?) > }; The implementation with Header sgtm, along with storing the length of the icc data. And I agree that returning the length from the API could be useful - but I'd rather not make that change until we have a caller that needs it. The current implementation on SkImageInfo uses read/writeData() APIs, so we already know that the blob we pass to Deserialize is just the serialized SkColorSpace.
I know I'm being picky, but it really sucks if we get the format wrong early one, and have to update .skps (and now chrome's serialization code). https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1213: struct ColorSpaceHeader { /* * Document (briefly is fine) how memory looks after the header based on flags... * * e.g. if (matrix_flag is set) then we write 9 floats (or whatever) */ https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1214: static constexpr uint8_t kMatrix_Flag = 0x1 << 0; nit: skia seems to always just use 1 << foo instead of 0x1 << foo https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1264: *((ColorSpaceHeader*) dataPtr) = ColorSpaceHeader::Pack(0, fNamed, fGammaNamed, Maybe an enum at the top of this file tracking the versions... enum Version { k0_Version, // Initial version: header + flags for matrix and profile }; https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1319: if (ColorSpaceHeader::kMatrix_Flag != header.fFlags || length < 16 * sizeof(float)) { do we really need all 16? Are there values we should never see?
> I know I'm being picky, but it really sucks if we get the format wrong early > one, and have to update .skps (and now chrome's serialization code). Seems like good comments. Now we only write 12 floats. I've also added convenience functions to SkMatrix44, and I've added a test. https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1213: struct ColorSpaceHeader { On 2016/06/21 18:21:22, reed1 wrote: > /* > * Document (briefly is fine) how memory looks after the header based on flags... > * > * e.g. if (matrix_flag is set) then we write 9 floats (or whatever) > */ Done. https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1214: static constexpr uint8_t kMatrix_Flag = 0x1 << 0; On 2016/06/21 18:21:23, reed1 wrote: > nit: skia seems to always just use 1 << foo instead of 0x1 << foo Done. https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1264: *((ColorSpaceHeader*) dataPtr) = ColorSpaceHeader::Pack(0, fNamed, fGammaNamed, On 2016/06/21 18:21:22, reed1 wrote: > Maybe an enum at the top of this file tracking the versions... > > enum Version { > k0_Version, // Initial version: header + flags for matrix and profile > }; Done. I put it at the top of the serialization code. Did you necessarily want it at the top of the file? https://codereview.chromium.org/2085653003/diff/80001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:1319: if (ColorSpaceHeader::kMatrix_Flag != header.fFlags || length < 16 * sizeof(float)) { On 2016/06/21 18:21:23, reed1 wrote: > do we really need all 16? Are there values we should never see? We can get away with 12, I've fixed this. I'm tempted to go for 9, but there technically *could* be a profile somewhere with an easily recognizable gamma and twelve matrix values... Do you think this matters enough to go for 9? I thought it was so irrelevant that 16 was fine.
Looking good. Possibly we'll want create SkColorSpace_serialize.cpp or something if this gets much bigger, just to break the file into manageable chunks. Two follow-on thoughts: 1. Sure seems like we should get ready to fuzz this guy (using Kevin's stuff I presume) 2. Given the desire to compare two colorspaces, perhaps we could use this code (or a shared subset) to perform a "deep-compare" ...? https://codereview.chromium.org/2085653003/diff/100001/include/core/SkMatrix44.h File include/core/SkMatrix44.h (right): https://codereview.chromium.org/2085653003/diff/100001/include/core/SkMatrix4... include/core/SkMatrix44.h:321: * perspective components (with [3][3] set to 1). */ Lets make these private helpers at first. Not sure this should be a public helper yet. https://codereview.chromium.org/2085653003/diff/100001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/100001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:1296: return SkData::MakeEmpty(); Is this an error condition? Should we return nullptr instead? https://codereview.chromium.org/2085653003/diff/100001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:1310: memcpy(dataPtr, as_CSB(this)->fProfileData->data(), profileSize); The allocation was align4. Does this memcpy always completely write to that allocation? (i.e. is profileSize always align4?)
msarett@google.com changed reviewers: + kjlubick@google.com
> Two follow-on thoughts: > 1. Sure seems like we should get ready to fuzz this guy (using Kevin's stuff I > presume) Adding Kevin. I imagine we can get a fuzz set-up after this lands. > 2. Given the desire to compare two colorspaces, perhaps we could use this code > (or a shared subset) to perform a "deep-compare" ...? Not sure what you mean by "this code". Maybe just the structure, i.e. first check Named, then check GammaNamed + matrix, then it gets harder etc. But yeah I think I need to make an attempt to provide an API for comparisons - maybe coming next? https://codereview.chromium.org/2085653003/diff/100001/include/core/SkMatrix44.h File include/core/SkMatrix44.h (right): https://codereview.chromium.org/2085653003/diff/100001/include/core/SkMatrix4... include/core/SkMatrix44.h:321: * perspective components (with [3][3] set to 1). */ On 2016/06/21 20:45:01, reed1 wrote: > Lets make these private helpers at first. Not sure this should be a public > helper yet. sgtm, done. https://codereview.chromium.org/2085653003/diff/100001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/2085653003/diff/100001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:1296: return SkData::MakeEmpty(); On 2016/06/21 20:45:01, reed1 wrote: > Is this an error condition? Should we return nullptr instead? Yes this an error. It's convenient to return an empty data because writeDataAsByteArray(nullptr) crashes, but writeDataAsByteArray(<empty data>) does what we want. But I think it's better to return nullptr here and write a little extra code in SkImageInfo. https://codereview.chromium.org/2085653003/diff/100001/src/core/SkColorSpace.... src/core/SkColorSpace.cpp:1310: memcpy(dataPtr, as_CSB(this)->fProfileData->data(), profileSize); On 2016/06/21 20:45:01, reed1 wrote: > The allocation was align4. Does this memcpy always completely write to that > allocation? (i.e. is profileSize always align4?) No it doesn't. I guess this is a problem - I'll fill with zeros. FWIW, ICC profiles are supposed to always be 4-byte aligned, but we don't really enforce that (and I don't think there's any reason too).
https://codereview.chromium.org/2085653003/diff/120001/include/core/SkColorSp... File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2085653003/diff/120001/include/core/SkColorSp... include/core/SkColorSpace.h:81: * Returns nullptr on failure. Fails when we fallback to serializing ICC data and Also meant to mention that I changed these comments.
lgtm
Thinking about "serializing" SkImageInfo as well (after looking at the chrome code), we may end up with a lower-level function as well, taking either WriteBuffer or Stream or something, so that we can efficiently serialize a colorspace object mid-stream along with other stuff. For now, I'm fine with the SkData signature, since it can remain even if we do add a lower-level version. Really, the thing I think chrome will want is serializing Bitmap and Image, which in turn have infos (and other things). We should study their code more.
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/patch-status/2085653003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
lgtm https://codereview.chromium.org/2085653003/diff/120001/include/core/SkColorSp... File include/core/SkColorSpace.h (right): https://codereview.chromium.org/2085653003/diff/120001/include/core/SkColorSp... include/core/SkColorSpace.h:86: static sk_sp<SkColorSpace> Deserialize(const void* data, size_t length); That's a perfect method signature to fuzz. I'll be able to add a fuzzer easily.
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/patch-status/2085653003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
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/patch-status/2085653003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/patch-status/2085653003/180001
The CQ bit was unchecked by msarett@google.com
Patchset #9 (id:180001) has been deleted
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/patch-status/2085653003/200001
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/patch-status/2085653003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from brianosman@google.com, kjlubick@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2085653003/#ps200001 (title: "Fix unit tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2085653003/200001
Message was sent while issue was closed.
Description was changed from ========== Enable flattening and unflattening of SkColorSpace BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2085653003 ========== to ========== Enable flattening and unflattening of SkColorSpace BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2085653003 Committed: https://skia.googlesource.com/skia/+/111a42d9cebf0bb8844c5d24f67fac57cc619d29 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://skia.googlesource.com/skia/+/111a42d9cebf0bb8844c5d24f67fac57cc619d29 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
