Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1776693002:
Add deferred texture upload API. (Closed)
DescriptionAdd deferred texture upload API.
Performs thread-safe decoding of SkImage in order to later create a texture-backed SkImage.
The client allocates storage for the data.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1776693002
Committed: https://skia.googlesource.com/skia/+/41b952c296e343eeabb07d52b6a55ba7565a286b
Patch Set 1 #
Total comments: 28
Patch Set 2 : Address comments #Patch Set 3 : cleanup #Patch Set 4 : Simplify interface #Patch Set 5 : cleanup and no gpu building again #Patch Set 6 : rebase on pixmap change #
Total comments: 8
Patch Set 7 : simplify usage params, remove img id, fix comments #
Total comments: 2
Patch Set 8 : rebase on sk_skp #
Messages
Total messages: 42 (8 generated)
Description was changed from ========== Add deferred texture upload API. Performs thread-safe decoding of SkImage in order to later create a texture-backed SkImage. The client allocates storage for the data. BUG=skia: ========== to ========== Add deferred texture upload API. Performs thread-safe decoding of SkImage in order to later create a texture-backed SkImage. The client allocates storage for the data. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
bsalomon@google.com changed reviewers: + reed@google.com, robertphillips@google.com
The intended use case is threaded decode for GPU rasterization in Chromium. This is mostly a stub implementation to land the API. It doesn't currently preserve YUV-planes or compressed texture upload paths.
1. Perhaps we just return a bool, instead of another faux object? 2. Bikeshed: do we need to add "clientStorage" to the end of the maker
3. something about alignment requirements on the provided storage
I like the pixmap changes, and the NewTextureFromPixmap as is. Feel free to land those ahead of time if you wish (I think they have value on their own).
https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h#newc... include/core/SkImage.h:323: * performed. Constructing one of these does not make calls to the underlying 3D API the of - the ? https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h#newc... include/core/SkImage.h:337: Are their any constraints on the proxy we hand to getDeferredTextureImageSize and createDeferredTextureImageInClientStorage (i.e., that have to be the same)? https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h#newc... include/core/SkImage.h:339: * Computes the size that the client must allocate in order to create a DeferredTextureImage tee -> te ? https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h#newc... include/core/SkImage.h:342: size_t getDeferredTextureImageSize(const GrContextThreadSafeProxy&, Why an array of usage parameters? https://codereview.chromium.org/1776693002/diff/1/include/core/SkPixmap.h File include/core/SkPixmap.h (right): https://codereview.chromium.org/1776693002/diff/1/include/core/SkPixmap.h#new... include/core/SkPixmap.h:213: /** rwo -> row ? https://codereview.chromium.org/1776693002/diff/1/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.chromium.org/1776693002/diff/1/include/gpu/GrContext.h#new... include/gpu/GrContext.h:459: // This class just gives access to the Caps & unique ID of a GrContext in a thread-safe manner ? https://codereview.chromium.org/1776693002/diff/1/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): https://codereview.chromium.org/1776693002/diff/1/src/gpu/GrContext.cpp#newco... src/gpu/GrContext.cpp:129: Should we make threadSafeProxy thread safe itself? https://codereview.chromium.org/1776693002/diff/1/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/1776693002/diff/1/src/gpu/SkGr.cpp#newcode253 src/gpu/SkGr.cpp:253: // "rowBytes", since they are the same now. rm the extra ' ' after kYes ? https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:345: static bool GetSizeOrInit(const SkImage* image, const GrContextThreadSafeProxy& proxy, line these up ? https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:391: if (!data) { too far over ? https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:405: size_t size = 0; is this supposed to be dtiSize ? https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:418: intptr_t allocation = reinterpret_cast<intptr_t>(inBuffer); itd -> dti ? https://codereview.chromium.org/1776693002/diff/1/tests/ImageTest.cpp File tests/ImageTest.cpp (right): https://codereview.chromium.org/1776693002/diff/1/tests/ImageTest.cpp#newcode788 tests/ImageTest.cpp:788: REPORTER_ASSERT(reporter, proxy); stray ';' ? https://codereview.chromium.org/1776693002/diff/1/tests/ImageTest.cpp#newcode825 tests/ImageTest.cpp:825: void* storage = sk_malloc_throw(size); td -> dti ?
bsalomon@google.com changed reviewers: + ericrk@chromium.org
Eric please take a look and see if this will work for you. https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h#newc... include/core/SkImage.h:323: * performed. Constructing one of these does not make calls to the underlying 3D API On 2016/03/08 18:43:10, robertphillips wrote: > the of - the ? Done. https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h#newc... include/core/SkImage.h:337: On 2016/03/08 18:43:10, robertphillips wrote: > Are their any constraints on the proxy we hand to getDeferredTextureImageSize > and createDeferredTextureImageInClientStorage (i.e., that have to be the same)? Added comment that the size of the buffer must be at least as big as that the getSize function with the same proxy. https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h#newc... include/core/SkImage.h:339: * Computes the size that the client must allocate in order to create a DeferredTextureImage On 2016/03/08 18:43:10, robertphillips wrote: > tee -> te ? Done. https://codereview.chromium.org/1776693002/diff/1/include/core/SkImage.h#newc... include/core/SkImage.h:342: size_t getDeferredTextureImageSize(const GrContextThreadSafeProxy&, On 2016/03/08 18:43:10, robertphillips wrote: > Why an array of usage parameters? The client may have multiple draws with the same image. https://codereview.chromium.org/1776693002/diff/1/include/core/SkPixmap.h File include/core/SkPixmap.h (right): https://codereview.chromium.org/1776693002/diff/1/include/core/SkPixmap.h#new... include/core/SkPixmap.h:213: /** On 2016/03/08 18:43:10, robertphillips wrote: > rwo -> row ? Done. https://codereview.chromium.org/1776693002/diff/1/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.chromium.org/1776693002/diff/1/include/gpu/GrContext.h#new... include/gpu/GrContext.h:459: On 2016/03/08 18:43:10, robertphillips wrote: > // This class just gives access to the Caps & unique ID of a GrContext in a > thread-safe manner > > ? I'd rather not document the implementation, but added a comment about what it is used for. https://codereview.chromium.org/1776693002/diff/1/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): https://codereview.chromium.org/1776693002/diff/1/src/gpu/GrContext.cpp#newco... src/gpu/GrContext.cpp:129: On 2016/03/08 18:43:10, robertphillips wrote: > Should we make threadSafeProxy thread safe itself? GrContext is not thread-safe so I don't think this method should be expected to be thread-safe and I think it'll get complicated to document which methods are thread safe. https://codereview.chromium.org/1776693002/diff/1/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/1776693002/diff/1/src/gpu/SkGr.cpp#newcode253 src/gpu/SkGr.cpp:253: // "rowBytes", since they are the same now. On 2016/03/08 18:43:10, robertphillips wrote: > rm the extra ' ' after kYes ? Done. https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:345: static bool GetSizeOrInit(const SkImage* image, const GrContextThreadSafeProxy& proxy, On 2016/03/08 18:43:10, robertphillips wrote: > line these up ? Done. https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:391: if (!data) { On 2016/03/08 18:43:10, robertphillips wrote: > too far over ? Done. https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:405: size_t size = 0; On 2016/03/08 18:43:10, robertphillips wrote: > is this supposed to be dtiSize ? Done. https://codereview.chromium.org/1776693002/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:418: intptr_t allocation = reinterpret_cast<intptr_t>(inBuffer); On 2016/03/08 18:43:10, robertphillips wrote: > itd -> dti ? Done. https://codereview.chromium.org/1776693002/diff/1/tests/ImageTest.cpp File tests/ImageTest.cpp (right): https://codereview.chromium.org/1776693002/diff/1/tests/ImageTest.cpp#newcode788 tests/ImageTest.cpp:788: REPORTER_ASSERT(reporter, proxy); On 2016/03/08 18:43:10, robertphillips wrote: > stray ';' ? Done. https://codereview.chromium.org/1776693002/diff/1/tests/ImageTest.cpp#newcode825 tests/ImageTest.cpp:825: void* storage = sk_malloc_throw(size); On 2016/03/08 18:43:10, robertphillips wrote: > td -> dti ? other changes obsoleted this (the type is gone).
On 2016/03/08 16:03:48, reed1 wrote: > 3. something about alignment requirements on the provided storage Documented 8 byte alignment
On 2016/03/08 16:03:28, reed1 wrote: > 1. Perhaps we just return a bool, instead of another faux object? > Done > 2. Bikeshed: do we need to add "clientStorage" to the end of the maker Done
This API will definitely work for my use cases. A few thoughts: Passing a void* is fine on my end, and I agree that if we're going to placement new the DeferredTextureImage into the void*, it's better not to pass out some sort of SkDeferredTextureImage*, as it's a bit unclear whether we need to delete both that and the void* we passed in. That said, it's completely fine on my end if you also pass out an SkDeferredTextureImage* that we need to delete in addition to the void*, if this is easier for you - completely up to you.
On 2016/03/08 21:27:43, ericrk wrote: > This API will definitely work for my use cases. > > A few thoughts: > Passing a void* is fine on my end, and I agree that if we're going to placement > new the DeferredTextureImage into the void*, it's better not to pass out some > sort of SkDeferredTextureImage*, as it's a bit unclear whether we need to delete > both that and the void* we passed in. > > That said, it's completely fine on my end if you also pass out an > SkDeferredTextureImage* that we need to delete in addition to the void*, if this > is easier for you - completely up to you. The return pointer in the original patchset was vestigial from an earlier version that took an allocator. I have a small preference for one allocation for both the pixel data and the metadata (DeferredTextureImageData struct).
Updated to simplify the API (pass null to get size, non-null to get data)
api lgtm What (should) happens if the src image is already GPU backed? It looks like we return 0 (failure), which is fine, but its a little funny (to me). Do we document this?
Similar questions about pictured-backed src images (which certainly *can* give you raster if you want it...)
On 2016/03/09 15:20:17, reed1 wrote: > Similar questions about pictured-backed src images (which certainly *can* give > you raster if you want it...) Both texture and picture images return 0 currently and the unit test fails in that case. I document that "inappropriate images" will fail, leaving it deliberately vague in case we change our mind.
On 2016/03/09 15:38:04, bsalomon wrote: > On 2016/03/09 15:20:17, reed1 wrote: > > Similar questions about pictured-backed src images (which certainly *can* give > > you raster if you want it...) > > Both texture and picture images return 0 currently and the unit test fails in > that case. I document that "inappropriate images" will fail, leaving it > deliberately vague in case we change our mind. I mean the unit test checks for the failure (0 return) in those cases.
Rebased after pulling NewTextureFromPixmap into its own CL.
https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h... include/core/SkImage.h:322: SkMatrix fViewMatrix; ViewMatrix is not really a term we use in the skia api per-se. Not sure I have a better name tho... CTM? just a thought
https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h... include/core/SkImage.h:322: SkMatrix fViewMatrix; On 2016/03/09 18:28:41, reed1 wrote: > ViewMatrix is not really a term we use in the skia api per-se. Not sure I have a > better name tho... CTM? just a thought Sure, "CTM" or "CanvasMatrix" would work.
https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h... include/core/SkImage.h:335: * to for -> for ? https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h... include/core/SkImage.h:353: /** produce -> produced ? https://codereview.chromium.org/1776693002/diff/100001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/1776693002/diff/100001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:333: uint32_t fContextUniqueID; What do we do with fImageUniqueID ?
Quick question - in our bookkeeping in CC we currently combine the src>dst transform and view matrix into just a src rect and a combined matrix. I see that this API takes src, dst, and view matrix. We could easily preserve the all these in CC, but before I make the change, I wanted to check and see if skia benefited from having src/dst/view_matrix, as opposed to just src/combined_matrix (in which case we just re-pass src as dst).
On 2016/03/09 19:03:46, ericrk wrote: > Quick question - in our bookkeeping in CC we currently combine the src>dst > transform and view matrix into just a src rect and a combined matrix. I see that > this API takes src, dst, and view matrix. We could easily preserve the all these > in CC, but before I make the change, I wanted to check and see if skia benefited > from having src/dst/view_matrix, as opposed to just src/combined_matrix (in > which case we just re-pass src as dst). Ah, I thought I was doing src/dst/vm on behalf of cc. If you've already combined the matrices, that's ok with me.
Removing fDst is slightly preferable too -- less to document, nothing is overspecified.
On 2016/03/09 19:06:52, bsalomon wrote: > On 2016/03/09 19:03:46, ericrk wrote: > > Quick question - in our bookkeeping in CC we currently combine the src>dst > > transform and view matrix into just a src rect and a combined matrix. I see > that > > this API takes src, dst, and view matrix. We could easily preserve the all > these > > in CC, but before I make the change, I wanted to check and see if skia > benefited > > from having src/dst/view_matrix, as opposed to just src/combined_matrix (in > > which case we just re-pass src as dst). > > Ah, I thought I was doing src/dst/vm on behalf of cc. If you've already combined > the matrices, that's ok with me. Actually, if you have the combined matrix I don't really have a use for the src rect either.
On 2016/03/09 19:10:32, bsalomon wrote: > On 2016/03/09 19:06:52, bsalomon wrote: > > On 2016/03/09 19:03:46, ericrk wrote: > > > Quick question - in our bookkeeping in CC we currently combine the src>dst > > > transform and view matrix into just a src rect and a combined matrix. I see > > that > > > this API takes src, dst, and view matrix. We could easily preserve the all > > these > > > in CC, but before I make the change, I wanted to check and see if skia > > benefited > > > from having src/dst/view_matrix, as opposed to just src/combined_matrix (in > > > which case we just re-pass src as dst). > > > > Ah, I thought I was doing src/dst/vm on behalf of cc. If you've already > combined > > the matrices, that's ok with me. > > Actually, if you have the combined matrix I don't really have a use for the src > rect either. I guess src only really matters later, during draw, when you might need to do strict clip stuff?
On 2016/03/09 19:11:32, ericrk wrote: > On 2016/03/09 19:10:32, bsalomon wrote: > > On 2016/03/09 19:06:52, bsalomon wrote: > > > On 2016/03/09 19:03:46, ericrk wrote: > > > > Quick question - in our bookkeeping in CC we currently combine the src>dst > > > > transform and view matrix into just a src rect and a combined matrix. I > see > > > that > > > > this API takes src, dst, and view matrix. We could easily preserve the all > > > these > > > > in CC, but before I make the change, I wanted to check and see if skia > > > benefited > > > > from having src/dst/view_matrix, as opposed to just src/combined_matrix > (in > > > > which case we just re-pass src as dst). > > > > > > Ah, I thought I was doing src/dst/vm on behalf of cc. If you've already > > combined > > > the matrices, that's ok with me. > > > > Actually, if you have the combined matrix I don't really have a use for the > src > > rect either. > > I guess src only really matters later, during draw, when you might need to do > strict clip stuff? Right
Latest change simplifies the usage params. https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h... include/core/SkImage.h:335: * On 2016/03/09 18:54:16, robertphillips wrote: > to for -> for ? Done. https://codereview.chromium.org/1776693002/diff/100001/include/core/SkImage.h... include/core/SkImage.h:353: /** On 2016/03/09 18:54:16, robertphillips wrote: > produce -> produced ? Done. https://codereview.chromium.org/1776693002/diff/100001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/1776693002/diff/100001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:333: uint32_t fContextUniqueID; On 2016/03/09 18:54:16, robertphillips wrote: > What do we do with fImageUniqueID ? Currently, nothing. I wasn't sure whether the new image should have the same uniqueID as the original. I'll remove this for now.
Is this good to go?
still lgtm https://codereview.chromium.org/1776693002/diff/120001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/1776693002/diff/120001/include/core/SkImage.h... include/core/SkImage.h:361: * The context must be the context that provided the proxy passed to btw -- can you assert that, by storing the GrCotnext* in your data-blob ad build time?
https://codereview.chromium.org/1776693002/diff/120001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/1776693002/diff/120001/include/core/SkImage.h... include/core/SkImage.h:361: * The context must be the context that provided the proxy passed to On 2016/03/10 21:34:12, reed1 wrote: > btw -- can you assert that, by storing the GrCotnext* in your data-blob ad build > time? I don't assert but I do check it by storing the GrContext's unique ID and fail if it is different.
The CQ bit was checked by bsalomon@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776693002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776693002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
lgtm on my end as well.
The CQ bit was checked by bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com, ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/1776693002/#ps140001 (title: "rebase on sk_skp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776693002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776693002/140001
Message was sent while issue was closed.
Description was changed from ========== Add deferred texture upload API. Performs thread-safe decoding of SkImage in order to later create a texture-backed SkImage. The client allocates storage for the data. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add deferred texture upload API. Performs thread-safe decoding of SkImage in order to later create a texture-backed SkImage. The client allocates storage for the data. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/41b952c296e343eeabb07d52b6a55ba7565a286b ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/41b952c296e343eeabb07d52b6a55ba7565a286b |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
