Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
444433002:
Initial ASTC decoder -- currently only supports 2D LDR decomrpession modes. (Closed)
|
Created:
6 years, 4 months ago by krajcevski Modified:
6 years, 4 months ago Reviewers:
robertphillips CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionInitial ASTC decoder -- currently only supports 2D LDR decomrpession modes.
Committed: https://skia.googlesource.com/skia/+/3c7edda88e275bcdaeb5d3afd0428a21f1635d13
Patch Set 1 #
Total comments: 124
Patch Set 2 : Code review comments #Patch Set 3 : Fix bugs from refactoring #Patch Set 4 : Change wording on comments #
Total comments: 32
Patch Set 5 : Code review comments #Patch Set 6 : Fix compiler errors #Patch Set 7 : Use ifdef instead of if #Messages
Total messages: 21 (0 generated)
Sorry
https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... File src/utils/SkTextureCompressor_ASTC.cpp (right): https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:275: // to efficiently operate on the block using bitwise operations. Skia-fy this? Maybe ASTCBlock ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:283: static inline void write_constant_color(uint8_t* dst, int dstRowBytes, SkColor color) { 12? i,j -> x,y ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:315: static uint64_t read_astc_bits(const astcBlock_t &block, int from, int to) { Assert from & to are >= 0 && < 128 ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:325: I don't think we need these two asserts. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:329: uint64_t result = 0; // remember: the 'to' bit isn't read https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:330: if (to <= 64) { // All desired bits are in low ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:332: } else if (from >= 64) { // All desired bits are in high ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:336: SkASSERT(nBits > (64 - from)); It seems like you should compute nLow first - clarity-wise. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:370: Would this make more sense as a method on astcBlock_t ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:372: // 128 bit block the MSB. x -> block ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:373: static inline void reverse_block(astcBlock_t *x) { uint64_t newLow = reverse64(block->high); block->high = reverse64(block->low); block->low = newLow; ?? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:378: SkIsPow2 in SkMath.h ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:383: Could SkNextLog2 serve here ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:397: Use SkClampMax ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:403: // Helper function defined in the ASTC spec, section C.2.14 // It ... ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:414: // Helper function defined in the ASTC spec, section C.2.14 // It ... ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:433: // Returns the bits in the given range, inclusive Why not start then end ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:447: Is there a better name then 'm'? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:451: } else { // The trits are arranged ... https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:458: Blarg for the rest of this function https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:509: trit -> quint ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:517: } else { // The quints are arranged ... https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:522: Blarg https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:579: // - Similarly, if the range is of the form 5*2^n, then the sequence is stored as a trits -> quints ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:580: // sequence of blocks, each block contains 3 trits and 3 bit sequences, which 5 -> 3 ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:587: int startBit, // The bit from which we're going to do the reading inclusive or not ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:653: // numbers... meant to match the ASTC hardware. This function is used to unquantize ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:663: replicated -> replicate ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:672: } Add assert that no bits are set outside of desired precision? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:889: // for which there is only a limited subset of valid values. such as ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1828: const ASTCDecompressionData &data) { 0x100 ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1857: // Decode the texel weights. 64? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1860: texelValues, 64, data.numWeights(), 128? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1900: dst += data.fDimY * dstRowBytes; i,j -> x,y ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1936: int width, int height, int blockDimX, int blockDimY) { // ASTC is encoded upside down so we need to walk backwards through the destination buffer ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1939: ASTCDecompressionData data(blockDimX, blockDimY); Do we need (or already have) and assert somewhere on height being a multiple of blockDimY? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1943: for (int i = 0; i < width; i += blockDimX) { Should we just pass in data here (rather then data.fBlock) and have read_astc_block handle things? Also should read_astc_block be a member function of ASTCDecompressionBlock? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1946: decompress_astc_block(reinterpret_cast<uint8_t*>(colorPtr + i), dstRowBytes, data); 16?
Next batch https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... File src/utils/SkTextureCompressor_ASTC.cpp (right): https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:654: // by unquantizing both colors (Table C.2.16) and weights (Table C.2.26) Is templating on a mask really useful? Since its inline shouldn't the compiler be able to figure things out if you pass in the mask as a parameter. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:691: static const int Cvals[6] = { 204, 93, 44, 22, 11, 5 }; Move these asserts to the start of the function? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:703: In Skia this is formatted as: case kGaussian_Filter: { Bitmap srcCopy = src->makeCopy(); ... break; } https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:798: // in place. This follows the algorithm laid out in section C.2.13 of the ASTC spec. This seems a bit odd. It seems like you actually want 3 functions and a switch in the caller function to decode the bits, trits or quints. I.e., the nBits, nTrits and nQuints values are only used as exclusive flags. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:802: SkASSERT(nQuints == 0); extra space after ',' ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:913: static const int kMaxPartitions = 4; // The possible CEM values are ... ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:925: int numWeights() const { Add a space before the '?' ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:928: #ifdef SK_DEBUG ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:941: } #endif ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1026: // LDR Luminance, direct Can these ints be happy, happy enum values ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1027: case 0: formatting ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1099: case 8: Could 8 & 12 share a sub-routine ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1119: case 9: Could 9 & 13 share a subroutine ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1155: endpoints[i][0] = SkColorSetARGB( Can we replace clamp_byte(c[0][0]) and clamp_byte(c[1][0]) with 0xFF ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1378: if (fDualPlaneEnabled) { space before '?' ?
Last set https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... File src/utils/SkTextureCompressor_ASTC.cpp (right): https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1444: // Do weight infill... Is there a reason to use s,t over x,y ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1446: for (int s = 0; s < fDimX; ++s) { Would it be clearer to unroll the plane loop and just have: texelWeights[0][s][t] = this->infillWeight(unquantizedValues, s, t, false); if (fDualPlaneEnabled) { texelWeights[1][s][t] = this->infillWeight(unquantizedValues, s, t, true); } https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1460: // Returns the partition for the texel located at position (x, y). Adapted from ... ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1462: int getPartition(int x, int y) const { bSmallBlock -> isSmallBlock ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1583: fError = true; Can we early out here? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1714: // This code corresponds to table C.2.8 of the ASTC spec highPrecision ? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1724: Would it be any better to define these two as flags (e.g., kDualPlaneBit) and then just check if they are set in blockMode? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1729: default: Do these 0..4 values have some clear symolic interpretation we could use for a enum? If not - maybe some comments?
https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... File src/utils/SkTextureCompressor_ASTC.cpp (right): https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:275: // to efficiently operate on the block using bitwise operations. On 2014/08/05 15:29:03, robertphillips wrote: > Skia-fy this? Maybe ASTCBlock ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:283: static inline void write_constant_color(uint8_t* dst, int dstRowBytes, SkColor color) { On 2014/08/05 15:29:03, robertphillips wrote: > 12? i,j -> x,y ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:315: static uint64_t read_astc_bits(const astcBlock_t &block, int from, int to) { On 2014/08/05 15:29:02, robertphillips wrote: > Assert from & to are >= 0 && < 128 ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:325: On 2014/08/05 15:29:03, robertphillips wrote: > I don't think we need these two asserts. Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:329: uint64_t result = 0; On 2014/08/05 15:29:04, robertphillips wrote: > // remember: the 'to' bit isn't read Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:330: if (to <= 64) { On 2014/08/05 15:29:02, robertphillips wrote: > // All desired bits are in low ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:332: } else if (from >= 64) { On 2014/08/05 15:29:02, robertphillips wrote: > // All desired bits are in high ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:336: SkASSERT(nBits > (64 - from)); On 2014/08/05 15:29:02, robertphillips wrote: > It seems like you should compute nLow first - clarity-wise. Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:370: On 2014/08/05 15:29:02, robertphillips wrote: > Would this make more sense as a method on astcBlock_t ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:372: // 128 bit block the MSB. On 2014/08/05 15:29:03, robertphillips wrote: > x -> block ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:373: static inline void reverse_block(astcBlock_t *x) { On 2014/08/05 15:29:03, robertphillips wrote: > uint64_t newLow = reverse64(block->high); > block->high = reverse64(block->low); > block->low = newLow; > > ?? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:378: On 2014/08/05 15:29:02, robertphillips wrote: > SkIsPow2 in SkMath.h ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:383: On 2014/08/05 15:29:04, robertphillips wrote: > Could SkNextLog2 serve here ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:397: On 2014/08/05 15:29:02, robertphillips wrote: > Use SkClampMax ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:403: // Helper function defined in the ASTC spec, section C.2.14 On 2014/08/05 15:29:03, robertphillips wrote: > // It ... ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:414: // Helper function defined in the ASTC spec, section C.2.14 On 2014/08/05 15:29:03, robertphillips wrote: > // It ... ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:433: // Returns the bits in the given range, inclusive On 2014/08/05 15:29:03, robertphillips wrote: > Why not start then end ? This way the code below matches the spec better. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:447: On 2014/08/05 15:29:03, robertphillips wrote: > Is there a better name then 'm'? m is chosen to match the naming in the spec. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:451: } else { On 2014/08/05 15:29:02, robertphillips wrote: > // The trits are arranged ... Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:458: On 2014/08/05 15:29:03, robertphillips wrote: > Blarg for the rest of this function I tried to match this function to the spec as much as possible. It might be made better using look-up tables? https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:509: On 2014/08/05 15:29:02, robertphillips wrote: > trit -> quint ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:517: } else { On 2014/08/05 15:29:02, robertphillips wrote: > // The quints are arranged ... Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:522: On 2014/08/05 15:29:03, robertphillips wrote: > Blarg Blarg. :( https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:579: // - Similarly, if the range is of the form 5*2^n, then the sequence is stored as a On 2014/08/05 15:29:02, robertphillips wrote: > trits -> quints ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:580: // sequence of blocks, each block contains 3 trits and 3 bit sequences, which On 2014/08/05 15:29:02, robertphillips wrote: > 5 -> 3 ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:587: int startBit, // The bit from which we're going to do the reading On 2014/08/05 15:29:03, robertphillips wrote: > inclusive or not ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:653: // numbers... meant to match the ASTC hardware. This function is used On 2014/08/05 15:29:02, robertphillips wrote: > to unquantize ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:654: // by unquantizing both colors (Table C.2.16) and weights (Table C.2.26) On 2014/08/05 17:30:01, robertphillips wrote: > Is templating on a mask really useful? Since its inline shouldn't the compiler > be able to figure things out if you pass in the mask as a parameter. Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:663: On 2014/08/05 15:29:04, robertphillips wrote: > replicated -> replicate ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:672: } On 2014/08/05 15:29:03, robertphillips wrote: > Add assert that no bits are set outside of desired precision? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:691: static const int Cvals[6] = { 204, 93, 44, 22, 11, 5 }; On 2014/08/05 17:30:00, robertphillips wrote: > Move these asserts to the start of the function? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:703: On 2014/08/05 17:30:00, robertphillips wrote: > In Skia this is formatted as: > > case kGaussian_Filter: { > Bitmap srcCopy = src->makeCopy(); > ... > break; > } Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:798: // in place. This follows the algorithm laid out in section C.2.13 of the ASTC spec. On 2014/08/05 17:30:00, robertphillips wrote: > This seems a bit odd. It seems like you actually want 3 functions and a switch > in the caller function to decode the bits, trits or quints. I.e., the nBits, > nTrits and nQuints values are only used as exclusive flags. nBits is a value that needs to be passed to either the trit or quint decoding function. The way integer sequences work, there can only ever be either 1 quint or 1 trit per value. I split them off into this function in order to keep the flow clear in the caller. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:802: SkASSERT(nQuints == 0); On 2014/08/05 17:30:00, robertphillips wrote: > extra space after ',' ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:889: // for which there is only a limited subset of valid values. On 2014/08/05 15:29:02, robertphillips wrote: > such as ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:913: static const int kMaxPartitions = 4; On 2014/08/05 17:30:00, robertphillips wrote: > // The possible CEM values are ... ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:925: int numWeights() const { On 2014/08/05 17:30:01, robertphillips wrote: > Add a space before the '?' ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:928: On 2014/08/05 17:30:00, robertphillips wrote: > #ifdef SK_DEBUG ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:941: } On 2014/08/05 17:30:00, robertphillips wrote: > #endif ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1026: // LDR Luminance, direct On 2014/08/05 17:30:00, robertphillips wrote: > Can these ints be happy, happy enum values ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1027: case 0: On 2014/08/05 17:30:00, robertphillips wrote: > formatting ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1099: case 8: On 2014/08/05 17:30:00, robertphillips wrote: > Could 8 & 12 share a sub-routine ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1119: case 9: On 2014/08/05 17:30:00, robertphillips wrote: > Could 9 & 13 share a subroutine ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1155: endpoints[i][0] = SkColorSetARGB( On 2014/08/05 17:30:00, robertphillips wrote: > Can we replace clamp_byte(c[0][0]) and clamp_byte(c[1][0]) with 0xFF ? Not if they're in their own subroutine. :) https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1378: if (fDualPlaneEnabled) { On 2014/08/05 17:30:01, robertphillips wrote: > space before '?' ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1444: // Do weight infill... On 2014/08/05 20:37:53, robertphillips wrote: > Is there a reason to use s,t over x,y ? s,t are what's used in the spec. They also map better to texture coordinates? I changed it anyway. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1446: for (int s = 0; s < fDimX; ++s) { On 2014/08/05 20:37:53, robertphillips wrote: > Would it be clearer to unroll the plane loop and just have: > > texelWeights[0][s][t] = this->infillWeight(unquantizedValues, s, t, false); > > if (fDualPlaneEnabled) { > texelWeights[1][s][t] = this->infillWeight(unquantizedValues, s, t, true); > } Yes, I think my brain was fried by this point. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1460: // Returns the partition for the texel located at position (x, y). On 2014/08/05 20:37:53, robertphillips wrote: > Adapted from ... ? Not sure what you mean? The next comment says where it's from. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1462: int getPartition(int x, int y) const { On 2014/08/05 20:37:53, robertphillips wrote: > bSmallBlock -> isSmallBlock ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1583: fError = true; On 2014/08/05 20:37:53, robertphillips wrote: > Can we early out here? Yes. If fError is true then none of the other values should be considered valid anyway. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1714: // This code corresponds to table C.2.8 of the ASTC spec On 2014/08/05 20:37:53, robertphillips wrote: > highPrecision ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1724: On 2014/08/05 20:37:53, robertphillips wrote: > Would it be any better to define these two as flags (e.g., kDualPlaneBit) and > then just check if they are set in blockMode? If life were easy, yes. Some block modes don't encode them in the mode itself. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1729: default: On 2014/08/05 20:37:53, robertphillips wrote: > Do these 0..4 values have some clear symolic interpretation we could use for a > enum? > If not - maybe some comments? I'm not sure that there's a meaningful way to comment this. I copied the relevant section of the spec into the comments above. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1828: const ASTCDecompressionData &data) { On 2014/08/05 15:29:03, robertphillips wrote: > 0x100 ? I think this is remains of a decompressor past. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1857: // Decode the texel weights. On 2014/08/05 15:29:03, robertphillips wrote: > 64? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1860: texelValues, 64, data.numWeights(), On 2014/08/05 15:29:02, robertphillips wrote: > 128? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1900: dst += data.fDimY * dstRowBytes; On 2014/08/05 15:29:03, robertphillips wrote: > i,j -> x,y ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1936: int width, int height, int blockDimX, int blockDimY) { On 2014/08/05 15:29:02, robertphillips wrote: > // ASTC is encoded upside down so we need to walk backwards through the > destination buffer ? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1939: ASTCDecompressionData data(blockDimX, blockDimY); On 2014/08/05 15:29:03, robertphillips wrote: > Do we need (or already have) and assert somewhere on height being a multiple of > blockDimY? Yes, that should be caught in SkTextureCompressor.cpp https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1943: for (int i = 0; i < width; i += blockDimX) { On 2014/08/05 15:29:02, robertphillips wrote: > Should we just pass in data here (rather then data.fBlock) and have > read_astc_block handle things? Also should read_astc_block be a member function > of ASTCDecompressionBlock? Done. https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1946: decompress_astc_block(reinterpret_cast<uint8_t*>(colorPtr + i), dstRowBytes, data); On 2014/08/05 15:29:02, robertphillips wrote: > 16? Done.
https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... File src/utils/SkTextureCompressor_ASTC.cpp (right): https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1460: // Returns the partition for the texel located at position (x, y). On 2014/08/05 20:55:05, krajcevski wrote: > On 2014/08/05 20:37:53, robertphillips wrote: > > Adapted from ... ? > > Not sure what you mean? The next comment says where it's from. I think we should replace "Copied directly from" with "Adapted from".
https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... File src/utils/SkTextureCompressor_ASTC.cpp (right): https://codereview.chromium.org/444433002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor_ASTC.cpp:1460: // Returns the partition for the texel located at position (x, y). On 2014/08/06 12:31:52, robertphillips wrote: > On 2014/08/05 20:55:05, krajcevski wrote: > > On 2014/08/05 20:37:53, robertphillips wrote: > > > Adapted from ... ? > > > > Not sure what you mean? The next comment says where it's from. > > I think we should replace "Copied directly from" with "Adapted from". Done.
lgtm modulo the 0 -> 0xff change. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor_ASTC.cpp (right): https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:272: // Enable this to debug decoding errors. If not set, errors will result in magenta images. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:305: // 128 bit block the MSB. lower case - since not static ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:307: const uint64_t newLow = reverse64(this->fHigh); Why have the newHigh temp? Why not plop it into fHigh? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:379: // Returns the number of bits needed to represent a number add "(excluding the power-of-two itself)" ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:407: // Helper function defined in the ASTC spec, section C.2.14 modulate -> tint ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:412: six or eight ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:438: six or eight ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:453: int v6 = isRGB ? 0xFF : v[6]; 0 -> 0xFF ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:486: endpoints[0] = SkColorSetARGB( tab these guys over? Maybe have them on two lines? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1200: switch (fCEM[i]) { Share subroutine with kLDR_LuminanceAlpha_Direct_ColorEndpointMode ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1251: Share subroutine with kLDR_RGB_BaseScaleWithAlpha_ColorEndpointMode? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1410: int getWeight(const int* unquantizedWeights, int idx, bool dualPlane) const { Space before '?' ? Do we need the parens around fDualPlaneEnabled ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1756: // A, B ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1758: // H, R - used to determine the number of bits/trits/quints in texel weight encoding R0, R1, R2 ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1911: space after "if" ? https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:2010: ASTCDecompressionData data(blockDimX, blockDimY); i,j -> x,y ?
https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor_ASTC.cpp (right): https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:272: On 2014/08/06 17:24:21, robertphillips wrote: > // Enable this to debug decoding errors. If not set, errors will result in > magenta images. Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:305: // 128 bit block the MSB. On 2014/08/06 17:24:21, robertphillips wrote: > lower case - since not static ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:307: const uint64_t newLow = reverse64(this->fHigh); On 2014/08/06 17:24:20, robertphillips wrote: > Why have the newHigh temp? Why not plop it into fHigh? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:379: // Returns the number of bits needed to represent a number On 2014/08/06 17:24:21, robertphillips wrote: > add "(excluding the power-of-two itself)" ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:407: // Helper function defined in the ASTC spec, section C.2.14 On 2014/08/06 17:24:21, robertphillips wrote: > modulate -> tint ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:412: On 2014/08/06 17:24:21, robertphillips wrote: > six or eight ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:438: On 2014/08/06 17:24:20, robertphillips wrote: > six or eight ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:453: int v6 = isRGB ? 0xFF : v[6]; On 2014/08/06 17:24:20, robertphillips wrote: > 0 -> 0xFF ? It's supposed to be 0 because it's added to v6. The resulting values shouldn't go over 0xFF. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:486: endpoints[0] = SkColorSetARGB( On 2014/08/06 17:24:20, robertphillips wrote: > tab these guys over? Maybe have them on two lines? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1200: switch (fCEM[i]) { On 2014/08/06 17:24:20, robertphillips wrote: > Share subroutine with kLDR_LuminanceAlpha_Direct_ColorEndpointMode ? This will probably create more upkeep than it saves. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1251: On 2014/08/06 17:24:21, robertphillips wrote: > Share subroutine with kLDR_RGB_BaseScaleWithAlpha_ColorEndpointMode? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1410: int getWeight(const int* unquantizedWeights, int idx, bool dualPlane) const { On 2014/08/06 17:24:21, robertphillips wrote: > Space before '?' ? > Do we need the parens around fDualPlaneEnabled ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1756: // On 2014/08/06 17:24:20, robertphillips wrote: > A, B ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1758: // H, R - used to determine the number of bits/trits/quints in texel weight encoding On 2014/08/06 17:24:21, robertphillips wrote: > R0, R1, R2 ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:1911: On 2014/08/06 17:24:21, robertphillips wrote: > space after "if" ? Done. https://codereview.chromium.org/444433002/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor_ASTC.cpp:2010: ASTCDecompressionData data(blockDimX, blockDimY); On 2014/08/06 17:24:20, robertphillips wrote: > i,j -> x,y ? Done.
The CQ bit was checked by krajcevski@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/krajcevski@google.com/444433002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)
The CQ bit was checked by krajcevski@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/krajcevski@google.com/444433002/100001
Message was sent while issue was closed.
Change committed as 1840dcd2f1368279df907988a9b584ffd500de4c
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/444103002/ by krajcevski@google.com. The reason for reverting is: Breaking chrome..
The CQ bit was checked by krajcevski@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/krajcevski@google.com/444433002/120001
Message was sent while issue was closed.
Change committed as 3c7edda88e275bcdaeb5d3afd0428a21f1635d13 |
