Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1096703002:
Reland: Add ETC1 powered SSE encoder for tile texture compression (Closed)
|
Created:
5 years, 8 months ago by radu.velea Modified:
5 years, 7 months ago CC:
cc-bugs_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland: Add ETC1 powered SSE encoder for tile texture compression
Created an ETC1 encoder that uses SSE2 to improve compression speed.
The SSE encoder extends TextureCompressor and uses the same algorithm as TextureCompressorETC1.
Added unittest for TextureCompressorETC1.
Moved shared code into a etc1 header.
Added new performance test scenarios.
Reland necessary for 32-bit GN issues.
Performance difference on Ubuntu x64, Haswell Processor:
Without SSE:
*RESULT Compress256x256BlackAndWhiteGradientImage: ETC1 Low= 1.966321587562561 us
*RESULT Compress256x256SolidBlackImage: ETC1 Low= .0956009104847908 us
*RESULT Compress256x256SolidColorImage: ETC1 Low= .4367307722568512 us
*RESULT Compress256x256RandomColorImage: ETC1 Low= 5.948055744171143 us
With SSE:
*RESULT Compress256x256BlackAndWhiteGradientImage: ETC1 Low= 1.0316201448440552 us
*RESULT Compress256x256SolidBlackImage: ETC1 Low= .25716209411621094 us
*RESULT Compress256x256SolidColorImage: ETC1 Low= .2768038809299469 us
*RESULT Compress256x256RandomColorImage: ETC1 Low= 1.834145426750183 us
BUG=434699
TEST=newly added unittest TextureCompressorETC1Test::Compress256x256CreateETC1, TextureCompressorETC1Test::Compress256x256RatioETC1
Committed: https://crrev.com/5f3849aa8307399b7e6dfe5665ed149594244077
Cr-Commit-Position: refs/heads/master@{#329840}
Committed: https://crrev.com/648fb54ee219104d27e2a2de0806408ac0e7c75b
Cr-Commit-Position: refs/heads/master@{#329859}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Using similarity 0 #Patch Set 4 : --no-find-copies #
Total comments: 11
Patch Set 5 : Instantiating new class based on CPU; removing SSE4.1 functions and falling back to SSE2 #
Total comments: 3
Patch Set 6 : Improved performance; added conditional compile; moved redundant code to header. #Patch Set 7 : Changed some header paths. #Patch Set 8 : Adding missing header. #Patch Set 9 : Changed solid block compression to SSE #Patch Set 10 : Fix after running trybot #Patch Set 11 : Update gyp and Build.gn #
Total comments: 1
Patch Set 12 : Using ALIGNAS instead of attribute and remove pragma's #
Total comments: 1
Patch Set 13 : Adding unit test for texture compression #
Total comments: 3
Patch Set 14 : #Patch Set 15 : #
Total comments: 7
Patch Set 16 : Applying feedback #
Total comments: 103
Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #
Total comments: 34
Patch Set 20 : Updated comments, casts and other minor issues. #
Total comments: 8
Patch Set 21 : Sync Build.gn #
Total comments: 10
Patch Set 22 : Fixing comments #Patch Set 23 : Fix BUILD.gn check #Patch Set 24 : Reland: Change ia32 to x86 in Build.gn #
Total comments: 2
Messages
Total messages: 53 (17 generated)
radu.velea@intel.com changed reviewers: + robert.bradford@intel.com
adrian.belgun@intel.com changed reviewers: + adrian.belgun@intel.com
Added quick review. https://codereview.chromium.org/1096703002/diff/60001/cc/resources/texture_co... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/60001/cc/resources/texture_co... cc/resources/texture_compressor_etc1_sse.cc:103: /* 0, 1, 2 - for ARM */ Please check image channel order for input. This is more likely an input issue of RGB vs BGR than an endianness one. Also, for clarity, I recommend changing the assignment from block[2,1,0] = channels[r,g,b] to block[0,1,2] = channels[b,g,r]. https://codereview.chromium.org/1096703002/diff/60001/cc/resources/texture_co... cc/resources/texture_compressor_etc1_sse.cc:134: /* 0, 1, 2 - for ARM */ Same comments as for :103. https://codereview.chromium.org/1096703002/diff/60001/cc/resources/texture_co... cc/resources/texture_compressor_etc1_sse.cc:238: /**************************************** START OF SSE CODE Use only one line here. Reduce number of stars. https://codereview.chromium.org/1096703002/diff/60001/cc/resources/texture_co... cc/resources/texture_compressor_etc1_sse.cc:277: for (int i = 0; i < size; i++) { Braces are optional for single-statement loops. Consider removing them for increased readability. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi... https://codereview.chromium.org/1096703002/diff/60001/cc/resources/texture_co... cc/resources/texture_compressor_etc1_sse.cc:788: I think something went wrong with 'git cl format' in this function. Please review your comments in the source file.
robert.bradford@intel.com changed reviewers: - adrian.belgun@intel.com
Good first start! General comments: Try and reduce copy and pasting from the texture_compressor.c. Could you put that code into a common .h? Use inline assembler rather than intrinsics with a CPU id test (use base/cpu.h) and then no cflags changes will be needed? https://codereview.chromium.org/1096703002/diff/60001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/1096703002/diff/60001/cc/cc.gyp#newcode491 cc/cc.gyp:491: 'resources/texture_compressor_etc1_sse.h', You also need to add entries to the BUILD.gn file too. https://codereview.chromium.org/1096703002/diff/60001/cc/cc.gyp#newcode585 cc/cc.gyp:585: '-msse4.1', You can't do this as this allows the compiler to use optimisations that generate SSE4.1 instructions for this whole compilation block which might not be supported by the platform the code runs on. https://codereview.chromium.org/1096703002/diff/60001/cc/resources/texture_co... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/60001/cc/resources/texture_co... cc/resources/texture_compressor.cc:17: } Looks like you're missing instantiating your new class. Use the methods in base/cpu.h to work out when to use that new class.
Done https://codereview.chromium.org/1096703002/diff/60001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/1096703002/diff/60001/cc/cc.gyp#newcode491 cc/cc.gyp:491: 'resources/texture_compressor_etc1_sse.h', On 2015/04/17 14:04:47, robert.bradford wrote: > You also need to add entries to the BUILD.gn file too. Done. https://codereview.chromium.org/1096703002/diff/60001/cc/cc.gyp#newcode491 cc/cc.gyp:491: 'resources/texture_compressor_etc1_sse.h', On 2015/04/17 14:04:47, robert.bradford wrote: > You also need to add entries to the BUILD.gn file too. Done. https://codereview.chromium.org/1096703002/diff/60001/cc/cc.gyp#newcode585 cc/cc.gyp:585: '-msse4.1', On 2015/04/17 14:04:47, robert.bradford wrote: > You can't do this as this allows the compiler to use optimisations that generate > SSE4.1 instructions for this whole compilation block which might not be > supported by the platform the code runs on. Done.
https://codereview.chromium.org/1096703002/diff/80001/cc/resources/texture_co... File cc/resources/texture_compressor_etc1_sse.h (right): https://codereview.chromium.org/1096703002/diff/80001/cc/resources/texture_co... cc/resources/texture_compressor_etc1_sse.h:12: class CC_EXPORT TextureCompressorETC1_SSE : public TextureCompressor { This class name is weird - don't think is within the coding standards.
https://codereview.chromium.org/1096703002/diff/80001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/1096703002/diff/80001/cc/cc.gyp#newcode490 cc/cc.gyp:490: 'resources/texture_compressor_etc1_sse.cc', These files need to be conditional on the arch. https://codereview.chromium.org/1096703002/diff/80001/cc/resources/texture_co... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/80001/cc/resources/texture_co... cc/resources/texture_compressor.cc:19: return make_scoped_ptr(new TextureCompressorETC1()); What about this instead: scoped_ptr<TextureCompressor> compressor(new TextureCompressorETC1()); #if defined(__i386__) || defined(__x86_64__) if (cpu.has_sse2()) compressor.reset(new TextureCompresorETC1_SSE()); #endif return compressor;
radu.velea@intel.com changed reviewers: + christiank@opera.com
radu.velea@intel.com changed reviewers: + jie.a.chen@intel.com
https://codereview.chromium.org/1096703002/diff/200001/cc/resources/texture_c... File cc/resources/texture_compressor.h (right): https://codereview.chromium.org/1096703002/diff/200001/cc/resources/texture_c... cc/resources/texture_compressor.h:42: static base::CPU cpu; static initialiser here (of non-POD type) is a big no-no: http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html
https://codereview.chromium.org/1096703002/diff/220001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/220001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:212: __m128i threashhold_upper = _mm_set1_epi32(3); nit: spelling s/threashold/threshold/g
Just tried it, and it works great. It seems to produce slightly better results than the non-SSE version for some cases. I haven't looked into why, but my version probably takes a shortcut somewhere. Can you tell me anything about the performance, compared to the non-SIMD-optimized version? https://codereview.chromium.org/1096703002/diff/240001/cc/resources/texture_c... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/240001/cc/resources/texture_c... cc/resources/texture_compressor.cc:19: scoped_ptr<TextureCompressor> compressor(new TextureCompressorETC1()); Nit-pick, but we could avoid unnecessary allocation here by doing something like this: ... if (cpu.has_sse2()) return make_scoped_ptr(new TextureCompressorETC1SSE()); #endif return make_scoped_ptr(new TextureCompressorETC1()); https://codereview.chromium.org/1096703002/diff/240001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/240001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:591: inline bool TransposeBlock(uint8_t* block, __m128i* transposed /* [4] */) { This function appears a little bit hard to read for me. Maybe it's possible to improve the indentation and align the comments a little bit better. https://codereview.chromium.org/1096703002/diff/240001/cc/resources/texture_c... File cc/resources/texture_compressor_util.h (right): https://codereview.chromium.org/1096703002/diff/240001/cc/resources/texture_c... cc/resources/texture_compressor_util.h:9: namespace texture_compress { We don't use the texture_compress namespace any longer. I guess you got it from the original complete (but old) CL?
On 2015/04/29 12:25:46, christiank wrote: > Just tried it, and it works great. It seems to produce slightly better results > than the non-SSE version for some cases. I haven't looked into why, but my > version probably takes a shortcut somewhere. If the color error is equal I pick the bigger color value when choosing the best table modifier index. Also since the avg color can't be represented accurately I am currently experimenting with the best way of approximating the average color per sub-block. > Can you tell me anything about the performance, compared to the > non-SIMD-optimized version? On my Haswell machine running Ubuntu x64 I got speedups ranging from 1.6x for solid blocks to 3x for images such as Lenna or randomly generated textures. > https://codereview.chromium.org/1096703002/diff/240001/cc/resources/texture_c... > cc/resources/texture_compressor_util.h:9: namespace texture_compress { > We don't use the texture_compress namespace any longer. I guess you got it from > the original complete (but old) CL? Yes. That is correct. Do you have any other suggestions for grouping the common code? I assume in the end you will use those auxiliary functions for other encoders as well.
On 2015/04/29 14:06:39, radu.velea wrote: > On 2015/04/29 12:25:46, christiank wrote: > > Just tried it, and it works great. It seems to produce slightly better results > > than the non-SSE version for some cases. I haven't looked into why, but my > > version probably takes a shortcut somewhere. > > If the color error is equal I pick the bigger color value when choosing the best > table modifier index. Also since the avg color can't be represented accurately I > am currently experimenting with the best way of approximating the average color > per sub-block. Ah, I see. I'll investigate if I can apply the same optimizations to the non-optimized, and neon-optimized code. > > Can you tell me anything about the performance, compared to the > > non-SIMD-optimized version? > > On my Haswell machine running Ubuntu x64 I got speedups ranging from 1.6x for > solid blocks to 3x for images such as Lenna or randomly generated textures. Cool, sounds good! > https://codereview.chromium.org/1096703002/diff/240001/cc/resources/texture_c... > > cc/resources/texture_compressor_util.h:9: namespace texture_compress { > > We don't use the texture_compress namespace any longer. I guess you got it > from > > the original complete (but old) CL? > > Yes. That is correct. Do you have any other suggestions for grouping the common > code? I assume in the end you will use those auxiliary functions for other > encoders as well. Yes, there will be code that can be shared across implementations. I am not sure about the best way, but maybe having a separate header like you do will work. I think it's good enough for the moment at least. Just remove the texture_compress namespace and let it be only in cc.
I applied your feedback. @christiank I left you a comment some time ago regarding ETC1: https://codereview.chromium.org/1015373003/ I don't know if you had a chance to see it. Maybe it has something to do with the quality difference between the compressed textures.
radu.velea@intel.com changed reviewers: + reveman@chromium.org
Hi David, please could you take a look at this change?
robert.bradford@intel.com changed required reviewers: + reveman@chromium.org
Please include the difference in perf test results in the description of this patch https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... cc/resources/texture_compressor.cc:7: #include <stdio.h> what is this for? https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... cc/resources/texture_compressor.cc:19: #if defined(__i386__) || defined(__x86_x64__) can we remove this ifdef an just rely on base::CPU::has_sse2 always returning the right thing? https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... cc/resources/texture_compressor.cc:27: default: please avoid having a default case so the compiler will remind us of updating this code when adding new formats https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... File cc/resources/texture_compressor_unittest.cc (right): https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... cc/resources/texture_compressor_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. If this is supposed to contain generic texture compression tests (as the file name suggest) please have them be parameterized just like the perf tests. If they are ETC specific, then they belong in a texture_compressor_etc1_unittest.cc file. https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... cc/resources/texture_compressor_unittest.cc:20: #define TEST_PIXEL_LIMIT 256 please use constants instead of defines. and maybe move them into the test function that use them instead of having them be global https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... File cc/resources/texture_compressor_util.h (right): https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... cc/resources/texture_compressor_util.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Is all this needed for ETC1_SSE? Can you move what you need for SSE to texture_compressor_etc1.h instead of adding this new file and keep everything that's only used by non-sse implementation in texture_compressor_etc1.cc? https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... cc/resources/texture_compressor_util.h:213: #ifdef USE_PERCEIVED_ERROR_METRIC where's the comment that explains this ifdef?
On 2015/05/05 15:30:11, reveman wrote: > Please include the difference in perf test results in the description of this > patch Done! Also added some more relevant perf tests. > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > File cc/resources/texture_compressor.cc (right): > > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > cc/resources/texture_compressor.cc:7: #include <stdio.h> > what is this for? Removed unused headers. > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > cc/resources/texture_compressor.cc:19: #if defined(__i386__) || > defined(__x86_x64__) > can we remove this ifdef an just rely on base::CPU::has_sse2 always returning > the right thing? We won't be compiling sse module for non-intel platforms so I thought I'd be extra safe and have the if-guards. Maybe adding __SSE2__ ifndef-guard to *sse* files would be better? > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > cc/resources/texture_compressor.cc:27: default: > please avoid having a default case so the compiler will remind us of updating > this code when adding new formats Done! > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > File cc/resources/texture_compressor_unittest.cc (right): > > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > cc/resources/texture_compressor_unittest.cc:1: // Copyright 2015 The Chromium > Authors. All rights reserved. > If this is supposed to contain generic texture compression tests (as the file > name suggest) please have them be parameterized just like the perf tests. If > they are ETC specific, then they belong in a texture_compressor_etc1_unittest.cc > file. Done! The compression ratio test could also be applied to DXT1 compression format in the future. > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > cc/resources/texture_compressor_unittest.cc:20: #define TEST_PIXEL_LIMIT 256 > please use constants instead of defines. and maybe move them into the test > function that use them instead of having them be global Changed some to const int and moved one to test function. > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > File cc/resources/texture_compressor_util.h (right): > > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > cc/resources/texture_compressor_util.h:1: // Copyright 2015 The Chromium > Authors. All rights reserved. > Is all this needed for ETC1_SSE? Can you move what you need for SSE to > texture_compressor_etc1.h instead of adding this new file and keep everything > that's only used by non-sse implementation in texture_compressor_etc1.cc? > > https://codereview.chromium.org/1096703002/diff/280001/cc/resources/texture_c... > cc/resources/texture_compressor_util.h:213: #ifdef USE_PERCEIVED_ERROR_METRIC > where's the comment that explains this ifdef? Done!
https://codereview.chromium.org/1096703002/diff/300001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/1096703002/diff/300001/cc/cc.gyp#newcode635 cc/cc.gyp:635: 'target_name': 'cc_opts', hm, do we really need a separate target for this? I assume we're doing something similar somewhere else that this is based on. Can you point to that code? https://codereview.chromium.org/1096703002/diff/300001/cc/cc.gyp#newcode665 cc/cc.gyp:665: '-msse2', is this not needed in the gn build case too? https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor.cc:7: #include "base/cpu.h" nit: guard this with a ifdef as it's not always used https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor.cc:10: #include "cc/resources/texture_compressor_etc1_sse.h" nit: guard this with a ifdef as it's not always used https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor.cc:17: #if defined(__i386__) || defined(__x86_64__) Can you add a define to the build files and use it here instead? That would avoid the chance of this somehow getting out of sync. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.h (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:13: namespace { hm, I don't think we ever use anonymous namespace in headers. can you move this code into the cc namespace below instead? https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:42: */ This was wrong before your patch but do you mind changing these comments into chromium style comments as part of this change? // line 1 of comment // line 2 of comment https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:16: */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:26: /* raw data */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:28: /* 8 bit packed values */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:30: /* 32 bit zero extended values - 4x4 arrays */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:34: // __m128i *alpha; nit: comment style should this even be here? https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:37: /* commonly used registers */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:48: /* changed from _mm_mullo_epi32 to _mm_mullo_epi16 */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:130: /* [S0 S0 S1 S1] */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:134: /* [S2 S2 S3 S3] */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:151: /* [S0 S0 S1 S1] */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:155: /* [S2 S2 S3 S3] */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:172: /* [S0 S0 S1 S1] */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:176: /* [S2 S2 S3 S3] */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:192: /* TODO(radu.velea): return int's instead of floats */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:267: /* this will have the minimum errors for each 4 pixels */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:271: /* this will have the matching table index combo for each 4 pixels */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:284: /* fail early to increase speed */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:289: 0x1B, 0x4E, 0xB1, 0xE4}; /* important they are sorted ascending */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:341: /* save winning pattern */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:346: * performance penalty */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:351: /* Second part of the block */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:357: /* save winning pattern */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:362: * performance penalty */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:419: /* error is growing and is well beyond expected error */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:451: * horizontal 2 */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:461: * rounding and shifts */ nit: comment style and line wrapping https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:525: /* transpose vertical data into horizontal lines */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:596: */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:615: return false; /* block is solid, no need to do any more work */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:665: /* unpack red */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:675: /* unpack green */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:685: /* unpack blue */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:695: /* unpack alpha */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:786: DCHECK(width >= 4 && (width & 3) == 0); nit: two DCHECKs instead: DCHECK_GE(width, 4); DCHECK_EQ(width & 3, 0); https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:787: DCHECK(height >= 4 && (height & 3) == 0); nit: two DCHECKs instead https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_unittest.cc (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:18: TEST(TextureCompressorETC1Test, Compress256x256CreateETC1) { This test is not ETC1 specific. If you think this is a useful test then please move to a generic texture_compressor_unittest.cc file and run it with all types. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:24: TEST(TextureCompressorETC1Test, Compress256x256RatioETC1) { s/Compress256x256RatioETC1/Compress256x256Ratio/ as ETC1 is already in base test name https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:28: uint8_t dst[kImageSizeInBytes]; How about we add TextureCompressor API that will return the minimum size needed for a destination buffer and use that in a generic test instead of the kImagePoison mechanism below? https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:29: int compressed_size = 0; nit: move this variable down to where it's first used https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:32: /* Poison destination bytes */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:38: /* Generate test texture */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:44: (TextureCompressor::Quality)0); Don't use c-style casts. I think we should use a valid quality setting here. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:55: * alpha channel */ nit: comment style https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:56: EXPECT_EQ(compressed_size * 8, kImageSizeInBytes); expected value should be the first parameter https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor_perftest.cc (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:88: for (auto& quality : kQualities) While touching this file, do you mind removing all these quality loops in favor of using this as test parameters: ::testing::Combine(::testing::Values(TextureCompressor::kQualityLow, TextureCompressor::kQualityMedium, TextureCompressor::kQualityHigh), ::testing::Values(TextureCompressor::kFormatETC1))
I think I've added all or most of your feedback. Let me know if you see anything else that is not in order. @christiank, what do you think about the texture_compressor_etc1_unittest.cc? How should we best change the interface at this time to allow better testing? https://codereview.chromium.org/1096703002/diff/300001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/1096703002/diff/300001/cc/cc.gyp#newcode635 cc/cc.gyp:635: 'target_name': 'cc_opts', On 2015/05/06 18:44:00, reveman wrote: > hm, do we really need a separate target for this? I assume we're doing something > similar somewhere else that this is based on. Can you point to that code? I got the idea from https://codereview.chromium.org/793693003/ I expect we will have a similar NEON module in the near future. https://codereview.chromium.org/1096703002/diff/300001/cc/cc.gyp#newcode665 cc/cc.gyp:665: '-msse2', On 2015/05/06 18:44:00, reveman wrote: > is this not needed in the gn build case too? It seems to compile without it. I'm not sure who sets msse2 flag. I've done a "grep" and noticed: ./build/common.gypi: # -mfpmath=sse -msse2 makes the compiler use SSE instructions ./build/common.gypi: '-msse2', ./cc/cc.gyp: '-msse2', ./media/media.gyp: '-msse2', ./media/base/BUILD.gn: cflags = [ "-msse2" ] I've added it in Build.gn. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor.cc:7: #include "base/cpu.h" On 2015/05/06 18:44:00, reveman wrote: > nit: guard this with a ifdef as it's not always used Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor.cc:10: #include "cc/resources/texture_compressor_etc1_sse.h" On 2015/05/06 18:44:00, reveman wrote: > nit: guard this with a ifdef as it's not always used Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor.cc:17: #if defined(__i386__) || defined(__x86_64__) On 2015/05/06 18:44:00, reveman wrote: > Can you add a define to the build files and use it here instead? That would > avoid the chance of this somehow getting out of sync. -msse2 sets __SSE2__ would this be enough for the ifguards of both the headers and this section? I see it is used elsewhere inside Chrome. Otherwise I can add something like -DUSE_SSE_COMPRESSION in the build files and replace __SSE2__ with that. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.h (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:13: namespace { On 2015/05/06 18:44:00, reveman wrote: > hm, I don't think we ever use anonymous namespace in headers. can you move this > code into the cc namespace below instead? Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:42: */ On 2015/05/06 18:44:00, reveman wrote: > This was wrong before your patch but do you mind changing these comments into > chromium style comments as part of this change? > > // line 1 of comment > // line 2 of comment Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:16: */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:26: /* raw data */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:28: /* 8 bit packed values */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:30: /* 32 bit zero extended values - 4x4 arrays */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:34: // __m128i *alpha; On 2015/05/06 18:44:01, reveman wrote: > nit: comment style > > should this even be here? This would be used when adding ETC2: http://en.wikipedia.org/wiki/Ericsson_Texture_Compression#ETC2_and_EAC But for know I guess it's not needed and I removed it. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:37: /* commonly used registers */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:48: /* changed from _mm_mullo_epi32 to _mm_mullo_epi16 */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:130: /* [S0 S0 S1 S1] */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:134: /* [S2 S2 S3 S3] */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:151: /* [S0 S0 S1 S1] */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:155: /* [S2 S2 S3 S3] */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:172: /* [S0 S0 S1 S1] */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:176: /* [S2 S2 S3 S3] */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:192: /* TODO(radu.velea): return int's instead of floats */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:267: /* this will have the minimum errors for each 4 pixels */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:271: /* this will have the matching table index combo for each 4 pixels */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:284: /* fail early to increase speed */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:289: 0x1B, 0x4E, 0xB1, 0xE4}; /* important they are sorted ascending */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:341: /* save winning pattern */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:346: * performance penalty */ On 2015/05/06 18:44:00, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:351: /* Second part of the block */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:357: /* save winning pattern */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:362: * performance penalty */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:419: /* error is growing and is well beyond expected error */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:451: * horizontal 2 */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:461: * rounding and shifts */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style and line wrapping Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:525: /* transpose vertical data into horizontal lines */ On 2015/05/06 18:44:00, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:596: */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:615: return false; /* block is solid, no need to do any more work */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:665: /* unpack red */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:675: /* unpack green */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:685: /* unpack blue */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:695: /* unpack alpha */ On 2015/05/06 18:44:01, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:786: DCHECK(width >= 4 && (width & 3) == 0); On 2015/05/06 18:44:01, reveman wrote: > nit: two DCHECKs instead: > DCHECK_GE(width, 4); > DCHECK_EQ(width & 3, 0); Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:787: DCHECK(height >= 4 && (height & 3) == 0); On 2015/05/06 18:44:02, reveman wrote: > nit: two DCHECKs instead Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:787: DCHECK(height >= 4 && (height & 3) == 0); On 2015/05/06 18:44:02, reveman wrote: > nit: two DCHECKs instead Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_unittest.cc (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:18: TEST(TextureCompressorETC1Test, Compress256x256CreateETC1) { On 2015/05/06 18:44:02, reveman wrote: > This test is not ETC1 specific. If you think this is a useful test then please > move to a generic texture_compressor_unittest.cc file and run it with all types. This might be useful for checking if all encoders are supported on a given platform. I guess I can add something like this later on. Removing for now. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:28: uint8_t dst[kImageSizeInBytes]; On 2015/05/06 18:44:02, reveman wrote: > How about we add TextureCompressor API that will return the minimum size needed > for a destination buffer and use that in a generic test instead of the > kImagePoison mechanism below? That would be a good idea. I had a talk with christiank to return the "quality" of the compressed image - ex: mean square error between the original image and the texture resulting from decompressing the dst buffer; and use that in a unit test as well. The metric would have to be a bit more complex if perceptual color error functions are used, etc. @christiank do you have any thoughts on this? https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:29: int compressed_size = 0; On 2015/05/06 18:44:02, reveman wrote: > nit: move this variable down to where it's first used Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:32: /* Poison destination bytes */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:38: /* Generate test texture */ On 2015/05/06 18:44:02, reveman wrote: > nit: comment style Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:44: (TextureCompressor::Quality)0); On 2015/05/06 18:44:02, reveman wrote: > Don't use c-style casts. I think we should use a valid quality setting here. Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_unittest.cc:56: EXPECT_EQ(compressed_size * 8, kImageSizeInBytes); On 2015/05/06 18:44:02, reveman wrote: > expected value should be the first parameter Done. https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... File cc/resources/texture_compressor_perftest.cc (right): https://codereview.chromium.org/1096703002/diff/300001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:88: for (auto& quality : kQualities) On 2015/05/06 18:44:03, reveman wrote: > While touching this file, do you mind removing all these quality loops in favor > of using this as test parameters: > > ::testing::Combine(::testing::Values(TextureCompressor::kQualityLow, > TextureCompressor::kQualityMedium, > TextureCompressor::kQualityHigh), > ::testing::Values(TextureCompressor::kFormatETC1)) Done.
lgtm with nits and % christiank's review https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor.cc:8: #include "base/cpu.h" nit: you can move this into the ifdef below https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.cc (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.cc:294: namespace cc { nit: move this above "namespace {" on line 21 and you can get rid of all the cc:: prefix changes above https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.h (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:187: } // namespace cc nit: remove this and "namespace cc {" two lines below.. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:20: #define ETC1_SET_ERROR(x) (x + x / 2 + 384) nit: inline function instead of macro https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:33: // commonly used registers nit: I don't feel too strongly about this but there's some inconsistency between comments in this patch. It's recommended to write comments as real sentences in chromium. First word capitalized and a period at the end. "// Commonly used registers." in this case. If you like to update all the comments in this patch to be more consistent in this regard then that's great but it's also fine with me to leave it as is. Up to you. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:472: (int)__sse_avg_colors[0], (int)__sse_avg_colors[0]); nit: please avoid c-style casts. static_cast<int>() instead https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:476: (int)__sse_avg_colors[1], (int)__sse_avg_colors[1]); nit: please avoid c-style casts. static_cast<int>() instead https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:480: (int)__sse_avg_colors[2], (int)__sse_avg_colors[2]); nit: please avoid c-style casts. static_cast<int>() instead https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:487: blue_avg[1] = _mm_set1_epi32((int)__sse_avg_colors[9]); nit: please avoid c-style casts. static_cast<int>() instead https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:490: green_avg[1] = _mm_set1_epi32((int)__sse_avg_colors[10]); nit: please avoid c-style casts. static_cast<int>() instead https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:493: red_avg[1] = _mm_set1_epi32((int)__sse_avg_colors[11]); nit: please avoid c-style casts. static_cast<int>() instead https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:500: uint32_t* expected_errors = flip == true ? horizontal_error : vertical_error; nit: s/flip == true/flip/ https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:519: if (flip == false) { nit: if (!flip) https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:773: namespace cc { nit: please move this up to line 17 just before "namespace {" and remove the cc:: prefix in the code above https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor_perftest.cc (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:63: void RunTest(const std::string& name, TextureCompressor::Quality quality) { nit: please remove the quality parameter/member and instead add TextureCompressor::Quality quality = ::testing::get<1>(GetParam()) as the first line of this function. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:71: FormatName(::testing::get<1>(GetParam())) + " " + QualityName(quality); nit: please add a TextureCompressor::Format format = ::testing::get<1>(GetParam()) just above this line to make it a bit easier to understand what this value we get from get<1>(GetParam()) is https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:107: #if defined(OS_WIN) do we need this ifdef? we use srand and std::rand unconditionally in some other tests. see content/common/discardable_shared_memory_heap_perftest.cc for example. I'd prefer if you removed this ifdef.
I think I fixed everything. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor.cc:8: #include "base/cpu.h" On 2015/05/07 14:24:35, reveman wrote: > nit: you can move this into the ifdef below Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.cc (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.cc:294: namespace cc { On 2015/05/07 14:24:35, reveman wrote: > nit: move this above "namespace {" on line 21 and you can get rid of all the > cc:: prefix changes above Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.h (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:187: } // namespace cc On 2015/05/07 14:24:35, reveman wrote: > nit: remove this and "namespace cc {" two lines below.. Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:20: #define ETC1_SET_ERROR(x) (x + x / 2 + 384) On 2015/05/07 14:24:35, reveman wrote: > nit: inline function instead of macro Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:33: // commonly used registers On 2015/05/07 14:24:35, reveman wrote: > nit: I don't feel too strongly about this but there's some inconsistency between > comments in this patch. It's recommended to write comments as real sentences in > chromium. First word capitalized and a period at the end. "// Commonly used > registers." in this case. If you like to update all the comments in this patch > to be more consistent in this regard then that's great but it's also fine with > me to leave it as is. Up to you. Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:472: (int)__sse_avg_colors[0], (int)__sse_avg_colors[0]); On 2015/05/07 14:24:35, reveman wrote: > nit: please avoid c-style casts. static_cast<int>() instead Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:476: (int)__sse_avg_colors[1], (int)__sse_avg_colors[1]); On 2015/05/07 14:24:35, reveman wrote: > nit: please avoid c-style casts. static_cast<int>() instead Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:480: (int)__sse_avg_colors[2], (int)__sse_avg_colors[2]); On 2015/05/07 14:24:35, reveman wrote: > nit: please avoid c-style casts. static_cast<int>() instead Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:487: blue_avg[1] = _mm_set1_epi32((int)__sse_avg_colors[9]); On 2015/05/07 14:24:35, reveman wrote: > nit: please avoid c-style casts. static_cast<int>() instead Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:490: green_avg[1] = _mm_set1_epi32((int)__sse_avg_colors[10]); On 2015/05/07 14:24:35, reveman wrote: > nit: please avoid c-style casts. static_cast<int>() instead Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:493: red_avg[1] = _mm_set1_epi32((int)__sse_avg_colors[11]); On 2015/05/07 14:24:35, reveman wrote: > nit: please avoid c-style casts. static_cast<int>() instead Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:500: uint32_t* expected_errors = flip == true ? horizontal_error : vertical_error; On 2015/05/07 14:24:35, reveman wrote: > nit: s/flip == true/flip/ Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:519: if (flip == false) { On 2015/05/07 14:24:35, reveman wrote: > nit: if (!flip) Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:773: namespace cc { On 2015/05/07 14:24:35, reveman wrote: > nit: please move this up to line 17 just before "namespace {" and remove the > cc:: prefix in the code above Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... File cc/resources/texture_compressor_perftest.cc (right): https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:63: void RunTest(const std::string& name, TextureCompressor::Quality quality) { On 2015/05/07 14:24:35, reveman wrote: > nit: please remove the quality parameter/member and instead add > > TextureCompressor::Quality quality = ::testing::get<1>(GetParam()) > > as the first line of this function. Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:71: FormatName(::testing::get<1>(GetParam())) + " " + QualityName(quality); On 2015/05/07 14:24:35, reveman wrote: > nit: please add a TextureCompressor::Format format = > ::testing::get<1>(GetParam()) just above this line to make it a bit easier to > understand what this value we get from get<1>(GetParam()) is Done. https://codereview.chromium.org/1096703002/diff/360001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:107: #if defined(OS_WIN) On 2015/05/07 14:24:35, reveman wrote: > do we need this ifdef? we use srand and std::rand unconditionally in some other > tests. see content/common/discardable_shared_memory_heap_perftest.cc for > example. I'd prefer if you removed this ifdef. Done.
lgtm still. just a few build related nits that I missed in my last review https://codereview.chromium.org/1096703002/diff/380001/cc/BUILD.gn File cc/BUILD.gn (right): https://codereview.chromium.org/1096703002/diff/380001/cc/BUILD.gn#newcode554 cc/BUILD.gn:554: if (target_cpu == "ia32" || target_cpu == "x64") { Should we add "source_set("cc_opts_sse")" here and add a source_set("cc_opts") { deps = [ "//cc:cc_opts_sse", ] } to be consistent with the gyp build? https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... cc/resources/texture_compressor.cc:10: #ifdef __SSE2__ #if defined(ARCH_CPU_X86_FAMILY), see comment below https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... cc/resources/texture_compressor.cc:20: #ifdef __SSE2__ I found another example in the chromium code base where we use sse2. media/base/yuv_convert.cc "#if defined(ARCH_CPU_X86_FAMILY)" is used there so I assume that's sufficient to build this code. I recommend switching to that for consistency. https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:7: #include <assert.h> is this needed?
Sync-ed Build.gn, replaced ifdefs, removed unused headers. https://codereview.chromium.org/1096703002/diff/380001/cc/BUILD.gn File cc/BUILD.gn (right): https://codereview.chromium.org/1096703002/diff/380001/cc/BUILD.gn#newcode554 cc/BUILD.gn:554: if (target_cpu == "ia32" || target_cpu == "x64") { On 2015/05/07 16:26:29, reveman wrote: > Should we add "source_set("cc_opts_sse")" here and add a > > source_set("cc_opts") { > deps = [ > "//cc:cc_opts_sse", > ] > } > > to be consistent with the gyp build? Done. https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... File cc/resources/texture_compressor.cc (right): https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... cc/resources/texture_compressor.cc:10: #ifdef __SSE2__ On 2015/05/07 16:26:29, reveman wrote: > #if defined(ARCH_CPU_X86_FAMILY), see comment below Done. https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... cc/resources/texture_compressor.cc:20: #ifdef __SSE2__ On 2015/05/07 16:26:29, reveman wrote: > I found another example in the chromium code base where we use sse2. > media/base/yuv_convert.cc > > "#if defined(ARCH_CPU_X86_FAMILY)" is used there so I assume that's sufficient > to build this code. I recommend switching to that for consistency. Done. https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/380001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:7: #include <assert.h> On 2015/05/07 16:26:29, reveman wrote: > is this needed? Done.
lgtm with nits. Noticed a small error in documentation, otherwise it looks good. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.h (right): https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:61: // [ 0][ 1][ 2][ 3] [ 0][ 4][ 4][ 5] nit: Should be: [ 0][ 1][ 4][ 5]
Some minor nits for you to fix when integrating christiank's feedback. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.h (right): https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:85: // for BGRA textures nit: comment style. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:119: // For BGRA textures nit: comment style. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:20: // ETC1 codeword table is sorted ascending. nit: "ETC1 codeword table is sorted in ascending order" https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:25: // We use this threashold to determine when it doesn't make sense to iterate nit: threshold
Fixed comments. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1.h (right): https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:61: // [ 0][ 1][ 2][ 3] [ 0][ 4][ 4][ 5] On 2015/05/11 13:56:16, christiank wrote: > nit: Should be: [ 0][ 1][ 4][ 5] Done. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:85: // for BGRA textures On 2015/05/11 14:18:02, robert.bradford wrote: > nit: comment style. Done. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1.h:119: // For BGRA textures On 2015/05/11 14:18:02, robert.bradford wrote: > nit: comment style. Done. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... File cc/resources/texture_compressor_etc1_sse.cc (right): https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:20: // ETC1 codeword table is sorted ascending. On 2015/05/11 14:18:02, robert.bradford wrote: > nit: "ETC1 codeword table is sorted in ascending order" Done. https://codereview.chromium.org/1096703002/diff/400001/cc/resources/texture_c... cc/resources/texture_compressor_etc1_sse.cc:25: // We use this threashold to determine when it doesn't make sense to iterate On 2015/05/11 14:18:02, robert.bradford wrote: > nit: threshold Done.
The CQ bit was checked by radu.velea@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, christiank@opera.com Link to the patchset: https://codereview.chromium.org/1096703002/#ps420001 (title: "Fixing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096703002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by radu.velea@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from christiank@opera.com, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1096703002/#ps440001 (title: "Fix BUILD.gn check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096703002/440001
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/5f3849aa8307399b7e6dfe5665ed149594244077 Cr-Commit-Position: refs/heads/master@{#329840}
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:440001) has been created in https://codereview.chromium.org/1136083003/ by sergeyv@chromium.org. The reason for reverting is: Speculative revert. Looks like this change breaks compilation on win8 GN: http://build.chromium.org/p/chromium.win/builders/Win8%20GN Failure Example: http://build.chromium.org/p/chromium.win/builders/Win8%20GN/builds/7283.
The CQ bit was checked by radu.velea@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from christiank@opera.com, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1096703002/#ps460001 (title: "Reland: Change ia32 to x86 in Build.gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096703002/460001
The CQ bit was unchecked by robert.bradford@intel.com
The CQ bit was checked by robert.bradford@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096703002/460001
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/648fb54ee219104d27e2a2de0806408ac0e7c75b Cr-Commit-Position: refs/heads/master@{#329859}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1096703002/diff/460001/cc/resources/texture_c... File cc/resources/texture_compressor_perftest.cc (right): https://codereview.chromium.org/1096703002/diff/460001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:105: srand(kImageSeed); Why not just use the appropriate functions from base/rand_util.h?
Message was sent while issue was closed.
https://codereview.chromium.org/1096703002/diff/460001/cc/resources/texture_c... File cc/resources/texture_compressor_perftest.cc (right): https://codereview.chromium.org/1096703002/diff/460001/cc/resources/texture_c... cc/resources/texture_compressor_perftest.cc:105: srand(kImageSeed); On 2015/05/25 22:31:58, Lei Zhang wrote: > Why not just use the appropriate functions from base/rand_util.h? I think we want pseudo random numbers here as real random numbers could cause unexpected differences between each run of this perf test. base/rand_util.h doesn't seem to provide this but maybe I'm missing something?
Message was sent while issue was closed.
On 2015/05/26 15:50:22, reveman wrote: > https://codereview.chromium.org/1096703002/diff/460001/cc/resources/texture_c... > File cc/resources/texture_compressor_perftest.cc (right): > > https://codereview.chromium.org/1096703002/diff/460001/cc/resources/texture_c... > cc/resources/texture_compressor_perftest.cc:105: srand(kImageSeed); > On 2015/05/25 22:31:58, Lei Zhang wrote: > > Why not just use the appropriate functions from base/rand_util.h? > > I think we want pseudo random numbers here as real random numbers could cause > unexpected differences between each run of this perf test. base/rand_util.h > doesn't seem to provide this but maybe I'm missing something? Got it. Thanks. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
