Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
580863004:
Adding 3D lut color filter (Closed)
DescriptionAdding 3D lut color filter
Included in this cl is support for 3D textures.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/ce686270f533b9d741ef85661ef9d78517a01b86
Patch Set 1 #Patch Set 2 : New version #
Total comments: 4
Patch Set 3 : Work in progress #Patch Set 4 : Work in progress (new files added) #Patch Set 5 : First working version #Patch Set 6 : As color filter instead of image filter #
Total comments: 3
Patch Set 7 : Replaced SkBitmap with SkData #
Total comments: 3
Patch Set 8 : Added generationID for 3D texture key #
Total comments: 5
Patch Set 9 : Color profile effect using only 2D textures #
Total comments: 18
Patch Set 10 : Added cache and fixed a few comments #Patch Set 11 : Fixed long lines and added a lut to remove int->float conversions #
Total comments: 15
Patch Set 12 : Added unplremultiply/premultiply in the shader #Patch Set 13 : Fixed comments and added bench #Patch Set 14 : Fixed a header #
Total comments: 1
Patch Set 15 : Removed change to SkColorMatrixFilter #Patch Set 16 : Changed GrTextureImpl for GrTexturePriv #Patch Set 17 : Update to ToT #
Total comments: 15
Patch Set 18 : Fixed comments #
Total comments: 6
Patch Set 19 : Fixed some comments #
Total comments: 30
Patch Set 20 : Fixed some comments #
Total comments: 6
Patch Set 21 : Fixed some minor stuff #Patch Set 22 : Registered filter for serialization #Patch Set 23 : Update to ToT #
Messages
Total messages: 76 (11 generated)
sugoi@chromium.org changed reviewers: + bsalomon@google.com, reed@google.com
Hi Brian and Mike, I haven't written much yet, this cl isn't close to ready, but I just wanted to get your opinion on this before I actually write anything. Any strong opinion against having 3D textures in skia? This could potentially be used to apply color profiles on desktop (maybe mobile in a few years), but I could emulate this with 2 fetches of a linearly interpolated 2D texture where the tiles are side by side (just have to make sure we don't interpolate between tiles). For example, a 32x32x32 3D texture could be represented by a 32x1024 texture.
If 3D textures are useful I've got no qualms about adding support. One particular thing: I'd rather not express it as a pixel config but rather as a property of GrSurfaceDesc.
On 2014/09/18 14:45:59, bsalomon wrote: > If 3D textures are useful I've got no qualms about adding support. One > particular thing: I'd rather not express it as a pixel config but rather as a > property of GrSurfaceDesc. Is this closer to what you had in mind?
https://codereview.chromium.org/580863004/diff/20001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/580863004/diff/20001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:602: Need tests for support on OpenGL, not just GLES. https://codereview.chromium.org/580863004/diff/20001/src/gpu/gl/GrGpuGL.cpp File src/gpu/gl/GrGpuGL.cpp (right): https://codereview.chromium.org/580863004/diff/20001/src/gpu/gl/GrGpuGL.cpp#n... src/gpu/gl/GrGpuGL.cpp:671: desc.fWidth, // FIXME: width is used as depth Why not just add a depth to fDesc and get rid of the flag?
I'm ready for a 1st review Right now, 3D luts of size N*N*N are packed into an SkBitmap of size N * N^2 for convenience and because the texture loading functions expect an SkBitmap as input. If that approach is disliked, I'm open to ideas here. I'm still missing the shader that applies the 3D lut as a 2D texture, in case 3D textures aren't supported, I'll add it soon. https://codereview.chromium.org/580863004/diff/20001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/580863004/diff/20001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:602: On 2014/09/19 13:47:04, bsalomon wrote: > Need tests for support on OpenGL, not just GLES. Done. https://codereview.chromium.org/580863004/diff/20001/src/gpu/gl/GrGpuGL.cpp File src/gpu/gl/GrGpuGL.cpp (right): https://codereview.chromium.org/580863004/diff/20001/src/gpu/gl/GrGpuGL.cpp#n... src/gpu/gl/GrGpuGL.cpp:671: desc.fWidth, // FIXME: width is used as depth On 2014/09/19 13:47:04, bsalomon wrote: > Why not just add a depth to fDesc and get rid of the flag? Done.
https://codereview.chromium.org/580863004/diff/100001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/100001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:84: const_cast<SkColorProfileFilter*>(this)->initProcessingLuts(); Hi Mike and Brian, I have an issue here. initProcessingLuts(), if called from multiple threads, could run into race conditions. I could add a mutex in there, but really, I'd like to be able to call this BEFORE calling any of the filterSpan calls. initProcessingLuts() depends only on the 3D lut's size and nothing else, so I have this information at construction time, but I don't want to build these luts in the constructor, because I don't need them if I'm using the GPU. FYI, these luts are only used to do 3D LERP, to avoid having to recompute the LERP factors inside the processing loop. Is there any point/function where I can hook this initialization call, only for the CPU case?
https://codereview.chromium.org/580863004/diff/100001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/100001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:37: SkBitmap fColorCube; I don't think SkBitmap is the right container for a 3D lookup table. Is there a reason this can't just be a 3D array or a new type?
https://codereview.chromium.org/580863004/diff/100001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/100001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:37: SkBitmap fColorCube; On 2014/09/24 20:29:44, bsalomon wrote: > I don't think SkBitmap is the right container for a 3D lookup table. Is there a reason this can't just be a 3D array or a new type? I can do that, I would then need to add the appropriate texture loading API for that new type. I only put it in SkBitmap for convenience, and because the version of the shader using texture2D (to be done) instead of texture3D would use the bitmap as is (but I can easily do it the other way around and pack an array into a bitmap if that happens). I'm all for a new type, I just wanted to make sure that all the new API functions it will create aroud SkGr... is desirable. Or do you just mean that I should store it in a 3D array here, but still put it into an SkBitmap for texture loading?
Is there a need/desire to have the 3d array be refcounted? If so, seems like Create(SkData*, int width, int height, int depth) could work for the effect constructor.
On 2014/09/24 20:47:50, reed1 wrote:
> Is there a need/desire to have the 3d array be refcounted? If so, seems like
>
> Create(SkData*, int width, int height, int depth)
>
> could work for the effect constructor.
Sure, that would be ok with me, but I'd need to add something like:
GrLockAndRefCachedDataTexture(GrContext* ctx, const SkData& data, int width, int
height, int depth, const GrTextureParams* params) {...}
... and all associated functions. Does that make sense? I can do that is it's ok
with you guys.
On 2014/09/24 20:54:51, sugoi1 wrote:
> On 2014/09/24 20:47:50, reed1 wrote:
> > Is there a need/desire to have the 3d array be refcounted? If so, seems like
> >
> > Create(SkData*, int width, int height, int depth)
> >
> > could work for the effect constructor.
>
> Sure, that would be ok with me, but I'd need to add something like:
> GrLockAndRefCachedDataTexture(GrContext* ctx, const SkData& data, int width,
int
> height, int depth, const GrTextureParams* params) {...}
> ... and all associated functions. Does that make sense? I can do that is it's
ok
> with you guys.
"IF it's ok with you guys"
On 2014/09/24 20:54:51, sugoi1 wrote:
> On 2014/09/24 20:47:50, reed1 wrote:
> > Is there a need/desire to have the 3d array be refcounted? If so, seems like
> >
> > Create(SkData*, int width, int height, int depth)
> >
> > could work for the effect constructor.
>
> Sure, that would be ok with me, but I'd need to add something like:
> GrLockAndRefCachedDataTexture(GrContext* ctx, const SkData& data, int width,
int
> height, int depth, const GrTextureParams* params) {...}
> ... and all associated functions. Does that make sense? I can do that is it's
ok
> with you guys.
I don't think you need anything in the SkGr glue code. Your asNewEffect() code
can just use a custom cache domain:
static const GrCacheID::Domain gCubeDomain = GrCacheID::GenerateDomain();
GrCacheID::Key key;
// fill in key with something that uniquely defines this cube here
GrResourceKey resourceKey = GrResourceKey(GrCacheID(gCubeDomain , key),
GrTexture::resourceType(), 0);
SkAutoTUnref<GrTexture> textureCube(
static_cast<GrPathRange*>(ctx->findAndRefCachedResource(resourceKey)));
if (NULL == textureCube) {
textureCube.reset(ctx->createTexture(...));
ctx->addResourceToCache(resourceKey, textureCube);
}
I assume you aren't planning to use mip maps or non-clamp wrap modes for these
cubes, right? If not, let's make Ganesh not support that use case. We can always
add it later but there are complexities for GPUs that have limited mipmap/wrap
support and how that interacts with the cache.
On 2014/09/24 21:20:27, bsalomon wrote: > static_cast<GrPathRange*>(ctx->findAndRefCachedResource(resourceKey))); this shouldn't use GrPathRange, obviously. I pasted existing code into my comment and missed this edit. Should be GrTexture.
On 2014/09/24 21:22:00, bsalomon wrote: > On 2014/09/24 21:20:27, bsalomon wrote: > > > > static_cast<GrPathRange*>(ctx->findAndRefCachedResource(resourceKey))); > > this shouldn't use GrPathRange, obviously. I pasted existing code into my > comment and missed this edit. Should be GrTexture. Cool, thanks for the code snippet Brian! I'll make the change tomorrow.
Here's the new version with SkData instead of SkBitmap https://codereview.chromium.org/580863004/diff/120001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/120001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:55: SkAutoMutexAcquire ama(fInitMutex); I added a mutex here for safety, although this *may* be overkill (maybe not) https://codereview.chromium.org/580863004/diff/120001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:293: key.fData64[1] = fCubeDimension; I made the texture's key out of the current object's data's address and cube dimension. I doubt this is completely bullet proof as data could be reallocated at the same address, so maybe the texture should be purged when the SkColorProfileFilter object is deleted OR maybe there's a smarter way to make this key.
https://codereview.chromium.org/580863004/diff/120001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/120001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:293: key.fData64[1] = fCubeDimension; On 2014/09/25 20:21:56, sugoi1 wrote: > I made the texture's key out of the current object's data's address and cube > dimension. I doubt this is completely bullet proof as data could be reallocated > at the same address, so maybe the texture should be purged when the > SkColorProfileFilter object is deleted OR maybe there's a smarter way to make > this key. I could add a member to SkColorProfileFilter which is simply an ID atomically increased from a global static counter when the object is constructed and use that as a key for the texture, but there might be a simpler way, although I can't think of one right now. I just realized this is pretty much what SkNextPixelRefGenerationID() does, so that's probably the right way to do it.
https://codereview.chromium.org/580863004/diff/140001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/140001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:25: int32_t SkNextColorProfileGenerationID() { Added SkNextColorProfileGenerationID(), similar to SkNextPixelRefGenerationID(), to solve the texture key problem in SkColorProfileFilter::asFragmentProcessor().
I'm starting to think that adding 3D textures might warrant its own CL independent of color profile. There are several things we need to either support or have failure modes for: making rendertarget/textures, copying 3D textures, wrapping external textures, ... Basically, I think we need to go through every public method in GrGpu.h and GrDrawTarget.h and decide whether 3D textures are supported and make it work, or decide not to support it and have a well defined failure mode. https://codereview.chromium.org/580863004/diff/140001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/140001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:41: int32_t fGenerationID; If this never changes (since color filters are immutable), we should call it a UniqueID and not a GenerationID to be consistent with other parts of Skia. https://codereview.chromium.org/580863004/diff/140001/src/gpu/gl/GrGpuGL.cpp File src/gpu/gl/GrGpuGL.cpp (right): https://codereview.chromium.org/580863004/diff/140001/src/gpu/gl/GrGpuGL.cpp#... src/gpu/gl/GrGpuGL.cpp:573: this->glCaps().texStorageSupport(); Is texture storage not supported for 3D? https://codereview.chromium.org/580863004/diff/140001/src/gpu/gl/GrGpuGL.cpp#... src/gpu/gl/GrGpuGL.cpp:2072: GrGLenum textureType = (texture->desc().fDepth > 0) ? GR_GL_TEXTURE_3D : GR_GL_TEXTURE_2D; Why don't we have GrGLTexture::textureType()? (Actually GL calls this a "target" not a "type" so it probably should be GrGLTexture::textureTarget(), glTarget(), glTargetType() or something like that). https://codereview.chromium.org/580863004/diff/140001/src/gpu/gl/GrGpuGL.cpp#... src/gpu/gl/GrGpuGL.cpp:2137: if (GR_GL_TEXTURE_3D == textureType) { Do all ES contexts that support 3D textures also support mip'ing and wrapping 3D textures that are NPOT?
On 2014/09/26 14:23:51, bsalomon wrote: > I'm starting to think that adding 3D textures might warrant its own CL > independent of color profile. > > There are several things we need to either support or have failure modes for: > making rendertarget/textures, copying 3D textures, wrapping external textures, > ... Basically, I think we need to go through every public method in GrGpu.h and > GrDrawTarget.h and decide whether 3D textures are supported and make it work, or > decide not to support it and have a well defined failure mode. Since Chrome is going to have to support platforms without 3D texture support, (e.g., plain GLES2), another option would be to simply implement only the planned fallback path (a 2D texture version), and not bother with real 3D textures in Skia or the command buffer at all.
On 2014/09/26 14:23:51, bsalomon wrote: > I'm starting to think that adding 3D textures might warrant its own CL > independent of color profile. > > There are several things we need to either support or have failure modes for: > making rendertarget/textures, copying 3D textures, wrapping external textures, > ... Basically, I think we need to go through every public method in GrGpu.h and > GrDrawTarget.h and decide whether 3D textures are supported and make it work, or > decide not to support it and have a well defined failure mode. I don't really need anything but "creating a 3D texture", "caching it when appropriate" and "using it in a shader". I could have everything else explicitly disabled.
On 2014/09/26 14:58:28, Stephen White wrote: > On 2014/09/26 14:23:51, bsalomon wrote: > > I'm starting to think that adding 3D textures might warrant its own CL > > independent of color profile. > > > > There are several things we need to either support or have failure modes for: > > making rendertarget/textures, copying 3D textures, wrapping external textures, > > ... Basically, I think we need to go through every public method in GrGpu.h > and > > GrDrawTarget.h and decide whether 3D textures are supported and make it work, > or > > decide not to support it and have a well defined failure mode. > > Since Chrome is going to have to support platforms without 3D texture support, > (e.g., plain GLES2), another option would be to simply implement only the > planned fallback path (a 2D texture version), and not bother with real 3D textures in > Skia or the command buffer at all. Agreed, but I'm doing this purely for performance concerns. Since this is something that could happen at draw time someday, mostly on Desktops using multiple displays and which only requires support for OpenGL 1.2, I don't know how likely it would be that a desktop wouldn't have support for OpenGL 1.2, especially desktops with these pre OpenGL 1.2 drivers that also use a monitor that has an ICC profile (seems like an unlikely config to me, but I could be wrong, I don't know how old the oldest color profiled monitors are).
On 2014/09/26 15:44:08, sugoi1 wrote: > On 2014/09/26 14:58:28, Stephen White wrote: > > On 2014/09/26 14:23:51, bsalomon wrote: > > > I'm starting to think that adding 3D textures might warrant its own CL > > > independent of color profile. > > > > > > There are several things we need to either support or have failure modes > for: > > > making rendertarget/textures, copying 3D textures, wrapping external > textures, > > > ... Basically, I think we need to go through every public method in GrGpu.h > > and > > > GrDrawTarget.h and decide whether 3D textures are supported and make it > work, > > or > > > decide not to support it and have a well defined failure mode. > > > > Since Chrome is going to have to support platforms without 3D texture support, > > > (e.g., plain GLES2), another option would be to simply implement only the > > planned fallback path (a 2D texture version), and not bother with real 3D > textures in > > Skia or the command buffer at all. > > Agreed, but I'm doing this purely for performance concerns. Since this is > something that could happen at draw time someday, mostly on Desktops using > multiple displays and which only requires support for OpenGL 1.2, I don't know > how likely it would be that a desktop wouldn't have support for OpenGL 1.2, > especially desktops with these pre OpenGL 1.2 drivers that also use a monitor > that has an ICC profile (seems like an unlikely config to me, but I could be > wrong, I don't know how old the oldest color profiled monitors are). It sounds to me like most of the time the code is invoked there will be 3D texture support. WRT to the NPOT/MIP stuff, if we want to keep the code pretty simple I think we have two options: 1. 3D textures don't work with non-clamp or mip maps at all in Ganesh (at least for now) 2. We will not allow 3D textures unless there are no MIP/wrap limitations for the GL context. 1 seems like the right way forward to me since we don't currently have a use case for MIPS or wrapping 3D textures AFAIK.
On 2014/09/26 15:44:08, sugoi1 wrote: > On 2014/09/26 14:58:28, Stephen White wrote: > > On 2014/09/26 14:23:51, bsalomon wrote: > > > I'm starting to think that adding 3D textures might warrant its own CL > > > independent of color profile. > > > > > > There are several things we need to either support or have failure modes > for: > > > making rendertarget/textures, copying 3D textures, wrapping external > textures, > > > ... Basically, I think we need to go through every public method in GrGpu.h > > and > > > GrDrawTarget.h and decide whether 3D textures are supported and make it > work, > > or > > > decide not to support it and have a well defined failure mode. > > > > Since Chrome is going to have to support platforms without 3D texture support, > > > (e.g., plain GLES2), another option would be to simply implement only the > > planned fallback path (a 2D texture version), and not bother with real 3D > textures in > > Skia or the command buffer at all. > > Agreed, but I'm doing this purely for performance concerns. Since this is > something that could happen at draw time someday, mostly on Desktops using > multiple displays and which only requires support for OpenGL 1.2, I don't know > how likely it would be that a desktop wouldn't have support for OpenGL 1.2, > especially desktops with these pre OpenGL 1.2 drivers that also use a monitor > that has an ICC profile (seems like an unlikely config to me, but I could be > wrong, I don't know how old the oldest color profiled monitors are). Hmmm... looking at GrGLAssembleGLInterface(), it seems like skia doesn't support anything below version 1.5, so I think having OpenGL 1.5 is a requirement to use any GL command in skia, right? I guess the concern is with GrGLAssembleGLESInterface() where "GL_OES_texture_3D" is required. Is this the one used by chromium?
On 2014/09/26 15:50:46, sugoi1 wrote: > On 2014/09/26 15:44:08, sugoi1 wrote: > > On 2014/09/26 14:58:28, Stephen White wrote: > > > On 2014/09/26 14:23:51, bsalomon wrote: > > > > I'm starting to think that adding 3D textures might warrant its own CL > > > > independent of color profile. > > > > > > > > There are several things we need to either support or have failure modes > > for: > > > > making rendertarget/textures, copying 3D textures, wrapping external > > textures, > > > > ... Basically, I think we need to go through every public method in > GrGpu.h > > > and > > > > GrDrawTarget.h and decide whether 3D textures are supported and make it > > work, > > > or > > > > decide not to support it and have a well defined failure mode. > > > > > > Since Chrome is going to have to support platforms without 3D texture > support, > > > > > (e.g., plain GLES2), another option would be to simply implement only the > > > planned fallback path (a 2D texture version), and not bother with real 3D > > textures in > > > Skia or the command buffer at all. > > > > Agreed, but I'm doing this purely for performance concerns. Since this is > > something that could happen at draw time someday, mostly on Desktops using > > multiple displays and which only requires support for OpenGL 1.2, I don't know > > how likely it would be that a desktop wouldn't have support for OpenGL 1.2, > > especially desktops with these pre OpenGL 1.2 drivers that also use a monitor > > that has an ICC profile (seems like an unlikely config to me, but I could be > > wrong, I don't know how old the oldest color profiled monitors are). > > Hmmm... looking at GrGLAssembleGLInterface(), it seems like skia doesn't support > anything below version 1.5, so I think having OpenGL 1.5 is a requirement to use > any GL command in skia, right? > I guess the concern is with GrGLAssembleGLESInterface() where > "GL_OES_texture_3D" is required. Is this the one used by chromium? That's true when running standalone Skia on desktop GL. But Chrome wraps desktop GL in a ES command buffer that Skia sees. You'd have to check what the min GL version for Chrome is, but I highly doubt it's < 1.5.
Reduced this cl to contain only the color filter and removed changes related to 3D textures. I'll keep it this way until it is used and we have performance data related to it, so I'll add 3D texture support at that point if performance improvements are required.
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:69: SkAutoMutexAcquire ama(fInitMutex); I'm concerned about the cost of taking this mutex once per span. reed@: is there a way to initialize this once before all CPU processing of a color filter begins? i.e., something equivalent to SkShader's createContext() call? If not, could we add one? Failing that, I suppose we could create these unconditionally in the constructor, but that'll be wasted effort (and memory) for the GPU path.
https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:322: key.fData32[0] = fGenerationID; I think you have to zero the other two words. Also, are there only a few standard cubes we'll use? If so I think we're missing a chance to really take advantage of the cache here by using the gen id. https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:329: desc.fConfig = kBGRA_8888_GrPixelConfig; BGRA support is a bit sketchier than RGBA. It's minor but if you can just make the original cube store RGBA and upload that here, it'd be good.
https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:87: SkScalar index = SkScalarMul(i, scale); OTOH, this looks to be mostly ALU, except for the conditional (which could be written as predicated execution). It might be worth trying it inline (ie., without precomputation) to see how much of a win these precomputed LUTs really are, especially since it would allow us to take the mutex out.
https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:69: SkAutoMutexAcquire ama(fInitMutex); On 2014/09/29 19:16:23, Stephen White wrote: > I'm concerned about the cost of taking this mutex once per span. > > reed@: is there a way to initialize this once before all CPU processing of a > color filter begins? i.e., something equivalent to SkShader's createContext() > call? If not, could we add one? > > Failing that, I suppose we could create these unconditionally in the > constructor, but that'll be wasted effort (and memory) for the GPU path. Have you looked at how we use SkOnce in gradients? That is how we do this sort of thing (lazily building a cache for an effect). https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:92: fColorToFactors[0][i] = SK_Scalar1 - fColorToFactors[1][i]; I wonder if we stored fColorToFactors as small ints (pow-2), we might save all the costs of float->int and int->float when we apply this to a color... https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:118: SkScalar factor = SkScalarMul(fColorToFactors[x][r], btw : we just use * now, no need for the monster SkScalarMul macro https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:123: rOut += SkScalarMul(SkColorGetR(lutColor), factor); btw : each of these muls involves int->float as well (much worse than a multiply) https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:129: dst[i] = SkPreMultiplyColor(SkColorSetARGB(a, btw : might be faster to perform the "premul" with the floats you already have, instead of doing it in ints w/ this function. (wag)
https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:69: SkAutoMutexAcquire ama(fInitMutex); On 2014/09/29 19:34:17, reed1 wrote: > On 2014/09/29 19:16:23, Stephen White wrote: > > I'm concerned about the cost of taking this mutex once per span. > > > > reed@: is there a way to initialize this once before all CPU processing of a > > color filter begins? i.e., something equivalent to SkShader's createContext() > > call? If not, could we add one? > > > > Failing that, I suppose we could create these unconditionally in the > > constructor, but that'll be wasted effort (and memory) for the GPU path. > > Have you looked at how we use SkOnce in gradients? That is how we do this sort > of thing (lazily building a cache for an effect). Done. Used SkOnce. https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:87: SkScalar index = SkScalarMul(i, scale); On 2014/09/29 19:28:40, Stephen White wrote: > OTOH, this looks to be mostly ALU, except for the conditional (which could be > written as predicated execution). It might be worth trying it inline (ie., > without precomputation) to see how much of a win these precomputed LUTs really > are, especially since it would allow us to take the mutex out. Acknowledged. Used SkOnce for now though. https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:92: fColorToFactors[0][i] = SK_Scalar1 - fColorToFactors[1][i]; On 2014/09/29 19:34:18, reed1 wrote: > I wonder if we stored fColorToFactors as small ints (pow-2), we might save all > the costs of float->int and int->float when we apply this to a color... These values are int the [0,1] range. Do we usually discretize values like these to integer values? I do need to multiply 3 floating point numbers together to get the "weight" of 1 of each of the 8 samples used to get 1 color value from the cube... https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:118: SkScalar factor = SkScalarMul(fColorToFactors[x][r], On 2014/09/29 19:34:18, reed1 wrote: > btw : we just use * now, no need for the monster SkScalarMul macro Done. https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:123: rOut += SkScalarMul(SkColorGetR(lutColor), factor); On 2014/09/29 19:34:17, reed1 wrote: > btw : each of these muls involves int->float as well (much worse than a > multiply) Acknowledged. Not sure how to get around that outside of making another lut. https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:129: dst[i] = SkPreMultiplyColor(SkColorSetARGB(a, On 2014/09/29 19:34:17, reed1 wrote: > btw : might be faster to perform the "premul" with the floats you already have, > instead of doing it in ints w/ this function. (wag) Done. https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:322: key.fData32[0] = fGenerationID; On 2014/09/29 19:27:28, bsalomon wrote: > I think you have to zero the other two words. > > Also, are there only a few standard cubes we'll use? If so I think we're missing > a chance to really take advantage of the cache here by using the gen id. I'll need to see how these are used, but since these will typically be the result of a transform from a color space to another color space, it is unlikely that there will only be a few known cubes used. There may be some cubes that are heavily reused, but I think the caching of these with the associated SkColorFilter should be done by the caller when it makes sense, if possible. https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:329: desc.fConfig = kBGRA_8888_GrPixelConfig; On 2014/09/29 19:27:28, bsalomon wrote: > BGRA support is a bit sketchier than RGBA. It's minor but if you can just make > the original cube store RGBA and upload that here, it'd be good. Done. I just added a "bgra" swizzle to the texture access for now. I'm not sure what the final format given by the QCMS library will be (although we'll be the once choosing it), so this should be flexible enough to allow me to change it easily later on, if required.
Fixed lines over 100 char and added a lut to avoid int->float conversions.
https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:323: // Note that input alpha is not modified by this shader Do you have to unpremul the input and then premul it?
https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:323: // Note that input alpha is not modified by this shader On 2014/09/30 15:49:06, bsalomon wrote: > Do you have to unpremul the input and then premul it? Good point. I do it in CPU, so I should definitely do it also in the shader.
https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:323: // Note that input alpha is not modified by this shader On 2014/09/30 15:49:06, bsalomon wrote: > Do you have to unpremul the input and then premul it? Done.
reed@google.com changed reviewers: + bungeman@google.com, mtklein@google.com
Why is fCache ref-counted? It is lazily initialized ... can it be lazily allocated? (SkLazyPtr?) https://codereview.chromium.org/580863004/diff/200001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/200001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:16: static SkColorProfileFilter* Create(SkData* cubeData, int cubeDimension); // document cubeData's contents. https://codereview.chromium.org/580863004/diff/200001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:66: SkData* fCubeData; Since you use it for fCache, you *could* use SkAutoTUnref<> or SkAutoData to manage fCubeData as well. https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:56: : fCubeData(cubeData) btw : fCubeData(SkRef(cubData)) -- no need to say ->ref() later one https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:92: static SkScalar inv8bit = SkScalarInvert(SkIntToScalar(255)); is this guy const? https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:94: cache->fLutStorage.reset(512 * sizeof(int) + 768 * sizeof(SkScalar)); // 512 is from ... // 768 is from ... https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:106: SkScalar index = scale * SkIntToScalar(i); tiny nit : SkIntToScalar not needed afaik
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/160001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:87: SkScalar index = SkScalarMul(i, scale); On 2014/09/29 21:47:49, sugoi1 wrote: > On 2014/09/29 19:28:40, Stephen White wrote: > > OTOH, this looks to be mostly ALU, except for the conditional (which could be > > written as predicated execution). It might be worth trying it inline (ie., > > without precomputation) to see how much of a win these precomputed LUTs really > > are, especially since it would allow us to take the mutex out. > > Acknowledged. Used SkOnce for now though. In order to test these two approaches, I'd like to see a bench added with this change.
https://codereview.chromium.org/580863004/diff/200001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/200001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:16: static SkColorProfileFilter* Create(SkData* cubeData, int cubeDimension); On 2014/09/30 17:07:34, reed1 wrote: > // document cubeData's contents. Done. https://codereview.chromium.org/580863004/diff/200001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:66: SkData* fCubeData; On 2014/09/30 17:07:34, reed1 wrote: > Since you use it for fCache, you *could* use SkAutoTUnref<> or SkAutoData to > manage fCubeData as well. Done. https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:56: : fCubeData(cubeData) On 2014/09/30 17:07:34, reed1 wrote: > btw : fCubeData(SkRef(cubData)) -- no need to say ->ref() later one Done. https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:92: static SkScalar inv8bit = SkScalarInvert(SkIntToScalar(255)); On 2014/09/30 17:07:34, reed1 wrote: > is this guy const? Done. https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:94: cache->fLutStorage.reset(512 * sizeof(int) + 768 * sizeof(SkScalar)); On 2014/09/30 17:07:34, reed1 wrote: > // 512 is from ... > // 768 is from ... Done. https://codereview.chromium.org/580863004/diff/200001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:106: SkScalar index = scale * SkIntToScalar(i); On 2014/09/30 17:07:34, reed1 wrote: > tiny nit : SkIntToScalar not needed afaik Done. https://codereview.chromium.org/580863004/diff/260001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/260001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:70: SkAutoTDelete<ColorProfileCache> fCache; Removed ref counting and changed SkAutoTUnref for SkAutoTDelete.
Once again updated to ToT to follow the GrFragmentProcessor API changes. PTAL!
Thanks for writing the bench! https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... File bench/ColorProfileBench.cpp (right): https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:22: fSize = SkISize::Make(1024, 768); Maybe we should make this 2Kx1.5K, to be closer to what people will see in a full-screen photo viewer. https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:71: canvas->clipRect(SkRect::MakeXYWH(SkIntToScalar(x), SkIntToScalar(y), Now that we have a bench, could you try it without this clipRect? If there's no delta, we can remove it. (It was mostly necessary for SkImageFilters, when they were too dumb to optimize their intermediate buffers.) https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:84: paint.setColorFilter(colorProfile); Presumably, the paint color will default to black (or white? I can't remember). Either way, won't this only be accessing a single element of the cube? IWBN to make sure that we exercise the whole cube (or at least, a good part of it). Maybe we should fill it with a gradient (as in your GM) or random pixels.
https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... File bench/ColorProfileBench.cpp (right): https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:13: bool fInitialized; Trivial, but this is the same as SkToBool(fCubeData) https://codereview.chromium.org/580863004/diff/320001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/320001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:19: static SkColorProfileFilter* Create(SkData* cubeData, int cubeDimension); Can this return SkColorFilter* rather than the derived class? https://codereview.chromium.org/580863004/diff/320001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:39: class ColorProfileCache { Why is this dynamically allocated when it is created in the constructor of the filter? https://codereview.chromium.org/580863004/diff/320001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/320001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:369: return (NULL == textureCube) ? NULL : GrColorProfileEffect::Create(textureCube); SkAutoTUnref casts to bool. This should just be "textureCube ? ..." I think (and in the above if)
https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... File bench/ColorProfileBench.cpp (right): https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:13: bool fInitialized; On 2014/10/03 19:45:00, bsalomon wrote: > Trivial, but this is the same as SkToBool(fCubeData) Done. https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:22: fSize = SkISize::Make(1024, 768); On 2014/10/03 19:40:49, Stephen White wrote: > Maybe we should make this 2Kx1.5K, to be closer to what people will see in a > full-screen photo viewer. Done. Changed it to 2880x1800 (Curent MacBook Pro resolution). https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:71: canvas->clipRect(SkRect::MakeXYWH(SkIntToScalar(x), SkIntToScalar(y), On 2014/10/03 19:40:49, Stephen White wrote: > Now that we have a bench, could you try it without this clipRect? If there's no > delta, we can remove it. (It was mostly necessary for SkImageFilters, when they > were too dumb to optimize their intermediate buffers.) Done. https://codereview.chromium.org/580863004/diff/320001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:84: paint.setColorFilter(colorProfile); On 2014/10/03 19:40:49, Stephen White wrote: > Presumably, the paint color will default to black (or white? I can't remember). > Either way, won't this only be accessing a single element of the cube? IWBN to > make sure that we exercise the whole cube (or at least, a good part of it). > Maybe we should fill it with a gradient (as in your GM) or random pixels. Done. https://codereview.chromium.org/580863004/diff/320001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/320001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:19: static SkColorProfileFilter* Create(SkData* cubeData, int cubeDimension); On 2014/10/03 19:45:00, bsalomon wrote: > Can this return SkColorFilter* rather than the derived class? Done. https://codereview.chromium.org/580863004/diff/320001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:39: class ColorProfileCache { On 2014/10/03 19:45:00, bsalomon wrote: > Why is this dynamically allocated when it is created in the constructor of the filter? If I don't keep it as a pointer, then I get an error for calling ColorProfileCache::getProcessingLuts (which isn't const) from SkColorProfileFilter::filterSpan (which is const). I changed it to a mutable member instead. https://codereview.chromium.org/580863004/diff/320001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/320001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:369: return (NULL == textureCube) ? NULL : GrColorProfileEffect::Create(textureCube); On 2014/10/03 19:45:00, bsalomon wrote: > SkAutoTUnref casts to bool. This should just be "textureCube ? ..." I think (and > in the above if) Done.
https://codereview.chromium.org/580863004/diff/340001/bench/ColorProfileBench... File bench/ColorProfileBench.cpp (right): https://codereview.chromium.org/580863004/diff/340001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:62: SkCanvas canvas(fBitmap); Probably not a huge deal, but this will create a raster canvas, and in GPU mode I think this will cause a texture upload on the first draw. https://codereview.chromium.org/580863004/diff/340001/gm/colorprofile.cpp File gm/colorprofile.cpp (right): https://codereview.chromium.org/580863004/diff/340001/gm/colorprofile.cpp#new... gm/colorprofile.cpp:119: drawClippedBitmap(canvas, 10, 10, paint); Not sure if drawClippedBitmap() is necessary here either, but if it isn't, consider removing it. https://codereview.chromium.org/580863004/diff/340001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/340001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:38: // The cache is initialized on-demand when getColorLuts is called. Nit: I don't see getColorLuts() anywhere.. stale comment?
https://codereview.chromium.org/580863004/diff/340001/bench/ColorProfileBench... File bench/ColorProfileBench.cpp (right): https://codereview.chromium.org/580863004/diff/340001/bench/ColorProfileBench... bench/ColorProfileBench.cpp:62: SkCanvas canvas(fBitmap); On 2014/10/06 16:40:40, Stephen White wrote: > Probably not a huge deal, but this will create a raster canvas, and in GPU mode > I think this will cause a texture upload on the first draw. Acknowledged. Since we don't know which mode we're about to use (AFAIK, tell me if there's a way to know), not sure how to fix that. https://codereview.chromium.org/580863004/diff/340001/gm/colorprofile.cpp File gm/colorprofile.cpp (right): https://codereview.chromium.org/580863004/diff/340001/gm/colorprofile.cpp#new... gm/colorprofile.cpp:119: drawClippedBitmap(canvas, 10, 10, paint); On 2014/10/06 16:40:40, Stephen White wrote: > Not sure if drawClippedBitmap() is necessary here either, but if it isn't, > consider removing it. Done. https://codereview.chromium.org/580863004/diff/340001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/340001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:38: // The cache is initialized on-demand when getColorLuts is called. On 2014/10/06 16:40:41, Stephen White wrote: > Nit: I don't see getColorLuts() anywhere.. stale comment? Done.
gpu lgtm https://codereview.chromium.org/580863004/diff/320001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/320001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:39: class ColorProfileCache { I don't fully grok the immutability requirements for SkPaint effects. I'd like reed@ to look at this part.
LGTM
noel@chromium.org changed reviewers: + noel@chromium.org
Like this btw, and some questions. https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:14: class SK_API SkColorProfileFilter : public SkColorFilter { SkColorProfileFilter? Would to makes sense to call SkColorCubeFilter (cubes can be used for more than just profiles, they are not used for profiles per say, they are used for color-transforms though). More generally, what if Skia wanted to create a color transform from RGB to CMKY in future. How would that be handled? https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:17: // cubeDimension * cubeDimension * cubeDimension * sizeof(SkPMColor) SkPMColor? The color in the cube are pre-multiplied? https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:51: // we need to recompute the corresponding cache values. The cached values of what? The ColorProfileCache names makes me think you are caching color profiles. I guess I'm not clear on what's being cached based on its name. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:43: size_t minMemorySize = sizeof(uint8_t) * 4 * cubeDimension * cubeDimension * cubeDimension; 4 surprised me? Why is that? Should it be 3? Or is the cube data RGBA or something? https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:89: static const SkScalar inv8bit = SkScalarInvert(SkIntToScalar(255)); Would it make sense to define thsi useful constant in SkScalar? I see SK_Scalar1 below, so was thinking we could have SK_ScalarInverse255, or SK_Scalar1On255 or something? https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:128: SkPMColor* colorCube = (SkPMColor*)fCubeData->data(); Again, cube data pre-multiplied, not sure about that. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:136: for (int x = 0; x < 2; ++x) { /question: the algorthim implemented here is tri-linear interpolation? https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:151: dst[i] = SkPackARGB32(a, You un-premultiply the src data, do you need to pre-multiply the output data? Maybe SkPackARGB32 does that? https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:168: int cubeDimension = buffer.readInt(); For future-proofing, should you serialize a LUT data format type here, say if we wanted to support 16-bit of float LUT data in future? https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:209: const GrProcessor&); Nit: const GrProcessor& fits on the line, so maybe not wrap it. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:246: , fColorCubeAccess(colorCube, "bgra", GrTextureParams::kBilerp_FilterMode) { /curious: what's the "bgra" do here? https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:307: // Unpremultiply color If I applied this filter to an image, is the image really uploaded to the GPU in premultiplied format? I'd would have thought the image texture upload would handle that for you, but maybe I'm misunderstanding how image texture upload works (I don't even know where it is done, btw). https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:308: fsBuilder->codeAppendf("\tfloat %s = max(%s.a, 0.00001);\n", nonZeroAlpha, inputColor); 0.00001 ? I'm looking at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... and some of the tips mentioned there. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:329: // Premultiply color by alpha. Note that the input alpha is not modified by this shader. Space before this line. // ... input alpha is not modified by this shader Do you need to override your filters getFlags() to state that? ... virtual uint32_t getFlags() const { return this->SkColorFilter::getFlags() | SkColorFilter::kAlphaUnchanged_Flag; }
https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:14: class SK_API SkColorProfileFilter : public SkColorFilter { On 2014/10/07 05:01:04, Noel Gordon wrote: > SkColorProfileFilter? Would to makes sense to call SkColorCubeFilter (cubes can > be used for more than just profiles, they are not used for profiles per say, > they are used for color-transforms though). > > More generally, what if Skia wanted to create a color transform from RGB to CMKY > in future. How would that be handled? +1 to rename something like SkColorCubeFilter https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:17: // cubeDimension * cubeDimension * cubeDimension * sizeof(SkPMColor) On 2014/10/07 05:01:04, Noel Gordon wrote: > SkPMColor? The color in the cube are pre-multiplied? Very good question : need to document if the cube contains SkColor (unpremul) or SkPMColor (premul and swizzled to default). My uninformed pref would be SkColor.
https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... File include/effects/SkColorProfileFilter.h (right): https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:14: class SK_API SkColorProfileFilter : public SkColorFilter { On 2014/10/07 13:41:05, reed1 wrote: > On 2014/10/07 05:01:04, Noel Gordon wrote: > > SkColorProfileFilter? Would to makes sense to call SkColorCubeFilter (cubes > can > > be used for more than just profiles, they are not used for profiles per say, > > they are used for color-transforms though). > > > > More generally, what if Skia wanted to create a color transform from RGB to > CMKY > > in future. How would that be handled? > > +1 to rename something like SkColorCubeFilter Done. https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:17: // cubeDimension * cubeDimension * cubeDimension * sizeof(SkPMColor) On 2014/10/07 13:41:05, reed1 wrote: > On 2014/10/07 05:01:04, Noel Gordon wrote: > > SkPMColor? The color in the cube are pre-multiplied? > > Very good question : need to document if the cube contains SkColor (unpremul) or > SkPMColor (premul and swizzled to default). My uninformed pref would be SkColor. Changed to SkColor. The cube's alpha is ignored. https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... include/effects/SkColorProfileFilter.h:51: // we need to recompute the corresponding cache values. On 2014/10/07 05:01:04, Noel Gordon wrote: > The cached values of what? The ColorProfileCache names makes me think you are > caching color profiles. I guess I'm not clear on what's being cached based on > its name. Renamed to ColorCubeProcessingCache https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... File src/effects/SkColorProfileFilter.cpp (right): https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:43: size_t minMemorySize = sizeof(uint8_t) * 4 * cubeDimension * cubeDimension * cubeDimension; On 2014/10/07 05:01:04, Noel Gordon wrote: > 4 surprised me? Why is that? Should it be 3? Or is the cube data RGBA or > something? Yes, it's RGBA. If it wasn't, it would need to be re-packed as RGBA before the upload anyway, so might as well save time and have it in the right format right away. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:89: static const SkScalar inv8bit = SkScalarInvert(SkIntToScalar(255)); On 2014/10/07 05:01:04, Noel Gordon wrote: > Would it make sense to define thsi useful constant in SkScalar? I see > SK_Scalar1 below, so was thinking we could have SK_ScalarInverse255, or > SK_Scalar1On255 or something? I could follow up on this if this is very common, but I'll keep it here in this cl for simplicity. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:128: SkPMColor* colorCube = (SkPMColor*)fCubeData->data(); On 2014/10/07 05:01:04, Noel Gordon wrote: > Again, cube data pre-multiplied, not sure about that. Changed to SkColor https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:136: for (int x = 0; x < 2; ++x) { On 2014/10/07 05:01:04, Noel Gordon wrote: > /question: the algorthim implemented here is tri-linear interpolation? yes https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:151: dst[i] = SkPackARGB32(a, On 2014/10/07 05:01:04, Noel Gordon wrote: > You un-premultiply the src data, do you need to pre-multiply the output data? > Maybe SkPackARGB32 does that? No, if you look at the math, rOut, gOut and bOut are all multiplied by aOut. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:168: int cubeDimension = buffer.readInt(); On 2014/10/07 05:01:04, Noel Gordon wrote: > For future-proofing, should you serialize a LUT data format type here, say if we > wanted to support 16-bit of float LUT data in future? I'd rather serialize it when it's supported. Also, since I'm serializing a number that can't exceed 64 in a 32bit int, I could always pack the data type and the size into 2 shorts without changing the serialization format. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:209: const GrProcessor&); On 2014/10/07 05:01:04, Noel Gordon wrote: > Nit: const GrProcessor& fits on the line, so maybe not wrap it. Done. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:246: , fColorCubeAccess(colorCube, "bgra", GrTextureParams::kBilerp_FilterMode) { On 2014/10/07 05:01:04, Noel Gordon wrote: > /curious: what's the "bgra" do here? It's swizzling the bytes, which are stored in BGRA format and loaded as the more widely supported RGBA texture format. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:307: // Unpremultiply color On 2014/10/07 05:01:04, Noel Gordon wrote: > If I applied this filter to an image, is the image really uploaded to the GPU in > premultiplied format? I'd would have thought the image texture upload would > handle that for you, but maybe I'm misunderstanding how image texture upload > works (I don't even know where it is done, btw). Images are always considered to be in premultiplied space in skia, AFAIK. This code is based on what SkColorMatrixFilter does. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:308: fsBuilder->codeAppendf("\tfloat %s = max(%s.a, 0.00001);\n", nonZeroAlpha, inputColor); On 2014/10/07 05:01:04, Noel Gordon wrote: > 0.00001 ? I'm looking at > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > and some of the tips mentioned there. This code is also based on what SkColorMatrixFilter does. https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... src/effects/SkColorProfileFilter.cpp:329: // Premultiply color by alpha. Note that the input alpha is not modified by this shader. On 2014/10/07 05:01:04, Noel Gordon wrote: > Space before this line. > > // ... input alpha is not modified by this shader > > Do you need to override your filters getFlags() to state that? ... > > virtual uint32_t getFlags() const > { > return this->SkColorFilter::getFlags() | > SkColorFilter::kAlphaUnchanged_Flag; > } > Done.
https://codereview.chromium.org/580863004/diff/380001/include/effects/SkColor... File include/effects/SkColorCubeFilter.h (right): https://codereview.chromium.org/580863004/diff/380001/include/effects/SkColor... include/effects/SkColorCubeFilter.h:16: // cubeData must containt a 3D data in the form of cube of the size: nit: If you used /** .... */ it will get scooped into Doxygen https://codereview.chromium.org/580863004/diff/380001/include/effects/SkColor... include/effects/SkColorCubeFilter.h:18: // This cube contains a transform where (x,y,z) maps to the (r,g,b). // The alpha components of the colors are ignored (treated as 0xFF) https://codereview.chromium.org/580863004/diff/380001/src/effects/SkColorCube... File src/effects/SkColorCubeFilter.cpp (right): https://codereview.chromium.org/580863004/diff/380001/src/effects/SkColorCube... src/effects/SkColorCubeFilter.cpp:139: SkScalar rOut(0), gOut(0), bOut(0), aOut(SkIntToScalar(a)); nit : aOut sounds like it will also be computed/accumulated like rOut etc. perhaps move this to line 155: const SkScalar aScale = SkIntToScalar(a); just bikeshedding, feel free to ignore.
https://codereview.chromium.org/580863004/diff/380001/include/effects/SkColor... File include/effects/SkColorCubeFilter.h (right): https://codereview.chromium.org/580863004/diff/380001/include/effects/SkColor... include/effects/SkColorCubeFilter.h:16: // cubeData must containt a 3D data in the form of cube of the size: On 2014/10/07 16:20:48, reed1 wrote: > nit: If you used /** .... */ it will get scooped into Doxygen Done. https://codereview.chromium.org/580863004/diff/380001/include/effects/SkColor... include/effects/SkColorCubeFilter.h:18: // This cube contains a transform where (x,y,z) maps to the (r,g,b). On 2014/10/07 16:20:48, reed1 wrote: > // The alpha components of the colors are ignored (treated as 0xFF) Done. https://codereview.chromium.org/580863004/diff/380001/src/effects/SkColorCube... File src/effects/SkColorCubeFilter.cpp (right): https://codereview.chromium.org/580863004/diff/380001/src/effects/SkColorCube... src/effects/SkColorCubeFilter.cpp:139: SkScalar rOut(0), gOut(0), bOut(0), aOut(SkIntToScalar(a)); On 2014/10/07 16:20:48, reed1 wrote: > nit : aOut sounds like it will also be computed/accumulated like rOut etc. > > perhaps move this to line 155: > > const SkScalar aScale = SkIntToScalar(a); > > just bikeshedding, feel free to ignore. Done.
lgtm Sometime we should document exactly how a colorcube works in this impl... 1. How are the filtered colors computed? 2. What about the input colors alpha? 3. Are the min and max of an axis required to be 0 and 0xFF ? If not, what is the defined result when applied?
More documentation requests: Which axis is Red -vs- Green -vs- Blue (i.e. how should the SkColor data be arranged in memory)?
On 2014/10/08 11:59:39, reed1 wrote: > More documentation requests: > > Which axis is Red -vs- Green -vs- Blue (i.e. how should the SkColor data be > arranged in memory)? Acknowledged. I'll follow up with more documentation. I'll try to this working in an actual world case and make sure this cube is of the right format to do color profile transforms ASAP.
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/580863004/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia (http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)
On 2014/10/07 15:44:51, sugoi1 wrote: > https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... > File include/effects/SkColorProfileFilter.h (right): > > https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... > include/effects/SkColorProfileFilter.h:14: class SK_API SkColorProfileFilter : > public SkColorFilter { > On 2014/10/07 13:41:05, reed1 wrote: > > On 2014/10/07 05:01:04, Noel Gordon wrote: > > > SkColorProfileFilter? Would to makes sense to call SkColorCubeFilter (cubes > > can > > > be used for more than just profiles, they are not used for profiles per say, > > > they are used for color-transforms though). > > > > > > More generally, what if Skia wanted to create a color transform from RGB to > > CMKY > > > in future. How would that be handled? > > > > +1 to rename something like SkColorCubeFilter > > Done. Good. > https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... > include/effects/SkColorProfileFilter.h:17: // cubeDimension * cubeDimension * > cubeDimension * sizeof(SkPMColor) > On 2014/10/07 13:41:05, reed1 wrote: > > On 2014/10/07 05:01:04, Noel Gordon wrote: > > > SkPMColor? The color in the cube are pre-multiplied? > > > > Very good question : need to document if the cube contains SkColor (unpremul) > or > > SkPMColor (premul and swizzled to default). My uninformed pref would be > SkColor. > > Changed to SkColor. The cube's alpha is ignored. And the format should RGBA little-endian on any platform. > https://codereview.chromium.org/580863004/diff/360001/include/effects/SkColor... > include/effects/SkColorProfileFilter.h:51: // we need to recompute the > corresponding cache values. > On 2014/10/07 05:01:04, Noel Gordon wrote: > > The cached values of what? The ColorProfileCache names makes me think you are > > caching color profiles. I guess I'm not clear on what's being cached based on > > its name. > > Renamed to ColorCubeProcessingCache Good. > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > File src/effects/SkColorProfileFilter.cpp (right): > > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > src/effects/SkColorProfileFilter.cpp:43: size_t minMemorySize = sizeof(uint8_t) > * 4 * cubeDimension * cubeDimension * cubeDimension; > On 2014/10/07 05:01:04, Noel Gordon wrote: > > 4 surprised me? Why is that? Should it be 3? Or is the cube data RGBA or > > something? > > Yes, it's RGBA. If it wasn't, it would need to be re-packed as RGBA before the > upload anyway, so might as well save time and have it in the right format right > away. Would ti be possible to support RGB and upload that cube texture? > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > src/effects/SkColorProfileFilter.cpp:89: static const SkScalar inv8bit = > SkScalarInvert(SkIntToScalar(255)); > On 2014/10/07 05:01:04, Noel Gordon wrote: > > Would it make sense to define thsi useful constant in SkScalar? I see > > SK_Scalar1 below, so was thinking we could have SK_ScalarInverse255, or > > SK_Scalar1On255 or something? > > I could follow up on this if this is very common, but I'll keep it here in this > cl for simplicity. Followup is fine. > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > src/effects/SkColorProfileFilter.cpp:128: SkPMColor* colorCube = > (SkPMColor*)fCubeData->data(); > On 2014/10/07 05:01:04, Noel Gordon wrote: > > Again, cube data pre-multiplied, not sure about that. > > Changed to SkColor Good. > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > src/effects/SkColorProfileFilter.cpp:136: for (int x = 0; x < 2; ++x) { > On 2014/10/07 05:01:04, Noel Gordon wrote: > > /question: the algorthim implemented here is tri-linear interpolation? > > yes Are you satisfied with, or have you accessed, algorithm accuracy, say compared to the direct equations given in [ http://books.google.com/books?isbn=0849337461 "Trilinear Interpolation" page 257 ] or my design note? > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > src/effects/SkColorProfileFilter.cpp:151: dst[i] = SkPackARGB32(a, > On 2014/10/07 05:01:04, Noel Gordon wrote: > > You un-premultiply the src data, do you need to pre-multiply the output data? > > Maybe SkPackARGB32 does that? > > No, if you look at the math, rOut, gOut and bOut are all multiplied by aOut. Ok noted. Do you expect stability, viz., source data passed though a filter twice unpremult -> Identity CLUT -> premult -> unpremult -> Identity CLUT -> premult remains stable ( we have that for <canvas>getImageData, putImageData, getImageData ... for example ). > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > src/effects/SkColorProfileFilter.cpp:168: int cubeDimension = buffer.readInt(); > On 2014/10/07 05:01:04, Noel Gordon wrote: > > For future-proofing, should you serialize a LUT data format type here, say if > we > > wanted to support 16-bit of float LUT data in future? > > I'd rather serialize it when it's supported. Also, since I'm serializing a > number that can't exceed 64 in a 32bit int, I could always pack the data type > and the size into 2 shorts without changing the serialization format. Yeap, that'll work fine. > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > src/effects/SkColorProfileFilter.cpp:307: // Unpremultiply color > On 2014/10/07 05:01:04, Noel Gordon wrote: > > If I applied this filter to an image, is the image really uploaded to the GPU > in > > premultiplied format? I'd would have thought the image texture upload would > > handle that for you, but maybe I'm misunderstanding how image texture upload > > works (I don't even know where it is done, btw). > > This code is also based on what SkColorMatrixFilter does. OK sounds good.
> And the format should RGBA little-endian on any platform. It has to be enough to say it is SkColor, which is defined elsewhere and has its own constraints on its swizzle/endianess. I think it is, as you say, RGBA little-endian, but I don't think that is appropriate to declare here, just in case SkColor's documentation changes in the future.
On 2014/10/08 13:05:05, Noel Gordon wrote: > Would it be possible to support RGB and upload that cube texture? Even if I did, the driver would most likely re-format it to RGBA, using the CPU, before the upload, which would cost time. The "smart" way to load RGB data, AFAIK, it to upload it as a 3/4 height (or width) RGBA texture (assuming the height or width is a multiple of 4, otherwise it gets a bit hairy) and then re-expand it properly into an RGBA texture on the GPU. That requires quite a bit of code for this to be efficient. I probably (realistically) won't have time to do this in the near future. > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > > src/effects/SkColorProfileFilter.cpp:136: for (int x = 0; x < 2; ++x) { > > On 2014/10/07 05:01:04, Noel Gordon wrote: > > > /question: the algorthim implemented here is tri-linear interpolation? > > > > yes > > Are you satisfied with, or have you accessed, algorithm accuracy, say compared > to the direct equations given in [ > http://books.google.com/books?isbn=0849337461 "Trilinear Interpolation" page 257 > ] or my design note? I can definitely improve on it, but I was going for: Step 1: Make it work with the most straightforward algorithm possible and generate reference results (for both quality and performance) Step 2: Optimize it and make sure I don't break the references created in step 1
On 2014/10/08 14:41:59, sugoi1 wrote: > On 2014/10/08 13:05:05, Noel Gordon wrote: > > Would it be possible to support RGB and upload that cube texture? > > Even if I did, the driver would most likely re-format it to RGBA, using the CPU, > before the upload, which would cost time. > The "smart" way to load RGB data, AFAIK, it to upload it as a 3/4 height (or > width) RGBA texture (assuming the height or width is a multiple of 4, otherwise > it gets a bit hairy) and then re-expand it properly into an RGBA texture on the > GPU. That requires quite a bit of code for this to be efficient. I probably > (realistically) won't have time to do this in the near future. I used three.js to do to the cube texture upload, and it calls webgl's gl.texImage2d to load it into the 2D texture sampler gl.texImage2d(gl.TEXTURE_2D, 0, gl.RGB, gl.RGB, gl.UNSIGNED_BYTE, cube); I'd be fascinated if a gl driver converted to RGBA here, but don't know enough of driver innards to say :) > https://codereview.chromium.org/580863004/diff/360001/src/effects/SkColorProf... > > > src/effects/SkColorProfileFilter.cpp:136: for (int x = 0; x < 2; ++x) { > > > On 2014/10/07 05:01:04, Noel Gordon wrote: > > > > /question: the algorthim implemented here is tri-linear interpolation? > > > > > > yes > > > > Are you satisfied with, or have you accessed, algorithm accuracy, say compared > > to the direct equations given in [ > > http://books.google.com/books?isbn=0849337461 "Trilinear Interpolation" page > 257 > > ] or my design note? > I can definitely improve on it, but I was going for: > Step 1: Make it work with the most straightforward algorithm possible and > generate reference results (for both quality and performance) > Step 2: Optimize it and make sure I don't break the references created in step 1 Yeap, sensible approach. Establish the base-line first, the golden result, then ...
On 2014/10/08 13:10:21, reed1 wrote: > > And the format should RGBA little-endian on any platform. > > It has to be enough to say it is SkColor, which is defined elsewhere and has its > own constraints on its swizzle/endianess. I think it is, as you say, RGBA > little-endian, but I don't think that is appropriate to declare here, just in > case SkColor's documentation changes in the future. Re documentation: some examples help to answer the question about how are 3D LUTs created. When qcms creates the LUT https://code.google.com/p/chromium/codesearch#chromium/src/third_party/qcms/s... the LUT data in RGB RGB RGB ... in memory: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/qcms/s... The data is [0.0 .. 1.0] and needs to be converted for this filter. Not too hard to add in A=255 during that conversion step, and do an RGBA cube texture upload I suppose (my tests used RGB cube texture uploads). If ColorSync on OSX was used to produce the LUT, I note it also creates RGB LUT data (16-bit per component, on [0 .. 2^16-1]). Search for colorSyncTexture in https://developer.apple.com/library/mac/samplecode/ImageApp/Listings/ImageFil... The data organization of the cube texture I used followed ... "The cube texture is defined with a 2d sampler uniform, along with uniforms representing the cube sampling rate and the cube resolution in the x- and y-directions (openGL uv co-ords). The color cube texture is flipped on upload to the GPU such that the R component increases in the positive y-direction (up, +v), and the G and B components increase in the positive x-direction (left, +u). Each G component is offset from the next by the color cube sampling rate to form a planar (2d) texture for sampling". for ( r in LUT[0 ..samples - 1].r ) for ( g in LUT[0 ... samples - 1].g ) for ( b in LUT[0 .. samples -1].b ) write rgb -> RGB_texture++; so fragment shader sampling of the cube could be done in uv-space (and not do my head in). The cube texture data was the same format regardless of platform swizzling, mainly because the color correction libraries generate RGB. HTH.
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/580863004/420001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
LGTM and #56 btw, nice stuff sugoi.
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/580863004/440001
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as ce686270f533b9d741ef85661ef9d78517a01b86 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
