Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2117773002:
Refactor parsing and storage of SkGammas (Closed)
DescriptionRefactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed: https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
Committed: https://skia.googlesource.com/skia/+/959ccc1f3f49e1ddeb51c32c30ac4a2d94653856
Committed: https://skia.googlesource.com/skia/+/1b93bd1e6eba3d14593490e4e24a34546638c8da
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added a test #Patch Set 3 : Use a Type enum instead of TypeMask #
Total comments: 43
Patch Set 4 : Response to comments #
Total comments: 9
Patch Set 5 : Fix nits #Patch Set 6 : Rebase #Patch Set 7 : Fix win #Patch Set 8 : Fixed memory errors #Patch Set 9 : Read fixed 0.16 properly #
Depends on Patchset: Messages
Total messages: 59 (40 generated)
Description was changed from ========== Refactor parsing and storage of SkGammas BUG=skia: ========== to ========== Refactor parsing and storage of SkGammas BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Refactor parsing and storage of SkGammas BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002 ========== to ========== Refactor parsing and storage of SkGammas Benefits: (1) Parses and stores gamma tags in a single allocation. (2) Recognizes equal gamma tags to skip parsing work and save memory. Non-Benefits: (1) Not less complicated. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002 ==========
Patchset #1 (id:40001) has been deleted
msarett@google.com changed reviewers: + brianosman@google.com, reed@google.com
No need to look until after the Android rush... https://codereview.chromium.org/2117773002/diff/60001/src/core/SkColorSpace_I... File src/core/SkColorSpace_ICC.cpp (right): https://codereview.chromium.org/2117773002/diff/60001/src/core/SkColorSpace_I... src/core/SkColorSpace_ICC.cpp:273: static SkGammas::TypeMask parse_gamma(SkGammas::Data* outData, SkGammas::Params* outParams, This isn't too different than the old "load_gammas()": *** Only parses one gamma (rather than looping) *** Returns the gamma type and skips parsing of tables until later. *** Stores the length of the tag in an out variable.
Added a test.
https://codereview.chromium.org/2117773002/diff/60001/src/core/SkColorSpace_B... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2117773002/diff/60001/src/core/SkColorSpace_B... src/core/SkColorSpace_Base.h:17: // There are four possible representations for gamma curves. These are mutually exclusive, right? Having them defined like flags (and named 'Mask') makes it seem like you could have more than one value. How about: enum class Type : uint_t { kNone, kNamed, kValue, kTable, kParam };
https://codereview.chromium.org/2117773002/diff/60001/src/core/SkColorSpace_B... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2117773002/diff/60001/src/core/SkColorSpace_B... src/core/SkColorSpace_Base.h:17: // There are four possible representations for gamma curves. On 2016/07/12 19:27:20, Brian Osman wrote: > These are mutually exclusive, right? Having them defined like flags (and named > 'Mask') makes it seem like you could have more than one value. How about: > > enum class Type : uint_t { kNone, kNamed, kValue, kTable, kParam }; Yeah that's way better. Changed to using a Type enum.
Please take a look! https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... File src/core/SkColorSpace_ICC.cpp (right): https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:273: static SkGammas::Type parse_gamma(SkGammas::Data* outData, SkGammas::Params* outParams, This isn't too different than the old "load_gammas()": *** Only parses one gamma (rather than looping) *** Returns the gamma type and skips parsing of tables until later. *** Stores the length of the tag in an out variable.
msarett@google.com changed reviewers: + mtklein@google.com
Adding mtklein.
First pass. Haven't looked at src/core/SkColorSpace_ICC.cpp yet. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:569: if (!memcmp(&gammas->data(0), &gammas->data(i), sizeof(SkGammas::Data))) { Probably best to write as 0 ==. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:629: // Check if this curve matches the first curve. In this case, we can Side question: is there any way to cleanly share the skeleton here between src and dst gammas? They look a lot alike. On of my rues of thumb is that writing the same code twice is probably fine, but writing the same comment twice is probably an indication that we need some refactoring. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:32: // point us to the start of the table. Is there a way to replace this comment with a method here or on Data that returns the start of the table, either calculated from this or from a passed-in pointer? (Same for kParamType.)
https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... File src/core/SkColorSpace_ICC.cpp (right): https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:29: static uint16_t read_big_endian_short(const uint8_t* ptr) { These might be clearer as read_big_endian_{u16,u32,i32}. No guessing about size or signedness. _short has been especially tricky for me to remember means uint16_t. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:287: uint32_t count = read_big_endian_uint(src + 8); What's in bytes 4-7? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:302: set_tag_bytes(outTagBytes, tagBytes); This one might be clearer inlined. Putting it behind a named function sort of makes it seem like it's going to be more complicated than it really is. It'd be even simpler if we just make outTagBytes required to be non-null. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:344: // gamma table. any idea about its origin? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:394: return set_gamma_value(outData, SkFixedToFloat(g)); Are we ever going to load these 16.16 values and not convert them to float? Feels like this'd all be easier to read with another method, static float read_big_endian_16_dot_16(const uint8_t buf[4]) { // It just so happens that SkFixed is also 16.16! return SkFixedToFloat(read_big_endian_int(buf)); } https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:405: switch(format) { Can we slim this down by factoring out parts that are always true? E.g., seems like GAB are always at 12,16,and 20. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:442: d = -b / a; Might be clearer to move the (0.0f == a) check right before this. Or maybe, just let the bad stuff propagate through to the end with your other degenerate d checks? if (!isfinite(d)) { return kNone; } You could even get fancy and only return kNone for NaN... +inf and -inf actually seem interpretable. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:453: // Y = cX otherwise eX? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:456: b = SkFixedToFloat(read_big_endian_int(src + 20)); I think it might make things clearer here to see an explicit c = 0 between b and d. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:457: d = SkFixedToFloat(read_big_endian_int(src + 28)); This +20, +24, +28 inversion is pretty unusual.. looks like a bug. If not, note that it's not a bug? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:559: static void handle_invalid_gamma(SkGammas::Type* Type, SkGammas::Data* data) { Type is a weird parameter name. Is there a reason we can't use type for these? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:562: data->fNamed = SkColorSpace::kLinear_GammaNamed; what decides the default is linear, not say, srgb? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:569: static uint32_t load_gammas(void* memory, uint32_t offset, SkGammas::Type Type, don't these uint32_t want to be size_t ? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:569: static uint32_t load_gammas(void* memory, uint32_t offset, SkGammas::Type Type, This function doesn't really explain itself, particularly around how offset is used and what's returned. More comments? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:584: for (uint32_t i = 0; i < data->fTable.fSize; i++) { size_t or int? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:585: outTable[i] = (read_big_endian_short((const uint8_t*) &inTable[i])) / 65535.0f; These read_ ... functions could alternatively take const void* https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:844: if (a->fOffset == b-> fOffset) { stray space? https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:1175: sk_sp<SkData> SkColorSpace_Base::writeToICC() const { Just curious, what use case has us writing these?
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Thanks for the review! I've responded to comments. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:569: if (!memcmp(&gammas->data(0), &gammas->data(i), sizeof(SkGammas::Data))) { On 2016/07/17 13:58:19, mtklein wrote: > Probably best to write as 0 ==. Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:629: // Check if this curve matches the first curve. In this case, we can On 2016/07/17 13:58:19, mtklein wrote: > Side question: is there any way to cleanly share the skeleton here between src > and dst gammas? They look a lot alike. On of my rues of thumb is that writing > the same code twice is probably fine, but writing the same comment twice is > probably an indication that we need some refactoring. I've refactored to share this code. I think it looks better now. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:32: // point us to the start of the table. On 2016/07/17 13:58:19, mtklein wrote: > Is there a way to replace this comment with a method here or on Data that > returns the start of the table, either calculated from this or from a passed-in > pointer? (Same for kParamType.) Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... File src/core/SkColorSpace_ICC.cpp (right): https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:29: static uint16_t read_big_endian_short(const uint8_t* ptr) { On 2016/07/18 17:18:54, mtklein wrote: > These might be clearer as read_big_endian_{u16,u32,i32}. No guessing about size > or signedness. _short has been especially tricky for me to remember means > uint16_t. Acknowledged. Will follow-up on this CL. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:287: uint32_t count = read_big_endian_uint(src + 8); On 2016/07/18 17:18:54, mtklein wrote: > What's in bytes 4-7? "Reserved. Shall be set to 0." Adding a comment. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:302: set_tag_bytes(outTagBytes, tagBytes); On 2016/07/18 17:18:55, mtklein wrote: > This one might be clearer inlined. Putting it behind a named function sort of > makes it seem like it's going to be more complicated than it really is. It'd be > even simpler if we just make outTagBytes required to be non-null. Now requiring outTagBytes to be non-null. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:344: // gamma table. On 2016/07/18 17:18:54, mtklein wrote: > any idea about its origin? Yeah lcms again. Improving these comments. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:394: return set_gamma_value(outData, SkFixedToFloat(g)); On 2016/07/18 17:18:55, mtklein wrote: > Are we ever going to load these 16.16 values and not convert them to float? > Feels like this'd all be easier to read with another method, > > static float read_big_endian_16_dot_16(const uint8_t buf[4]) { > // It just so happens that SkFixed is also 16.16! > return SkFixedToFloat(read_big_endian_int(buf)); > } Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:405: switch(format) { On 2016/07/18 17:18:54, mtklein wrote: > Can we slim this down by factoring out parts that are always true? E.g., seems > like GAB are always at 12,16,and 20. Hmm yeah. Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:442: d = -b / a; On 2016/07/18 17:18:54, mtklein wrote: > Might be clearer to move the (0.0f == a) check right before this. > > Or maybe, just let the bad stuff propagate through to the end with your other > degenerate d checks? > > if (!isfinite(d)) { > return kNone; > } > > You could even get fancy and only return kNone for NaN... +inf and -inf actually > seem interpretable. Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:453: // Y = cX otherwise On 2016/07/18 17:18:54, mtklein wrote: > eX? Uhhh ya. The spec calls it "c" but I call it "e" to be consistent with the uniform format. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:456: b = SkFixedToFloat(read_big_endian_int(src + 20)); On 2016/07/18 17:18:54, mtklein wrote: > I think it might make things clearer here to see an explicit c = 0 between b and > d. Acknowledged. I've refactored for g, a, b, are defined above. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:457: d = SkFixedToFloat(read_big_endian_int(src + 28)); On 2016/07/18 17:18:54, mtklein wrote: > This +20, +24, +28 inversion is pretty unusual.. looks like a bug. If not, > note that it's not a bug? Not a bug... I've added a comment. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:559: static void handle_invalid_gamma(SkGammas::Type* Type, SkGammas::Data* data) { On 2016/07/18 17:18:54, mtklein wrote: > Type is a weird parameter name. Is there a reason we can't use type for these? I think this is a bad case of find and replace... Fixed. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:562: data->fNamed = SkColorSpace::kLinear_GammaNamed; On 2016/07/18 17:18:54, mtklein wrote: > what decides the default is linear, not say, srgb? Hmmm let's make it sRGB. It's been linear since back when we were being a little more cautious about marking things as sRGB. But sRGB should be the default now. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:569: static uint32_t load_gammas(void* memory, uint32_t offset, SkGammas::Type Type, On 2016/07/18 17:18:54, mtklein wrote: > don't these uint32_t want to be size_t ? Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:569: static uint32_t load_gammas(void* memory, uint32_t offset, SkGammas::Type Type, On 2016/07/18 17:18:54, mtklein wrote: > This function doesn't really explain itself, particularly around how offset is > used and what's returned. More comments? Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:584: for (uint32_t i = 0; i < data->fTable.fSize; i++) { On 2016/07/18 17:18:54, mtklein wrote: > size_t or int? Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:585: outTable[i] = (read_big_endian_short((const uint8_t*) &inTable[i])) / 65535.0f; On 2016/07/18 17:18:54, mtklein wrote: > These read_ ... functions could alternatively take const void* Will consider in follow-up on read_ functions. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:844: if (a->fOffset == b-> fOffset) { On 2016/07/18 17:18:54, mtklein wrote: > stray space? Done. https://codereview.chromium.org/2117773002/diff/100001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:1175: sk_sp<SkData> SkColorSpace_Base::writeToICC() const { On 2016/07/18 17:18:54, mtklein wrote: > Just curious, what use case has us writing these? Implemented it to help with serialization. Then decided not to use it for serialization. Now I'm regretting that it exists (and it is becoming increasingly broken), but there is one caller... This exists until I decide to fix it or delete it.
lgtm https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:641: build_gamma_tables<float>(fSrcGammaTables, fSrcGammaTableStorage, 256, srcSpace, kToLinear); I suspect the <float> and <uint8_t> can be inferred if you don't want to type them. https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:17: // There are four possible representations for gamma curves. and then the enum has 5 values... https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... File src/core/SkColorSpace_ICC.cpp (right): https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:417: case kGAB_ParaCurveType: { Probably doesn't need {} ? https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:490: // d is NaN If you use isnan, there's no need for the comment: if (isnan(d)) { return SkGammas::Type::kNone_Type; } isnan(x) of course is implemented as x != x. https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:564: * Finish loading the gammas, now that we have allocated memory for the SkGammas struct. Thanks!
Thanks a bunch for this review. https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpaceX... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpaceX... src/core/SkColorSpaceXform.cpp:641: build_gamma_tables<float>(fSrcGammaTables, fSrcGammaTableStorage, 256, srcSpace, kToLinear); On 2016/07/20 13:13:05, mtklein wrote: > I suspect the <float> and <uint8_t> can be inferred if you don't want to type > them. Cool. Done. https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... File src/core/SkColorSpace_Base.h (right): https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... src/core/SkColorSpace_Base.h:17: // There are four possible representations for gamma curves. On 2016/07/20 13:13:05, mtklein wrote: > and then the enum has 5 values... kNone isn't a type, just means uninitialized. I'll improve the comment. https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... File src/core/SkColorSpace_ICC.cpp (right): https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:417: case kGAB_ParaCurveType: { On 2016/07/20 13:13:05, mtklein wrote: > Probably doesn't need {} ? Done. https://codereview.chromium.org/2117773002/diff/160001/src/core/SkColorSpace_... src/core/SkColorSpace_ICC.cpp:490: // d is NaN On 2016/07/20 13:13:05, mtklein wrote: > If you use isnan, there's no need for the comment: > > if (isnan(d)) { > return SkGammas::Type::kNone_Type; > } > > isnan(x) of course is implemented as x != x. Done.
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: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
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 mtklein@google.com Link to the patchset: https://codereview.chromium.org/2117773002/#ps220001 (title: "Fix win")
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
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
==========
to
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:220001) has been created in https://codereview.chromium.org/2159253005/ by msarett@google.com. The reason for reverting is: Broken bots..
Message was sent while issue was closed.
Description was changed from
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
==========
to
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
==========
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...
Patchset #8 (id:240001) has been deleted
Patchset #8 (id:260001) 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/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 mtklein@google.com Link to the patchset: https://codereview.chromium.org/2117773002/#ps280001 (title: "Fixed memory errors")
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
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
==========
to
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
Committed:
https://skia.googlesource.com/skia/+/959ccc1f3f49e1ddeb51c32c30ac4a2d94653856
==========
Message was sent while issue was closed.
Committed patchset #8 (id:280001) as https://skia.googlesource.com/skia/+/959ccc1f3f49e1ddeb51c32c30ac4a2d94653856
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:280001) has been created in https://codereview.chromium.org/2171623002/ by msarett@google.com. The reason for reverting is: Tests failing.
Message was sent while issue was closed.
Description was changed from
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
Committed:
https://skia.googlesource.com/skia/+/959ccc1f3f49e1ddeb51c32c30ac4a2d94653856
==========
to
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
Committed:
https://skia.googlesource.com/skia/+/959ccc1f3f49e1ddeb51c32c30ac4a2d94653856
==========
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 mtklein@google.com Link to the patchset: https://codereview.chromium.org/2117773002/#ps300001 (title: "Read fixed 0.16 properly")
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
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
Committed:
https://skia.googlesource.com/skia/+/959ccc1f3f49e1ddeb51c32c30ac4a2d94653856
==========
to
==========
Refactor parsing and storage of SkGammas
Benefits:
(1) Parses and stores gamma tags in a single allocation.
(2) Recognizes equal gamma tags to skip parsing work and
save memory.
Non-Benefits:
(1) Not less complicated.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2117773002
Committed:
https://skia.googlesource.com/skia/+/2ea944c2b710caf29d4795ac953bad14224796f7
Committed:
https://skia.googlesource.com/skia/+/959ccc1f3f49e1ddeb51c32c30ac4a2d94653856
Committed:
https://skia.googlesource.com/skia/+/1b93bd1e6eba3d14593490e4e24a34546638c8da
==========
Message was sent while issue was closed.
Committed patchset #9 (id:300001) as https://skia.googlesource.com/skia/+/1b93bd1e6eba3d14593490e4e24a34546638c8da |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
