Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11412043:
cc: Add asynchronous setPixel interface. (Closed)
|
Created:
8 years, 1 month ago by reveman Modified:
8 years ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Add asynchronous setPixel interface.
Implements asynchronous texture uploads using
CHROMIUM_async_pixel_transfers extension.
BUG=155209
TEST=cc_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171657
Patch Set 1 #Patch Set 2 : Add software renderer support and remove unnecessary changes to PrioritizedResource class. #
Total comments: 11
Patch Set 3 : scheduleSetPixels -> beginSetPixels #Patch Set 4 : Lock for write until asynchronous setPixels complete. #
Total comments: 2
Patch Set 5 : #Patch Set 6 : Add abortSetPixels. #
Total comments: 2
Patch Set 7 : rebase #
Total comments: 5
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : Rebase #Patch Set 11 : Rebase and upload the correct patch. This is rebased version of patch set 9 without abortSetPixels. #Patch Set 12 : rebase #Messages
Total messages: 29 (0 generated)
This implements compositor side support for CHROMIUM_async_texture_uploads. Requires GPU process side changes to land first.
looks neat. get one quick review from danakj or apatrick sanity check? https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:858: if (resource->glId) { behavior on context lost? https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h File cc/resource_provider.h (right): https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h#new... cc/resource_provider.h:205: void scheduleSetPixels(ResourceId id); beginSetPixels? https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h#new... cc/resource_provider.h:215: unsigned glPixelBufferId; would comments on these vars help indicate that they're used together?
https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:813: context3d->bindTexture(GL_TEXTURE_2D, resource->glId); Should this be locking for write? https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:858: if (resource->glId) { On 2012/11/17 17:10:30, nduca wrote: > behavior on context lost? +1, also consider the lock, if we write lock during the upload.
https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:813: context3d->bindTexture(GL_TEXTURE_2D, resource->glId); On 2012/11/17 17:47:25, danakj wrote: > Should this be locking for write? Maybe but we currently don't lock for write in setPixels and unless that's incorrect I have a hard time seeing why we should lock here. The difference between setPixels and beginSetPixels is that in the later we don't know when the upload will happen in relation to commands issued through the same command buffer. Is that somehow a reason to lock for write here? https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:858: if (resource->glId) { On 2012/11/17 17:10:30, nduca wrote: > behavior on context lost? This returns true on context lost (same behavior as for the queries used by the TextureUploader). This should make sure the logic in the TileManager that uses this async upload interface behave properly on context lost. https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h File cc/resource_provider.h (right): https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h#new... cc/resource_provider.h:205: void scheduleSetPixels(ResourceId id); On 2012/11/17 17:10:30, nduca wrote: > beginSetPixels? Done. https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h#new... cc/resource_provider.h:215: unsigned glPixelBufferId; On 2012/11/17 17:10:30, nduca wrote: > would comments on these vars help indicate that they're used together? Done.
https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:813: context3d->bindTexture(GL_TEXTURE_2D, resource->glId); On 2012/11/18 18:15:59, David Reveman wrote: > On 2012/11/17 17:47:25, danakj wrote: > > Should this be locking for write? > > Maybe but we currently don't lock for write in setPixels and unless that's > incorrect I have a hard time seeing why we should lock here. > > The difference between setPixels and beginSetPixels is that in the later we > don't know when the upload will happen in relation to commands issued through > the same command buffer. Is that somehow a reason to lock for write here? Well my thought is that setPixels doesn't set the lock since it does the write and is done, but it does check that no one else has a write lock, right? So that setPixels won't clobber another write. Here, we don't know when the write is going to happen, and if someone else does a write, we could clobber it with the async upload. So it feels to me like we should be locking it until the upload is done, so we get predictable behaviour. What do you think?
https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... cc/resource_provider.cc:813: context3d->bindTexture(GL_TEXTURE_2D, resource->glId); On 2012/11/18 18:22:49, danakj wrote: > On 2012/11/18 18:15:59, David Reveman wrote: > > On 2012/11/17 17:47:25, danakj wrote: > > > Should this be locking for write? > > > > Maybe but we currently don't lock for write in setPixels and unless that's > > incorrect I have a hard time seeing why we should lock here. > > > > The difference between setPixels and beginSetPixels is that in the later we > > don't know when the upload will happen in relation to commands issued through > > the same command buffer. Is that somehow a reason to lock for write here? > > Well my thought is that setPixels doesn't set the lock since it does the write > and is done, but it does check that no one else has a write lock, right? So that > setPixels won't clobber another write. > > Here, we don't know when the write is going to happen, and if someone else does > a write, we could clobber it with the async upload. So it feels to me like we > should be locking it until the upload is done, so we get predictable behaviour. > What do you think? Ok, that makes sense. I added the write lock but we probably need a read lock too, right?
On 2012/11/18 19:04:41, David Reveman wrote: > https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc > File cc/resource_provider.cc (right): > > https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... > cc/resource_provider.cc:813: context3d->bindTexture(GL_TEXTURE_2D, > resource->glId); > On 2012/11/18 18:22:49, danakj wrote: > > On 2012/11/18 18:15:59, David Reveman wrote: > > > On 2012/11/17 17:47:25, danakj wrote: > > > > Should this be locking for write? > > > > > > Maybe but we currently don't lock for write in setPixels and unless that's > > > incorrect I have a hard time seeing why we should lock here. > > > > > > The difference between setPixels and beginSetPixels is that in the later we > > > don't know when the upload will happen in relation to commands issued > through > > > the same command buffer. Is that somehow a reason to lock for write here? > > > > Well my thought is that setPixels doesn't set the lock since it does the write > > and is done, but it does check that no one else has a write lock, right? So > that > > setPixels won't clobber another write. > > > > Here, we don't know when the write is going to happen, and if someone else > does > > a write, we could clobber it with the async upload. So it feels to me like we > > should be locking it until the upload is done, so we get predictable > behaviour. > > What do you think? > > Ok, that makes sense. I added the write lock but we probably need a read lock > too, right? never mind, looks like locking for write implicitly also locks for read
LGTM with one question about the DCHECK https://codereview.chromium.org/11412043/diff/7001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/7001/cc/resource_provider.cc#ne... cc/resource_provider.cc:857: DCHECK(!resource->lockedForWrite); This is going to fail every time, since we have locked it, isn't it? Did you mean to DCHECK that it is locked?
https://codereview.chromium.org/11412043/diff/7001/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/7001/cc/resource_provider.cc#ne... cc/resource_provider.cc:857: DCHECK(!resource->lockedForWrite); On 2012/11/18 22:16:57, danakj wrote: > This is going to fail every time, since we have locked it, isn't it? Did you > mean to DCHECK that it is locked? Done.
Added abortSetPixels to the API. I'm going to split this CL into two changes, one that adds pixel buffer support and one that adds async upload support. The first can land now, while we need to wait for async upload support in the gpu process before we can land the second.
https://codereview.chromium.org/11412043/diff/11002/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/11002/cc/resource_provider.cc#n... cc/resource_provider.cc:906: resource->size, if we land this, should we file a followup bug that says make abort actually work? or, alternatively, should abort return a success code?
https://codereview.chromium.org/11412043/diff/11002/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/11002/cc/resource_provider.cc#n... cc/resource_provider.cc:906: resource->size, On 2012/11/23 22:16:42, nduca wrote: > if we land this, should we file a followup bug that says make abort actually > work? > > or, alternatively, should abort return a success code? I don't have any intentions to land this until we have some initial command buffer support in place. In the current CL I make the assumption that any non-async texture upload commands will force previously scheduled async uploads to the same texture to complete or when possible be canceled. That would allow us to use TexImage2D to abort an upload but it might be better to include an explicit abort command in the async command buffer API. I'll adjust this CL as the command buffer side of async uploads take shape. Important point is we definitely need the ability to abort uploads.
I feel like there's a semantic disconnect here about how wer'e defining abort. Does unmapPixelBuffer send anything to the gpu process? It shouldn't... - Create pbo id - Map pbo - Enqueue raster task When raster task is done: - if tile has "can_use_gpu_memory": - Unmap pbo - Acquire pbo - beginSetPixels on some texture id - release pbo - mark its tile as pending and start polling for its completion - else - unmap pbo Do we even need abort? If the map/unmap of a pbo purely a client-side operation, then there's no need to abort anyting. We've just made a pbo and we're deciding to not do a beginSetPixels withit...
On 2012/11/26 12:36:49, nduca wrote: > I feel like there's a semantic disconnect here about how wer'e defining abort. > > Does unmapPixelBuffer send anything to the gpu process? It shouldn't... it doesn't. > > > - Create pbo id > - Map pbo > - Enqueue raster task > > When raster task is done: > - if tile has "can_use_gpu_memory": > - Unmap pbo > - Acquire pbo > - beginSetPixels on some texture id > - release pbo > - mark its tile as pending and start polling for its completion > > - else > - unmap pbo > > Do we even need abort? If the map/unmap of a pbo purely a client-side operation, > then there's no need to abort anyting. We've just made a pbo and we're deciding > to not do a beginSetPixels withit... Yes, we can always avoid to start the upload after the raster task is done. CHROMIUM_pixel_transfer_buffer_object makes that easy and my tile manager CL has been doing that all along. abortSetPixels is for aborting an upload after issuing beginSetPixels. I think this will be important for performance and it makes it easier to implement a clean tear down of TileManager/ResourcePool. Maybe abortSetPixels is a bad name. stopSetPixels better?
Ok I think I follow.... the thing that I dont quite get is that abort||stopSet is racey --- it might fail, and knowing synchronously whether it did or not is hard, no? What happens when you tell it to abort just as the gpu process is about to (or just did) finish the upload?
On 2012/11/26 17:51:52, nduca wrote: > Ok I think I follow.... the thing that I dont quite get is that abort||stopSet > is racey --- it might fail, and knowing synchronously whether it did or not is > hard, no? What happens when you tell it to abort just as the gpu process is > about to (or just did) finish the upload? Yes, abortSetPixels provides no guarantee that the upload is actually aborted (another reason why "abort" is probably a bad name). You could use the ASYNC_TEXTURE_UPLOAD query to determine if the upload actually took place but I'm not aware of a situation where the compositor actually cares. I'm currently aborting the upload by issuing a texImage2D call and redefining the texture. That means the texture is in a consistent state once that command has been processed independent of if the async upload happened or not.
https://codereview.chromium.org/11412043/diff/3006/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/3006/cc/resource_provider.cc#ne... cc/resource_provider.cc:877: lockForWrite(id); We should do this for both software and GL. All it does is tracking the locking the semantics, and those should be the same for software and GL (whether upload actually happens asynchronously or not). Feel free to remove the state DCHECKs above that are covered by lockForWrite. https://codereview.chromium.org/11412043/diff/3006/cc/resource_provider.cc#ne... cc/resource_provider.cc:922: unlockForWrite(id); It would be useful I think to add some state on the resource to make sure we don't do something like rp->beginSetPixels(id); rp->unlockForWrite(id); rp->beginSetPixels(id); https://codereview.chromium.org/11412043/diff/3006/cc/resource_provider.h File cc/resource_provider.h (right): https://codereview.chromium.org/11412043/diff/3006/cc/resource_provider.h#new... cc/resource_provider.h:213: void beginSetPixels(ResourceId id); naming nit: I would name this something like setPixelsFromBufferAsync. The reason is that it does the same thing as setPixelsFromBuffer, but asynchronously. 'begin' makes me look for an 'end'
On 2012/11/18 18:15:59, David Reveman wrote: > https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc > File cc/resource_provider.cc (right): > > https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... > cc/resource_provider.cc:813: context3d->bindTexture(GL_TEXTURE_2D, > resource->glId); > On 2012/11/17 17:47:25, danakj wrote: > > Should this be locking for write? > > Maybe but we currently don't lock for write in setPixels and unless that's > incorrect I have a hard time seeing why we should lock here. Sorry for the belated answer, and it looks like you're doing close to the right thing on the later patch, but just to give background, the lockForRead/Write is essentially just a check for readers and writers to ensure we have consistent state. If you lockForWrite here you ensure that: - you own the texture you're uploading to (e.g. it's not sent to the parent compositor and you're uploading to it behind its back). - no-one will touch or read the texture while the upload is pending (which would be a race). Not locking won't break anything, but you'll be missing on useful checks (that caught issues in the past). > > The difference between setPixels and beginSetPixels is that in the later we > don't know when the upload will happen in relation to commands issued through > the same command buffer. Is that somehow a reason to lock for write here? > > https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.cc#ne... > cc/resource_provider.cc:858: if (resource->glId) { > On 2012/11/17 17:10:30, nduca wrote: > > behavior on context lost? > > This returns true on context lost (same behavior as for the queries used by the > TextureUploader). This should make sure the logic in the TileManager that uses > this async upload interface behave properly on context lost. > > https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h > File cc/resource_provider.h (right): > > https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h#new... > cc/resource_provider.h:205: void scheduleSetPixels(ResourceId id); > On 2012/11/17 17:10:30, nduca wrote: > > beginSetPixels? > > Done. > > https://codereview.chromium.org/11412043/diff/2001/cc/resource_provider.h#new... > cc/resource_provider.h:215: unsigned glPixelBufferId; > On 2012/11/17 17:10:30, nduca wrote: > > would comments on these vars help indicate that they're used together? > > Done.
Thanks for the feedback. I see the value in locking as a sanity check and it makes sense to take full advantage of that here. https://codereview.chromium.org/11412043/diff/3006/cc/resource_provider.cc File cc/resource_provider.cc (right): https://codereview.chromium.org/11412043/diff/3006/cc/resource_provider.cc#ne... cc/resource_provider.cc:877: lockForWrite(id); On 2012/11/27 00:40:01, piman wrote: > We should do this for both software and GL. All it does is tracking the locking > the semantics, and those should be the same for software and GL (whether upload > actually happens asynchronously or not). > > Feel free to remove the state DCHECKs above that are covered by lockForWrite. Done. https://codereview.chromium.org/11412043/diff/3006/cc/resource_provider.cc#ne... cc/resource_provider.cc:922: unlockForWrite(id); On 2012/11/27 00:40:01, piman wrote: > It would be useful I think to add some state on the resource to make sure we > don't do something like rp->beginSetPixels(id); rp->unlockForWrite(id); > rp->beginSetPixels(id); Done.
lgtm
lgtm Is this unblocked now that the API is in?
On 2012/12/04 16:17:17, epenner wrote: > lgtm > > Is this unblocked now that the API is in? yup, just need to land the WebKit side first: https://bugs.webkit.org/show_bug.cgi?id=103995
On 2012/12/04 17:03:20, David Reveman wrote: > On 2012/12/04 16:17:17, epenner wrote: > > lgtm > > > > Is this unblocked now that the API is in? > > yup, just need to land the WebKit side first: > https://bugs.webkit.org/show_bug.cgi?id=103995 Ahh, right. I forgot I added a cast for testing the functions. Hmm, is there still a good reason that we can't just do a cast to a chromium context in this class?
james, is there a good reason we don't cast to chromium context in the compositor?
On 2012/12/04 18:00:39, David Reveman wrote: > james, is there a good reason we don't cast to chromium context in the > compositor? What's "chromium context"? We use several different implementations of WebGraphicsContext3D in cc:: (content/common/gpu/client/web... in most production code, webkit/gpu/wgc3d_command_buffer_impl and webkit/gpu/wgc3d_impl in different cases in various test + less common modes. Longer-term, the solution here is to port all of cc:: to use gpu::GLES2Interface instead of WebGraphicsContext3D to avoid having to pipe stuff through WGC3D etc. That's probably a task for Q1
FYI, patch 10 was bad. I accidentally rebased and uploaded an old version of this CL. Latest patch should be good.
On 2012/12/04 20:29:46, jamesr wrote: > On 2012/12/04 18:00:39, David Reveman wrote: > > james, is there a good reason we don't cast to chromium context in the > > compositor? > > What's "chromium context"? We use several different implementations of > WebGraphicsContext3D in cc:: (content/common/gpu/client/web... in most > production code, webkit/gpu/wgc3d_command_buffer_impl and webkit/gpu/wgc3d_impl > in different cases in various test + less common modes. > > Longer-term, the solution here is to port all of cc:: to use gpu::GLES2Interface > instead of WebGraphicsContext3D to avoid having to pipe stuff through WGC3D etc. > That's probably a task for Q1 Thanks James, I realized I was using command-buffer-impl which as you mention, isn't always used.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/11412043/21001
Message was sent while issue was closed.
Change committed as 171657 |
