Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
373243002:
Optimized R11 EAC compressor (Closed)
|
Created:
6 years, 5 months ago by krajcevski Modified:
6 years, 5 months ago Reviewers:
robertphillips CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionOptimized R11 EAC compressor
Committed: https://skia.googlesource.com/skia/+/1459be5ae3b1c959451427ab7d148322662ae6f7
Patch Set 1 #
Total comments: 22
Patch Set 2 : Code reviews and cleanup #Patch Set 3 : Comment touch-ups #Patch Set 4 : Change -1's to 0xFFF's #Messages
Total messages: 12 (0 generated)
Some of the compression routines here might be a little convoluted. I tried to address them with comments but let me know if I need to expand on any of them.
lgtm + nits (mainly comment requests) https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... File src/utils/SkTextureCompressor.cpp (right): https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:442: #if COMPRESS_R11_EAC_FAST // This works by ... ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:451: switch(idx) { each of these guys on their own line ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:530: // May want to pick different input names. 'a' and 'b' conflict with the index names you just gave above. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:618: // Most of the voodoo in this function comes from Hacker's Delight, section 2-18 inline ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:645: static uint64_t compress_r11eac_block_fast(const uint8_t* src, int rowBytes) { ri* -> inputRow* ? ri* -> alphaRow* ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:652: if (0 == ri1) { // Fully transparent block ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:654: } else if (0xFFFFFFFF == ri1) { // Fully opaque block ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:658: r* -> indexRow* ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:662: const uint32_t r4 = convert_indices(ri4); // comment ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:665: const uint64_t indices = interleave6(r1r2, r3r4); // comment ? https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:675: const ?
Do you have any timing results?
Timing: Uncompressed: loops min mean max stddev config bench Device supports ARM NEON instructions! 1 76.3ms 76.6ms 77.3ms 0% 8888 polygon 1 143ms 145ms 161ms 2% gpu polygon 34 169µs 169µs 171µs 0% 8888 compressed_alpha 16 317µs 322µs 334µs 2% gpu compressed_alpha COMPRESS_R11_EAC_FASTEST: loops min mean max stddev config bench Device supports ARM NEON instructions! 1 76.4ms 76.7ms 77.5ms 0% 8888 polygon 1 153ms 155ms 157ms 1% gpu polygon 34 169µs 169µs 173µs 0% 8888 compressed_alpha 15 346µs 349µs 354µs 0% gpu compressed_alpha COMPRESS_R11_EAC_FAST: loops min mean max stddev config bench Device supports ARM NEON instructions! 1 75.8ms 76ms 78.2ms 0% 8888 polygon 1 194ms 195ms 198ms 0% gpu polygon 35 167µs 168µs 169µs 0% 8888 compressed_alpha 12 425µs 427µs 432µs 0% gpu compressed_alpha COMPRESS_R11_EAC_SLOW: loops min mean max stddev config bench Device supports ARM NEON instructions! 1 75.9ms 76.1ms 76.9ms 0% 8888 polygon 1 1.42s 1.42s 1.42s 0% gpu polygon 33 167µs 168µs 170µs 0% 8888 compressed_alpha 2 3.78ms 3.81ms 3.85ms 0% gpu compressed_alpha All gpu timings render the alpha bitmap and then compress it. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... File src/utils/SkTextureCompressor.cpp (right): https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:442: #if COMPRESS_R11_EAC_FAST On 2014/07/09 12:24:25, robertphillips wrote: > // This works by ... ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:451: switch(idx) { On 2014/07/09 12:24:26, robertphillips wrote: > each of these guys on their own line ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:530: // On 2014/07/09 12:24:25, robertphillips wrote: > May want to pick different input names. 'a' and 'b' conflict with the index > names you just gave above. Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:618: // Most of the voodoo in this function comes from Hacker's Delight, section 2-18 On 2014/07/09 12:24:26, robertphillips wrote: > inline ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:645: static uint64_t compress_r11eac_block_fast(const uint8_t* src, int rowBytes) { On 2014/07/09 12:24:26, robertphillips wrote: > ri* -> inputRow* ? > ri* -> alphaRow* ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:652: if (0 == ri1) { On 2014/07/09 12:24:26, robertphillips wrote: > // Fully transparent block ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:654: } else if (0xFFFFFFFF == ri1) { On 2014/07/09 12:24:25, robertphillips wrote: > // Fully opaque block ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:658: On 2014/07/09 12:24:26, robertphillips wrote: > r* -> indexRow* ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:662: const uint32_t r4 = convert_indices(ri4); On 2014/07/09 12:24:26, robertphillips wrote: > // comment ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:665: const uint64_t indices = interleave6(r1r2, r3r4); On 2014/07/09 12:24:25, robertphillips wrote: > // comment ? Done. https://codereview.chromium.org/373243002/diff/1/src/utils/SkTextureCompresso... src/utils/SkTextureCompressor.cpp:675: On 2014/07/09 12:24:25, robertphillips wrote: > const ? 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/373243002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)
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/373243002/60001
Message was sent while issue was closed.
Change committed as 1459be5ae3b1c959451427ab7d148322662ae6f7 |
