Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1623653002:
skia: Add support for CHROMIUM_image backed textures. (Closed)
Descriptionskia: Add support for CHROMIUM_image backed textures.
I created a new abstract base class TextureStorageAllocator that consumers of
Skia can subclass and pass back to Skia. When a surface is created with a
pointer to a TextureStorageAllocator, any textures it creates, or that are
derived from the original surface, will allocate and deallocate storage using
the methods on TextureStorageAllocator.
BUG=https://code.google.com/p/chromium/issues/detail?id=579664
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1623653002
Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55
Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b
Committed: https://skia.googlesource.com/skia/+/b8d6e088590160f1198110c2371b802c1d541a36
Committed: https://skia.googlesource.com/skia/+/9a1ed5d81dbfc7d5b67b568dfe12b4033a9af154
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Compile error. #Patch Set 4 : Add test. #
Total comments: 4
Patch Set 5 : Comments from bsalomon. #Patch Set 6 : Minor cleanup. #
Total comments: 19
Patch Set 7 : Comments from salomon, round two. #Patch Set 8 : Minor cleanup. #
Total comments: 4
Patch Set 9 : Comments from bsalomon, round three. #Patch Set 10 : Add a context parameter to function pointers. #
Total comments: 10
Patch Set 11 : Comments from bsalomon. #Patch Set 12 : Comments from bsalomon, round four. #
Total comments: 8
Patch Set 13 : Comments from bsalomon, round five. #Patch Set 14 : Fix compile error. #Patch Set 15 : Re-add tests that went missing, undo changes to GrGLGpu that were causing roll failure. #Patch Set 16 : Style. #Patch Set 17 : Fix MSVS compile issue, as it doesn't support dynamic-size C arrays. #Patch Set 18 : Rebase against top of tree. New tests still expected to be broken. #Patch Set 19 : Possibly fix ANGLE test. #Patch Set 20 : Rebase against top of tree. Still expected to fail ASAN. #Patch Set 21 : Add scoped ptrs. #Patch Set 22 : Add missing ')'. #
Messages
Total messages: 83 (32 generated)
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. BUG=skia: ========== to ========== skia: Add support for CHROMIUM_image backed textures. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-01-23 06:19 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
erikchen@chromium.org changed reviewers: + bsalomon@chromium.org
bsalomon: Please review.
bsalomon@google.com changed reviewers: + bsalomon@google.com, cblume@chromium.org
Adding Chris Blume as this may have interactions with his mip map changes. https://codereview.chromium.org/1623653002/diff/60001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/1623653002/diff/60001/include/gpu/GrTypes.h#n... include/gpu/GrTypes.h:426: virtual bool allocateTextureStorage(unsigned textureId, unsigned width, unsigned height) = 0; What do you think about virtual bool allocateTextureStorage(GrBackendObject* allocatedTexture, int width, int height) ? The allocator then just tells Skia the texture ID and texture target rather than having Skia create the ID. It could work with alternative backends that don't use ints a texture IDs (e.g. Vulkan). Also I can imagine that the allocator might want more context than the ID when deallocate is called (e.g. pointer to GpuMemoryBuffer, ...). Wondering if GrGLTextureInfo should have a void* fAllocatorContext? https://codereview.chromium.org/1623653002/diff/60001/include/gpu/GrTypes.h#n... include/gpu/GrTypes.h:571: TextureStorageAllocator* fTextureStorageAllocator; We shouldn't need this here. This is used to import an already existing texture into Skia.
bsalomon@google.com changed reviewers: + egdaniel@google.com, robertphillips@google.com
+A couple more Skia people.
On 2016/01/25 20:51:09, bsalomon wrote: > Adding Chris Blume as this may have interactions with his mip map changes. I presume this CL is a stepping stone to enable a custom texture allocator. I have a question about the custom texture allocator's goals because mipmapping may make it more difficult. Is this for storing the texture data before it becomes a GL texture? Is this for shared memory where GL will use this already-allocated memory as the texture? If it is the latter, we may need to account for what GL calls mutable and immutable storage. Right now, we use TexImage2D deliberately because it is mutable. I have a patch waiting for my mipmap code to land which enables TexStorage2D, which is immutable (and thus gives the driver a useful hint when allocating). What mutable/immutable storage means for us is we do not know in advance if we will later generate mipmaps for this given texture. By default, we only allocate memory for the base mipmap level and do not enable mipmaps. But someone could later come and request a filter quality which requires mipmaps. In that case, we need to allocate the extra mipmap levels and populate them. When we ask the driver to generate mipmaps, it reallocs the texture storage if it is mutable and hasn't yet allocated space for the mipmap levels. I do not know if we can hook GL's memory allocation so that when a call to GenerateMipmaps needs to realloc it calls our allocator. Maybe GL provides a way. But if not, this may mean either 1.) we can only use this when we have immutable storage (IE the full mipmap stack) in advance, or 2.) prior to calling GenerateMipmaps we copy the texture contents over to a driver-allocated texture, or 3.) we cannot allow mipmaps when using a custom allocator. In Skia, we have either no mipmaps or a full mipmap stack. So maybe the alloc could simply add a bool allocFullMipMapStack or some such?
On 2016/01/25 23:17:41, cblume wrote: > On 2016/01/25 20:51:09, bsalomon wrote: > > Adding Chris Blume as this may have interactions with his mip map changes. > > I presume this CL is a stepping stone to enable a custom texture allocator. I > have a question about the custom texture allocator's goals because mipmapping > may make it more difficult. > > Is this for storing the texture data before it becomes a GL texture? > Is this for shared memory where GL will use this already-allocated memory as the > texture? > > If it is the latter, we may need to account for what GL calls mutable and > immutable storage. > Right now, we use TexImage2D deliberately because it is mutable. > I have a patch waiting for my mipmap code to land which enables TexStorage2D, > which is immutable (and thus gives the driver a useful hint when allocating). > > What mutable/immutable storage means for us is we do not know in advance if we > will later generate mipmaps for this given texture. > By default, we only allocate memory for the base mipmap level and do not enable > mipmaps. But someone could later come and request a filter quality which > requires mipmaps. In that case, we need to allocate the extra mipmap levels and > populate them. When we ask the driver to generate mipmaps, it reallocs the > texture storage if it is mutable and hasn't yet allocated space for the mipmap > levels. > > I do not know if we can hook GL's memory allocation so that when a call to > GenerateMipmaps needs to realloc it calls our allocator. Maybe GL provides a > way. But if not, this may mean either 1.) we can only use this when we have > immutable storage (IE the full mipmap stack) in advance, or 2.) prior to calling > GenerateMipmaps we copy the texture contents over to a driver-allocated texture, > or 3.) we cannot allow mipmaps when using a custom allocator. > > In Skia, we have either no mipmaps or a full mipmap stack. So maybe the alloc > could simply add a bool allocFullMipMapStack or some such? IIUC this will be used with the TEXTURE_RECTANGLE target. Rectangle textures don't support MIP maps at all. So in Skia if we see a draw that requires MIP maps referencing the texture then internally we will create a copy of the texture as a TEXTURE_2D and create a MIP chain for that.
> IIUC this will be used with the TEXTURE_RECTANGLE target. Rectangle textures > don't support MIP maps at all. So in Skia if we see a draw that requires MIP > maps referencing the texture then internally we will create a copy of the > texture as a TEXTURE_2D and create a MIP chain for that. Okay. So in the case we want to create mipmaps we would fallback. Works for me.
bsalomon: Please review. https://codereview.chromium.org/1623653002/diff/60001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/1623653002/diff/60001/include/gpu/GrTypes.h#n... include/gpu/GrTypes.h:426: virtual bool allocateTextureStorage(unsigned textureId, unsigned width, unsigned height) = 0; On 2016/01/25 20:51:09, bsalomon wrote: > What do you think about > > virtual bool allocateTextureStorage(GrBackendObject* allocatedTexture, int > width, int height) > > ? > > The allocator then just tells Skia the texture ID and texture target rather than > having Skia create the ID. It could work with alternative backends that don't > use ints a texture IDs (e.g. Vulkan). > > Also I can imagine that the allocator might want more context than the ID when > deallocate is called (e.g. pointer to GpuMemoryBuffer, ...). Wondering if > GrGLTextureInfo should have a void* fAllocatorContext? I went with your suggested API for allocateTextureStorage(). For deallocate, I kept the current API. On a local branch, when I integrated this patch, the allocator kept a hash map of texture ids : custom allocations. this way, the allocator can also ASSERT that before it is destroyed, each of the custom allocations has been destroyed. https://codereview.chromium.org/1623653002/diff/60001/include/gpu/GrTypes.h#n... include/gpu/GrTypes.h:571: TextureStorageAllocator* fTextureStorageAllocator; On 2016/01/25 20:51:09, bsalomon wrote: > We shouldn't need this here. This is used to import an already existing texture > into Skia. Done. https://codereview.chromium.org/1623653002/diff/100001/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/1623653002/diff/100001/include/core/SkSurface... include/core/SkSurface.h:120: const SkSurfaceProps* = NULL, void* customAllocator = NULL); I wasn't sure what type of interface you wanted. I could add a member to SkSurfaceProps, or I could create another Abstract base class in SkTypes, which the GrType allocator inherits from. Or I could just pass around a void*. Your call.
some tiny nits, mostly more questions. https://codereview.chromium.org/1623653002/diff/100001/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/1623653002/diff/100001/include/core/SkSurface... include/core/SkSurface.h:120: const SkSurfaceProps* = NULL, void* customAllocator = NULL); On 2016/01/26 18:36:16, erikchen wrote: > I wasn't sure what type of interface you wanted. I could add a member to > SkSurfaceProps, or I could create another Abstract base class in SkTypes, which > the GrType allocator inherits from. Or I could just pass around a void*. Your > call. Why not just pass GrTextureStorageAllocator* here? https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:417: * to use custom storage allocation in place of TexImage2D. Maybe say "in place of backend API texture creation function" as there will soon be a non-GL backend. https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:419: class TextureStorageAllocator { Should be GrTextureStorageAllocator https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:421: /* nit, we use four space indents. https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:432: virtual bool allocateTextureStorage(GrBackendObject texture, unsigned width, Don't we also need to somehow tell the allocator the pixel format that we want to use? Also, wondering what we should do when the client is creating a compressed texture. Should we ignore the allocator param, respect it, or fail? Or give the allocator the option (ternary return value - success, fail, fallback to normal allocation). https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:437: virtual void deallocateTextureStorage(GrBackendObject texture) = 0; We have a method on GrContext called abandonContext(), it's called when the 3D context behind the GrContext is no longer available (e.g. lost context on chrome). When that happens we don't call glDeleteTextures() (or equivalent for other resource types). What's our contract in that case? Do we call the deallocate function? Should we let it know that the context was lost? https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:440: ~TextureStorageAllocator() {} Who deletes the allocator, Skia? Should this be virtual? https://codereview.chromium.org/1623653002/diff/100001/src/gpu/gl/GrGLGpu.cpp File src/gpu/gl/GrGLGpu.cpp (right): https://codereview.chromium.org/1623653002/diff/100001/src/gpu/gl/GrGLGpu.cpp... src/gpu/gl/GrGLGpu.cpp:1123: if (!createTextureImpl(desc, &idDesc.fInfo, renderTarget, srcData, nit, we prefix method calls with "this->" https://codereview.chromium.org/1623653002/diff/100001/tests/TextureStorageAl... File tests/TextureStorageAllocator.cpp (right): https://codereview.chromium.org/1623653002/diff/100001/tests/TextureStorageAl... tests/TextureStorageAllocator.cpp:40: void deallocateTextureStorage(GrBackendObject) override { why not delete?
bsalomon: PTAL https://codereview.chromium.org/1623653002/diff/100001/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/1623653002/diff/100001/include/core/SkSurface... include/core/SkSurface.h:120: const SkSurfaceProps* = NULL, void* customAllocator = NULL); On 2016/01/26 22:20:54, bsalomon wrote: > On 2016/01/26 18:36:16, erikchen wrote: > > I wasn't sure what type of interface you wanted. I could add a member to > > SkSurfaceProps, or I could create another Abstract base class in SkTypes, > which > > the GrType allocator inherits from. Or I could just pass around a void*. Your > > call. > > Why not just pass GrTextureStorageAllocator* here? Oops, fixed. https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:417: * to use custom storage allocation in place of TexImage2D. On 2016/01/26 22:20:54, bsalomon wrote: > Maybe say "in place of backend API texture creation function" as there will soon > be a non-GL backend. Done. https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:419: class TextureStorageAllocator { On 2016/01/26 22:20:54, bsalomon wrote: > Should be GrTextureStorageAllocator Done. https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:421: /* On 2016/01/26 22:20:54, bsalomon wrote: > nit, we use four space indents. Done. https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:432: virtual bool allocateTextureStorage(GrBackendObject texture, unsigned width, On 2016/01/26 22:20:54, bsalomon wrote: > Don't we also need to somehow tell the allocator the pixel format that we want > to use? > > Also, wondering what we should do when the client is creating a compressed > texture. Should we ignore the allocator param, respect it, or fail? Or give the > allocator the option (ternary return value - success, fail, fallback to normal > allocation). CHROMIUM_image on Mac supports very few pixel formats, and does not support compressed textures. I added a new method supportsPixelConfig(GrPixelConfig config) that Skia can call to determine whether a given pixel format is supported. https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:437: virtual void deallocateTextureStorage(GrBackendObject texture) = 0; On 2016/01/26 22:20:54, bsalomon wrote: > We have a method on GrContext called abandonContext(), it's called when the 3D > context behind the GrContext is no longer available (e.g. lost context on > chrome). When that happens we don't call glDeleteTextures() (or equivalent for > other resource types). > > What's our contract in that case? Do we call the deallocate function? Should we > let it know that the context was lost? The consumer of Skia will be responsible for destroying all extant textures. I've updated the documentation. https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:440: ~TextureStorageAllocator() {} On 2016/01/26 22:20:54, bsalomon wrote: > Who deletes the allocator, Skia? Should this be virtual? Skia does not delete the allocator, but this should still be virtual. https://codereview.chromium.org/1623653002/diff/100001/src/gpu/gl/GrGLGpu.cpp File src/gpu/gl/GrGLGpu.cpp (right): https://codereview.chromium.org/1623653002/diff/100001/src/gpu/gl/GrGLGpu.cpp... src/gpu/gl/GrGLGpu.cpp:1123: if (!createTextureImpl(desc, &idDesc.fInfo, renderTarget, srcData, On 2016/01/26 22:20:54, bsalomon wrote: > nit, we prefix method calls with "this->" Done. https://codereview.chromium.org/1623653002/diff/100001/tests/TextureStorageAl... File tests/TextureStorageAllocator.cpp (right): https://codereview.chromium.org/1623653002/diff/100001/tests/TextureStorageAl... tests/TextureStorageAllocator.cpp:40: void deallocateTextureStorage(GrBackendObject) override { On 2016/01/26 22:20:55, bsalomon wrote: > why not delete? Added implementation.
https://codereview.chromium.org/1623653002/diff/140001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/1623653002/diff/140001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:433: * The internal format must be GL_RGBA. I'm not crazy about this restriction. I get that that's all that will be supported for the initial subclass in Chromium, but I'd like to use this for some testing on the Skia side and would like to allow support for all formats. I think that would mean that the Allocator can fail and allow the default implementation to take over. Also, I think for certain potential use cases the allocator should at least have the option of performing the upload, in case it can do something more efficient than glTexSubImage2D. https://codereview.chromium.org/1623653002/diff/140001/src/gpu/SkGpuDevice.h File src/gpu/SkGpuDevice.h (right): https://codereview.chromium.org/1623653002/diff/140001/src/gpu/SkGpuDevice.h#... src/gpu/SkGpuDevice.h:57: InitContents, GrTextureStorageAllocator* = NULL); nullptr
On 2016/01/27 21:55:15, erikchen wrote: > bsalomon: PTAL > > https://codereview.chromium.org/1623653002/diff/100001/include/core/SkSurface.h > File include/core/SkSurface.h (right): > > https://codereview.chromium.org/1623653002/diff/100001/include/core/SkSurface... > include/core/SkSurface.h:120: const SkSurfaceProps* = NULL, void* > customAllocator = NULL); > On 2016/01/26 22:20:54, bsalomon wrote: > > On 2016/01/26 18:36:16, erikchen wrote: > > > I wasn't sure what type of interface you wanted. I could add a member to > > > SkSurfaceProps, or I could create another Abstract base class in SkTypes, > > which > > > the GrType allocator inherits from. Or I could just pass around a void*. > Your > > > call. > > > > Why not just pass GrTextureStorageAllocator* here? > > Oops, fixed. > > https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h > File include/gpu/GrTypes.h (right): > > https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... > include/gpu/GrTypes.h:417: * to use custom storage allocation in place of > TexImage2D. > On 2016/01/26 22:20:54, bsalomon wrote: > > Maybe say "in place of backend API texture creation function" as there will > soon > > be a non-GL backend. > > Done. > > https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... > include/gpu/GrTypes.h:419: class TextureStorageAllocator { > On 2016/01/26 22:20:54, bsalomon wrote: > > Should be GrTextureStorageAllocator > > Done. > > https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... > include/gpu/GrTypes.h:421: /* > On 2016/01/26 22:20:54, bsalomon wrote: > > nit, we use four space indents. > > Done. > > https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... > include/gpu/GrTypes.h:432: virtual bool allocateTextureStorage(GrBackendObject > texture, unsigned width, > On 2016/01/26 22:20:54, bsalomon wrote: > > Don't we also need to somehow tell the allocator the pixel format that we want > > to use? > > > > Also, wondering what we should do when the client is creating a compressed > > texture. Should we ignore the allocator param, respect it, or fail? Or give > the > > allocator the option (ternary return value - success, fail, fallback to normal > > allocation). > > CHROMIUM_image on Mac supports very few pixel formats, and does not support > compressed textures. I added a new method supportsPixelConfig(GrPixelConfig > config) that Skia can call to determine whether a given pixel format is > supported. > > https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... > include/gpu/GrTypes.h:437: virtual void deallocateTextureStorage(GrBackendObject > texture) = 0; > On 2016/01/26 22:20:54, bsalomon wrote: > > We have a method on GrContext called abandonContext(), it's called when the 3D > > context behind the GrContext is no longer available (e.g. lost context on > > chrome). When that happens we don't call glDeleteTextures() (or equivalent for > > other resource types). > > > > What's our contract in that case? Do we call the deallocate function? Should > we > > let it know that the context was lost? > > The consumer of Skia will be responsible for destroying all extant textures. > I've updated the documentation. > > https://codereview.chromium.org/1623653002/diff/100001/include/gpu/GrTypes.h#... > include/gpu/GrTypes.h:440: ~TextureStorageAllocator() {} > On 2016/01/26 22:20:54, bsalomon wrote: > > Who deletes the allocator, Skia? Should this be virtual? > > Skia does not delete the allocator, but this should still be virtual. Are we adding a object lifetime issue where there needn't be one? Maybe the allocator should be a simple struct pair of function pointers so that there is no issue of who deletes the allocator. > > https://codereview.chromium.org/1623653002/diff/100001/src/gpu/gl/GrGLGpu.cpp > File src/gpu/gl/GrGLGpu.cpp (right): > > https://codereview.chromium.org/1623653002/diff/100001/src/gpu/gl/GrGLGpu.cpp... > src/gpu/gl/GrGLGpu.cpp:1123: if (!createTextureImpl(desc, &idDesc.fInfo, > renderTarget, srcData, > On 2016/01/26 22:20:54, bsalomon wrote: > > nit, we prefix method calls with "this->" > > Done. > > https://codereview.chromium.org/1623653002/diff/100001/tests/TextureStorageAl... > File tests/TextureStorageAllocator.cpp (right): > > https://codereview.chromium.org/1623653002/diff/100001/tests/TextureStorageAl... > tests/TextureStorageAllocator.cpp:40: void > deallocateTextureStorage(GrBackendObject) override { > On 2016/01/26 22:20:55, bsalomon wrote: > > why not delete? > > Added implementation.
bsalomon: PTAL I changed the abstract base class to be a struct container of function pointers, as you requested. Note that I had to add void* ctx parameter to the function calls. https://codereview.chromium.org/1623653002/diff/140001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/1623653002/diff/140001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:433: * The internal format must be GL_RGBA. On 2016/01/29 16:57:38, bsalomon wrote: > I'm not crazy about this restriction. I get that that's all that will be > supported for the initial subclass in Chromium, but I'd like to use this for > some testing on the Skia side and would like to allow support for all formats. I > think that would mean that the Allocator can fail and allow the default > implementation to take over. > > Also, I think for certain potential use cases the allocator should at least have > the option of performing the upload, in case it can do something more efficient > than glTexSubImage2D. I added two more parameters: GrPixelConfig and a const void* srcData. https://codereview.chromium.org/1623653002/diff/140001/src/gpu/SkGpuDevice.h File src/gpu/SkGpuDevice.h (right): https://codereview.chromium.org/1623653002/diff/140001/src/gpu/SkGpuDevice.h#... src/gpu/SkGpuDevice.h:57: InitContents, GrTextureStorageAllocator* = NULL); On 2016/01/29 16:57:38, bsalomon wrote: > nullptr Done.
https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:441: * The MIN and MAX filters for the created texture must be GL_LINEAR. The In OpenGL, ... https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:444: * If |srcData| is not nullptr, then the implementation of this function I think we need an origin param as well. Also, maybe make the docs say that the impl *may* attempt to upload. I think it's only worth implementing that if there is a faster upload path. https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:514: * A custom platform-specific allocator to use in place of TexImage2D. All ...in place of the backend APIs usual texture creation method (e.g. TexImage2D in OpenGL). https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:515: * surfaces derived from the original surface will have the same value for What exactly is meant here by surfaces derived from the original surface? https://codereview.chromium.org/1623653002/diff/180001/src/gpu/gl/GrGLTexture... File src/gpu/gl/GrGLTexture.cpp (right): https://codereview.chromium.org/1623653002/diff/180001/src/gpu/gl/GrGLTexture... src/gpu/gl/GrGLTexture.cpp:45: GL_CALL(DeleteTextures(1, &fInfo.fID)); this->desc(). Shouldn't we be skipping DeleteTextures() here if the deallocator is used?
bsalomon: PTAL https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:441: * The MIN and MAX filters for the created texture must be GL_LINEAR. The On 2016/02/01 14:54:42, bsalomon wrote: > In OpenGL, ... Done. https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:444: * If |srcData| is not nullptr, then the implementation of this function On 2016/02/01 14:54:42, bsalomon wrote: > I think we need an origin param as well. Also, maybe make the docs say that the > impl *may* attempt to upload. I think it's only worth implementing that if there > is a faster upload path. Done and done. https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:514: * A custom platform-specific allocator to use in place of TexImage2D. All On 2016/02/01 14:54:42, bsalomon wrote: > ...in place of the backend APIs usual texture creation method (e.g. TexImage2D > in OpenGL). Done. https://codereview.chromium.org/1623653002/diff/180001/include/gpu/GrTypes.h#... include/gpu/GrTypes.h:515: * surfaces derived from the original surface will have the same value for On 2016/02/01 14:54:42, bsalomon wrote: > What exactly is meant here by surfaces derived from the original surface? I was referring to RenderTargets created by copy-on-write. I guess that's probably implied, and I just removed that entire portion of the comment, because apparently it was too confusing. https://codereview.chromium.org/1623653002/diff/180001/src/gpu/gl/GrGLTexture... File src/gpu/gl/GrGLTexture.cpp (right): https://codereview.chromium.org/1623653002/diff/180001/src/gpu/gl/GrGLTexture... src/gpu/gl/GrGLTexture.cpp:45: GL_CALL(DeleteTextures(1, &fInfo.fID)); On 2016/02/01 14:54:42, bsalomon wrote: > this->desc(). > > Shouldn't we be skipping DeleteTextures() here if the deallocator is used? Responded to both comments.
I thought of another wrinkle here: interaction with the resource cache. 1) Should textures created by the allocator be recyclable by the cache. My instinct is "no", that these are special textures managed by the client and Skia shouldn't presume to be able to reuse them for different purposes. 2) We should make sure that if the caller specifies an allocator that we don't instead pull an existing texture from the cache (in GrTextureProvider).
On 2016/02/02 18:08:00, bsalomon wrote: > I thought of another wrinkle here: interaction with the resource cache. > > 1) Should textures created by the allocator be recyclable by the cache. My > instinct is "no", that these are special textures managed by the client and Skia > shouldn't presume to be able to reuse them for different purposes. > > 2) We should make sure that if the caller specifies an allocator that we don't > instead pull an existing texture from the cache (in GrTextureProvider). bsalomon: PTAL 1. I added a new virtual method |reuseable| to GrGpuResource which prevents custom-allocator textures from being reused by the resource cache. (More specifically, prevents an unbudgeted resource from being becoming a budgeted resource on destruction. 2. I also updated the logic to createTexture to prevent custom-allocator textures from trying to reuse textures from the resource cache. 3. Finally, I added the condition that custom-allocator textures must be unbudgeted. I also added tests.
https://codereview.chromium.org/1623653002/diff/220001/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/1623653002/diff/220001/include/core/SkSurface... include/core/SkSurface.h:115: * render target, allocated by the surface. Put the comment here that the allocator will be reused if additional texture allocations are made do to SkImage snapshots? I think it will be clearer here then on GrSurfaceDesc. https://codereview.chromium.org/1623653002/diff/220001/include/gpu/GrSurface.h File include/gpu/GrSurface.h (right): https://codereview.chromium.org/1623653002/diff/220001/include/gpu/GrSurface.... include/gpu/GrSurface.h:165: bool reuseable() const override; I think this can be done without a new function. If we just don't add a scratch key when the resource is created it won't be recycled for another purpose. https://codereview.chromium.org/1623653002/diff/220001/src/gpu/GrTextureProvi... File src/gpu/GrTextureProvider.cpp (right): https://codereview.chromium.org/1623653002/diff/220001/src/gpu/GrTextureProvi... src/gpu/GrTextureProvider.cpp:44: !desc.fTextureStorageAllocator.fAllocateTextureStorage) { nit, style aligns this ! with the one above (not crazy about it but it is what do :) ) https://codereview.chromium.org/1623653002/diff/220001/src/image/SkSurface_Gp... File src/image/SkSurface_Gpu.cpp (right): https://codereview.chromium.org/1623653002/diff/220001/src/image/SkSurface_Gp... src/image/SkSurface_Gpu.cpp:127: if (customAllocator.fAllocateTextureStorage && (budgeted == kYes_Budgeted)) This doesn't necessarily have to be true... kYes_Budgeted just means that the texture is counts against the budget until it is deleted. If we don't add a scratch key it won't get recycled so the space is reclaimed as soon as the SkSurface is deleted.
bsalomon: PTAL https://codereview.chromium.org/1623653002/diff/220001/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/1623653002/diff/220001/include/core/SkSurface... include/core/SkSurface.h:115: * render target, allocated by the surface. On 2016/02/04 16:27:08, bsalomon wrote: > Put the comment here that the allocator will be reused if additional texture > allocations are made do to SkImage snapshots? I think it will be clearer here > then on GrSurfaceDesc. Done. https://codereview.chromium.org/1623653002/diff/220001/include/gpu/GrSurface.h File include/gpu/GrSurface.h (right): https://codereview.chromium.org/1623653002/diff/220001/include/gpu/GrSurface.... include/gpu/GrSurface.h:165: bool reuseable() const override; On 2016/02/04 16:27:08, bsalomon wrote: > I think this can be done without a new function. If we just don't add a scratch > key when the resource is created it won't be recycled for another purpose. You're right. Done. https://codereview.chromium.org/1623653002/diff/220001/src/gpu/GrTextureProvi... File src/gpu/GrTextureProvider.cpp (right): https://codereview.chromium.org/1623653002/diff/220001/src/gpu/GrTextureProvi... src/gpu/GrTextureProvider.cpp:44: !desc.fTextureStorageAllocator.fAllocateTextureStorage) { On 2016/02/04 16:27:08, bsalomon wrote: > nit, style aligns this ! with the one above (not crazy about it but it is what > do :) ) Done. https://codereview.chromium.org/1623653002/diff/220001/src/image/SkSurface_Gp... File src/image/SkSurface_Gpu.cpp (right): https://codereview.chromium.org/1623653002/diff/220001/src/image/SkSurface_Gp... src/image/SkSurface_Gpu.cpp:127: if (customAllocator.fAllocateTextureStorage && (budgeted == kYes_Budgeted)) On 2016/02/04 16:27:08, bsalomon wrote: > This doesn't necessarily have to be true... kYes_Budgeted just means that the > texture is counts against the budget until it is deleted. > > If we don't add a scratch key it won't get recycled so the space is reclaimed as > soon as the SkSurface is deleted. Right - I've removed this logic.
lgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1623653002/#ps260001 (title: "Fix compile error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/260001
Message was sent while issue was closed.
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55
Message was sent while issue was closed.
reed@google.com changed reviewers: + reed@google.com
Message was sent while issue was closed.
Could this CL be responsible for the DEPS roll failures? @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@[5247:5247:0204/191232:ERROR:CONSOLE(72)] "Uncaught TypeError: Cannot read property 'addExtensions' of undefined", source: (72)@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@Received signal 11 SEGV_MAPERR 000000000050@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#0 0x000005e4026b base::debug::(anonymous namespace)::StackDumpSignalHandler()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#1 0x7f9f05fe4cb0 <unknown>@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#2 0x000001d8452c GrGLGpu::uploadTexData()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#3 0x000001d82e84 GrGLGpu::onWritePixels()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#4 0x000001ccc9f6 GrBatchAtlas::BatchPlot::uploadToTexture()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#5 0x000001ce1911 GrBatchFlushState::preIssueDraws()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#6 0x000001ce1530 GrDrawingManager::flush()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#7 0x000001cd920d GrContext::flush()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#8 0x000001cdb0ae GrContext::prepareSurfaceForExternalIO()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#9 0x000001dc93ee SkGpuDevice::flush()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#10 0x0000038e0e74 blink::Canvas2DLayerBridge::flush()@@@ @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#11 0x0000038e1b9f blink::Canvas2DLayerBridge::newImageSnapshot()@@@
Message was sent while issue was closed.
On 2016/02/05 at 03:34:18, reed wrote: > Could this CL be responsible for the DEPS roll failures? > > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@[5247:5247:0204/191232:ERROR:CONSOLE(72)] "Uncaught TypeError: Cannot read property 'addExtensions' of undefined", source: (72)@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@Received signal 11 SEGV_MAPERR 000000000050@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#0 0x000005e4026b base::debug::(anonymous namespace)::StackDumpSignalHandler()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#1 0x7f9f05fe4cb0 <unknown>@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#2 0x000001d8452c GrGLGpu::uploadTexData()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#3 0x000001d82e84 GrGLGpu::onWritePixels()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#4 0x000001ccc9f6 GrBatchAtlas::BatchPlot::uploadToTexture()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#5 0x000001ce1911 GrBatchFlushState::preIssueDraws()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#6 0x000001ce1530 GrDrawingManager::flush()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#7 0x000001cd920d GrContext::flush()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#8 0x000001cdb0ae GrContext::prepareSurfaceForExternalIO()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#9 0x000001dc93ee SkGpuDevice::flush()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#10 0x0000038e0e74 blink::Canvas2DLayerBridge::flush()@@@ > @@@STEP_LOG_LINE@DevToolsPixelOutputTests.TestScreenshotRecording@#11 0x0000038e1b9f blink::Canvas2DLayerBridge::newImageSnapshot()@@@ Looks like it is, the roll started to fail with just two commits: https://codereview.chromium.org/1668293002 And fmalita's CL only added a function. I'll revert.
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1669213002/ by jcgregorio@google.com. The reason for reverting is: Seems to causing the DEPS roll to fail: https://codereview.chromium.org/1668293002/.
Message was sent while issue was closed.
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 ========== to ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 ==========
Patchset #15 (id:280001) has been deleted
On 2016/02/05 03:51:11, jcgregorio wrote: > A revert of this CL (patchset #14 id:260001) has been created in > https://codereview.chromium.org/1669213002/ by mailto:jcgregorio@google.com. > > The reason for reverting is: Seems to causing the DEPS roll to fail: > > https://codereview.chromium.org/1668293002/. bsalomon: PTAL (Patch set 14 was landed/reverted, so feel free to diff against that.) For details on the failure, see https://code.google.com/p/chromium/issues/detail?id=579664#c9. In one of my earlier revisions, I changed the signature of GrGLGpu::uploadTexData(). Due to our changes to the interface of the texture allocator, this change is no longer necessary, which is good, because the change was likely incorrect (probably for some custom wrapped textures).
On 2016/02/05 18:34:31, erikchen wrote: > On 2016/02/05 03:51:11, jcgregorio wrote: > > A revert of this CL (patchset #14 id:260001) has been created in > > https://codereview.chromium.org/1669213002/ by mailto:jcgregorio@google.com. > > > > The reason for reverting is: Seems to causing the DEPS roll to fail: > > > > https://codereview.chromium.org/1668293002/. > > bsalomon: PTAL > > (Patch set 14 was landed/reverted, so feel free to diff against that.) > > For details on the failure, see > https://code.google.com/p/chromium/issues/detail?id=579664#c9. In one of my > earlier revisions, I changed the signature of GrGLGpu::uploadTexData(). Due to > our changes to the interface of the texture allocator, this change is no longer > necessary, which is good, because the change was likely incorrect (probably for > some custom wrapped textures). lgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1623653002/#ps340001 (title: "Fix MSVS compile issue, as it doesn't support dynamic-size C arrays.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/340001
Message was sent while issue was closed.
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 ========== to ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b ==========
Message was sent while issue was closed.
Committed patchset #17 (id:340001) as https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b
Message was sent while issue was closed.
Is this CL related to this failure? c:\0\build\slave\workdir\build\skia\tests\texturestorageallocator.cpp:83 GrColorUnpackG(dest) == 255 https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-Sh...
Message was sent while issue was closed.
On 2016/02/06 01:52:24, reed1 wrote: > Is this CL related to this failure? > > c:\0\build\slave\workdir\build\skia\tests\texturestorageallocator.cpp:83 GrColorUnpackG(dest) > == 255 > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-Sh... yes, since that's a newly added file.
Message was sent while issue was closed.
On 2016/02/06 01:52:24, reed1 wrote: > Is this CL related to this failure? > > c:\0\build\slave\workdir\build\skia\tests\texturestorageallocator.cpp:83 GrColorUnpackG(dest) > == 255 > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-Sh... It appears to be
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:340001) has been created in https://codereview.chromium.org/1673923003/ by bsalomon@google.com. The reason for reverting is: New unit test is failing on Windows ANGLE bots: http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-... .
Message was sent while issue was closed.
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b ========== to ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b ==========
On 2016/02/06 01:56:28, bsalomon wrote: > A revert of this CL (patchset #17 id:340001) has been created in > https://codereview.chromium.org/1673923003/ by mailto:bsalomon@google.com. > > The reason for reverting is: New unit test is failing on Windows ANGLE bots: > http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-... > . bsalomon: I'm relanding this CL. Note that patch set 18 fails ANGLE bots but patch set 19 passes, so the ANGLE problem is fixed. The test was missing some calls to glTexParameteri().
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1623653002/#ps380001 (title: "Possibly fix ANGLE test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/380001
On 2016/02/09 21:01:34, erikchen wrote: > On 2016/02/06 01:56:28, bsalomon wrote: > > A revert of this CL (patchset #17 id:340001) has been created in > > https://codereview.chromium.org/1673923003/ by mailto:bsalomon@google.com. > > > > The reason for reverting is: New unit test is failing on Windows ANGLE bots: > > > http://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-GPU-... > > . > > bsalomon: I'm relanding this CL. Note that patch set 18 fails ANGLE bots but > patch set 19 passes, so the ANGLE problem is fixed. The test was missing some > calls to glTexParameteri(). sgtm
Message was sent while issue was closed.
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b ========== to ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b Committed: https://skia.googlesource.com/skia/+/b8d6e088590160f1198110c2371b802c1d541a36 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:380001) as https://skia.googlesource.com/skia/+/b8d6e088590160f1198110c2371b802c1d541a36
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:380001) has been created in https://codereview.chromium.org/1684993002/ by caryclark@google.com. The reason for reverting is: Breaks ASAN bot: Direct leak of 56 byte(s) in 1 object(s) allocated from: ... test_CustomTexture https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G....
Message was sent while issue was closed.
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b Committed: https://skia.googlesource.com/skia/+/b8d6e088590160f1198110c2371b802c1d541a36 ========== to ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b Committed: https://skia.googlesource.com/skia/+/b8d6e088590160f1198110c2371b802c1d541a36 ==========
On 2016/02/10 00:28:29, caryclark wrote: > A revert of this CL (patchset #19 id:380001) has been created in > https://codereview.chromium.org/1684993002/ by mailto:caryclark@google.com. > > The reason for reverting is: Breaks ASAN bot: > > Direct leak of 56 byte(s) in 1 object(s) allocated from: > ... > test_CustomTexture > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G.... Passing ASAN. Trying to land again...
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1623653002/#ps430001 (title: "Add missing ')'.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623653002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623653002/430001
Message was sent while issue was closed.
Description was changed from ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b Committed: https://skia.googlesource.com/skia/+/b8d6e088590160f1198110c2371b802c1d541a36 ========== to ========== skia: Add support for CHROMIUM_image backed textures. I created a new abstract base class TextureStorageAllocator that consumers of Skia can subclass and pass back to Skia. When a surface is created with a pointer to a TextureStorageAllocator, any textures it creates, or that are derived from the original surface, will allocate and deallocate storage using the methods on TextureStorageAllocator. BUG=https://code.google.com/p/chromium/issues/detail?id=579664 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/92098e691f10a010e7421125ba4d44c02506bb55 Committed: https://skia.googlesource.com/skia/+/7fec91ce6660190f8d7c5eb6f3061e4550cc672b Committed: https://skia.googlesource.com/skia/+/b8d6e088590160f1198110c2371b802c1d541a36 Committed: https://skia.googlesource.com/skia/+/9a1ed5d81dbfc7d5b67b568dfe12b4033a9af154 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:430001) as https://skia.googlesource.com/skia/+/9a1ed5d81dbfc7d5b67b568dfe12b4033a9af154 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
