Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
325733004:
Add texture compression utility (Closed)
|
Created:
6 years, 6 months ago by krajcevski Modified:
6 years, 6 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionAdd texture compression utility
Committed: https://skia.googlesource.com/skia/+/ae614409e4cff083c070face78cc349153d0b932
Patch Set 1 #Patch Set 2 : Actually add the files. #Patch Set 3 : Some more small cleanup #
Total comments: 20
Patch Set 4 : Code review changes #
Total comments: 12
Patch Set 5 : Code review changes #Patch Set 6 : Compute palette indices with palette #
Total comments: 6
Patch Set 7 : Code review changes #Patch Set 8 : Fix compiler warnings in debug #Patch Set 9 : Move abs_diff back #Patch Set 10 : Remove static cast #
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor.cpp (right): https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:19: Maybe SkTAbsDiff in SkTypes.h ? https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:30: static const int kLATCPaletteSize = 8; ? // Return the squared minimum error cost of approximating 'pixel' using the provided palette https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:39: // Maybe an overview of the compression algorithm ? // This attempts a brute force compression of an LATC block by determining which of two possible palettes represent it with the least error. ? https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:52: // palette 0 is a straight up linear ramp between minVal and maxVal ? https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:58: // palette 1 is a shorter linear ramp between minVal and maxVal but includes 0 and 255 ? Is this palette ever selected ? https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:65: palettes[1][7] = 255; The raster code only ever generates 16 different alpha values (it only does 4x4 super sampling). It seems this knowledge could be used to select a better palette. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:66: error0 & error1 to match palette numbering ? https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:77: uint64_t result = 0; Is this jamming the right values in? https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:86: uint8_t minErrorIndex = 0; So we essentially do this loop to select the palette. Can we store both then and then just pick one? https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor.h (right): https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:24: // Returns an SKData holding a blob of compressed data that corresponds pixelConfig -> config ?
Adding image handling experts
https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor.cpp (right): https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:19: On 2014/06/09 19:50:18, robertphillips wrote: > Maybe SkTAbsDiff in SkTypes.h ? Done. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:30: On 2014/06/09 19:50:18, robertphillips wrote: > static const int kLATCPaletteSize = 8; ? > > // Return the squared minimum error cost of approximating 'pixel' using the > provided palette Done. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:39: On 2014/06/09 19:50:18, robertphillips wrote: > // Maybe an overview of the compression algorithm ? > > // This attempts a brute force compression of an LATC block by determining which > of two possible palettes represent it with the least error. ? Done. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:52: On 2014/06/09 19:50:18, robertphillips wrote: > // palette 0 is a straight up linear ramp between minVal and maxVal ? Done. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:58: On 2014/06/09 19:50:18, robertphillips wrote: > // palette 1 is a shorter linear ramp between minVal and maxVal but includes 0 > and 255 ? > > Is this palette ever selected ? The way this algorithm is set up, probably not. In general this palette is probably better for alpha masks, since you can ignore min/max pixels and try to optimize for the 1-2 non saturated colors that the AA computes. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:65: palettes[1][7] = 255; On 2014/06/09 19:50:17, robertphillips wrote: > The raster code only ever generates 16 different alpha values (it only does 4x4 > super sampling). It seems this knowledge could be used to select a better > palette. Yes, probably. I think that investigating better compression schemes is probably good to do once all of the other plumbing is in place (texture cache, etc). https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:66: On 2014/06/09 19:50:18, robertphillips wrote: > error0 & error1 to match palette numbering ? Done. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:77: uint64_t result = 0; On 2014/06/09 19:50:18, robertphillips wrote: > Is this jamming the right values in? Yes, although I did have to double check. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:86: uint8_t minErrorIndex = 0; On 2014/06/09 19:50:18, robertphillips wrote: > So we essentially do this loop to select the palette. Can we store both then and > then just pick one? The palette is selected in the preceding loop. This loop is to determine what index into the palette to store for each pixel. I've added this as a comment. https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor.h (right): https://codereview.chromium.org/325733004/diff/40001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:24: // Returns an SKData holding a blob of compressed data that corresponds On 2014/06/09 19:50:18, robertphillips wrote: > pixelConfig -> config ? Done.
On 2014/06/09 19:50:54, robertphillips wrote: > Adding image handling experts No image specific comments, but I do have some others: https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor.cpp (right): https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:136: (bm.config() != SkBitmap::kA8_Config)) { Use colorType() instead of config(). We are removing config(). https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:174: SkData *CompressBitmapToConfig(const SkBitmap &bitmap, Format config) { Shouldn't this be ToFormat, as in the header file? https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor.h (right): https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:24: // Returns an SKData holding a blob of compressed data that corresponds nit: SkData. https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:25: // to the bitmap. If the bitmap config cannot be compressed using the SkBitmap::Config is deprecated. Use SkColorType. https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:27: // a refcount of one. Typically I think we state that the caller is responsible for calling unref(). https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:28: SkData *CompressBitmapToFormat(const SkBitmap &bitmap, Format format); nit: SkData* CompressBitmapToFormat const SkBitmap& bitmap
https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor.cpp (right): https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:136: (bm.config() != SkBitmap::kA8_Config)) { On 2014/06/09 20:54:18, scroggo wrote: > Use colorType() instead of config(). We are removing config(). Done. https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.cpp:174: SkData *CompressBitmapToConfig(const SkBitmap &bitmap, Format config) { On 2014/06/09 20:54:18, scroggo wrote: > Shouldn't this be ToFormat, as in the header file? Yes, whoops. Done. https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... File src/utils/SkTextureCompressor.h (right): https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:24: // Returns an SKData holding a blob of compressed data that corresponds On 2014/06/09 20:54:18, scroggo wrote: > nit: SkData. Done. https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:25: // to the bitmap. If the bitmap config cannot be compressed using the On 2014/06/09 20:54:18, scroggo wrote: > SkBitmap::Config is deprecated. Use SkColorType. Done. https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:27: // a refcount of one. On 2014/06/09 20:54:18, scroggo wrote: > Typically I think we state that the caller is responsible for calling unref(). Done. https://codereview.chromium.org/325733004/diff/60001/src/utils/SkTextureCompr... src/utils/SkTextureCompressor.h:28: SkData *CompressBitmapToFormat(const SkBitmap &bitmap, Format format); On 2014/06/09 20:54:18, scroggo wrote: > nit: SkData* CompressBitmapToFormat > const SkBitmap& bitmap Done.
1gtm
I updated the compression code to calculate the indices at the same time as the optimal palette.
lgtm + some nits We probably need a unit test for this https://codereview.chromium.org/325733004/diff/100001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/325733004/diff/100001/include/core/SkTypes.h#... include/core/SkTypes.h:359: template <typename T> inline T SkTAbsDiff(const T &a, const T &b) { space before '?' ? https://codereview.chromium.org/325733004/diff/100001/src/utils/SkTextureComp... File src/utils/SkTextureCompressor.cpp (right): https://codereview.chromium.org/325733004/diff/100001/src/utils/SkTextureComp... src/utils/SkTextureCompressor.cpp:64: static uint64_t compress_latc_block(uint8_t block[16]) { from which -> which ? https://codereview.chromium.org/325733004/diff/100001/src/utils/SkTextureComp... src/utils/SkTextureCompressor.cpp:119: // Assemble the compressed block ?
https://codereview.chromium.org/325733004/diff/100001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/325733004/diff/100001/include/core/SkTypes.h#... include/core/SkTypes.h:359: template <typename T> inline T SkTAbsDiff(const T &a, const T &b) { On 2014/06/09 22:12:40, robertphillips wrote: > space before '?' ? Done. https://codereview.chromium.org/325733004/diff/100001/src/utils/SkTextureComp... File src/utils/SkTextureCompressor.cpp (right): https://codereview.chromium.org/325733004/diff/100001/src/utils/SkTextureComp... src/utils/SkTextureCompressor.cpp:64: static uint64_t compress_latc_block(uint8_t block[16]) { On 2014/06/09 22:12:40, robertphillips wrote: > from which -> which ? Done. https://codereview.chromium.org/325733004/diff/100001/src/utils/SkTextureComp... src/utils/SkTextureCompressor.cpp:119: On 2014/06/09 22:12:40, robertphillips wrote: > // Assemble the compressed block ? Done.
+ reed
SkTAbsDiff might be slower than SkTAbs(a - b) for things like floats.
Since your specific use is for unsigned, I would suggest just writing one for
unsigned, and (for now) making it local to your call-site.
e.g.
static inline unsigned uabs(unsigned a, unsigned b) { ... }
Are we okay having a bunch of these lying around? There is already one in DMUtil.cpp (abs_diff) and BlendTest.cpp (abs_diff). I was hoping to centralize things a bit.
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/325733004/160001
On 2014/06/10 17:59:22, robertphillips wrote: > Are we okay having a bunch of these lying around? There is already one in > DMUtil.cpp (abs_diff) and BlendTest.cpp (abs_diff). I was hoping to centralize > things a bit. 1. I'm fine(r) with sharing them, if it is not so generally templated (i.e. floats) 2. I know we're bad about this today, but I'd like to see a trend of identifying internal -vs- external (public) utilities
On 2014/06/10 18:11:49, reed1 wrote:
> On 2014/06/10 17:59:22, robertphillips wrote:
> > Are we okay having a bunch of these lying around? There is already one in
> > DMUtil.cpp (abs_diff) and BlendTest.cpp (abs_diff). I was hoping to
centralize
> > things a bit.
>
> 1. I'm fine(r) with sharing them, if it is not so generally templated (i.e.
> floats)
> 2. I know we're bad about this today, but I'd like to see a trend of
identifying
> internal -vs- external (public) utilities
Although I haven't seen this programming pattern in Skia yet, there is a
solution to (1) using template specialization:
template<typename T> inline T SkTAbsDiff(const T& a, const T& b) {
return (a > b)? (a - b) : (b - a);
}
template<> inline float SkTAbsDiff<float>(const float& a, const float& b) {
return SkTAbs(a - b);
}
template<> inline double SkTAbsDiff<double>(const double& a, const double& b) {
return SkTAbs(a - b);
}
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: Build-Win7-VS2010-x86-Debug-Trybot on tryserver.chromium (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/325733004/180001
Message was sent while issue was closed.
Change committed as ae614409e4cff083c070face78cc349153d0b932 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
