Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1157943004:
cc: [WIP] Use worker context and OrderingBarrierCHROMIUM for one-copy. (Closed)
Descriptioncc: Use worker context and OrderingBarrierCHROMIUM for one-copy.
This patch makes the texture copy operation in a worker context
instead of the main cc context, and flushes the output to cc on
completion.
BUG=490295
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Patch Set 1 #Patch Set 2 : do copy in worker thread. #
Total comments: 27
Patch Set 3 : review comments addressed. #
Total comments: 3
Patch Set 4 : handle sync while copying texture. #
Total comments: 1
Patch Set 5 : worker context in resource provider. #Patch Set 6 : avoid invoking ResourceProvider from worker thread. #Patch Set 7 : piman's comments. #
Total comments: 22
Patch Set 8 : review comments + command completed check. #
Total comments: 16
Patch Set 9 : piman's comments #
Total comments: 4
Patch Set 10 : keep resource ownership in compositor. #
Total comments: 2
Messages
Total messages: 34 (4 generated)
sohan.jyoti@samsung.com changed reviewers: + danakj@chromium.org, reveman@chromium.org
Can you please take a look. thanks.
This doesn't move the copy operations to a worker context. It just makes some unnecessary worker context calls. Also, the point of using a worker context is to be able to issue copy operations on the worker thread.
On 2015/06/04 13:52:12, reveman wrote: > This doesn't move the copy operations to a worker context. It just makes some > unnecessary worker context calls. Also, the point of using a worker context is > to be able to issue copy operations on the worker thread. Ok. Let me update this. thanks.
On 2015/06/04 14:05:38, sohanjg wrote: > On 2015/06/04 13:52:12, reveman wrote: > > This doesn't move the copy operations to a worker context. It just makes some > > unnecessary worker context calls. Also, the point of using a worker context is > > to be able to issue copy operations on the worker thread. > > Ok. Let me update this. thanks. Sorry, i assumed copy operation was already on the worker thread. Will it make sense, if we avoid posting AdvanceLastFlushedCopyTo, to cc thread and post it to current message loop. And use the remaining worker context provider code of this patch ?
How does this change what context the upload is done on? The copy happens in ResourceProvider not in this class.
On Thu, Jun 4, 2015 at 10:07 AM, <danakj@chromium.org> wrote: > How does this change what context the upload is done on? The copy happens > in > ResourceProvider not in this class. > Ah, sorry I see David said the same thing. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Can you please take a look, if this is what we want ? Based on your review, i'll update the tests. thanks.
Can you please take a look, if this is what we want ? Based on your review, i'll update the tests. thanks.
https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (left): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:103: const int kMaxCopyOperations = 32; we still need this https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:338: IssueCopyOperations(new CopyOperation( This is leaking a CopyOperations instance. IssueCopyOperations function and CopyOperations struct are both unnecessary when performing the copy on a worker thread. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:342: context_provider->ContextGL()->OrderingBarrierCHROMIUM(); please add comment explaining this ordering barrier https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:368: resource_provider_->CopyResource(copy_operation->src->id(), We can't use ResourceProvider on this thread. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.h (right): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:65: void PlaybackAndCopyOnWorkerThread( Is this function needed? Can't RasterBuffer::Playback do all the work now as it's supposed to? https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:111: void AdvanceLastFlushedCopyTo(CopySequenceNumber sequence); is this used? https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:113: void ScheduleCheckForCompletedCopyOperationsWithLockAcquired( used? https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:115: void CheckForCompletedCopyOperations(bool wait_if_needed); used? https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:138: base::Lock lock_; why do we still need this? https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:140: CopyOperation::Deque pending_copy_operations_; used? https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:141: base::TimeTicks last_check_for_completed_copy_operations_time_; used?
Patchset #3 (id:40001) has been deleted
please take a look, thanks. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (left): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:103: const int kMaxCopyOperations = 32; On 2015/06/09 13:30:55, reveman wrote: > we still need this i cant find it being used in this file, now that we have removed the sync code. am i missing something ? https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:338: IssueCopyOperations(new CopyOperation( On 2015/06/09 13:30:55, reveman wrote: > This is leaking a CopyOperations instance. IssueCopyOperations function and > CopyOperations struct are both unnecessary when performing the copy on a worker > thread. Done. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:342: context_provider->ContextGL()->OrderingBarrierCHROMIUM(); On 2015/06/09 13:30:55, reveman wrote: > please add comment explaining this ordering barrier Done. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:368: resource_provider_->CopyResource(copy_operation->src->id(), On 2015/06/09 13:30:55, reveman wrote: > We can't use ResourceProvider on this thread. Done. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.h (right): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:65: void PlaybackAndCopyOnWorkerThread( On 2015/06/09 13:30:55, reveman wrote: > Is this function needed? Can't RasterBuffer::Playback do all the work now as > it's supposed to? Hmm..except for max_bytes_per_copy_operation_ we can get all values here. Do you want me to plumb the settings here ? https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:111: void AdvanceLastFlushedCopyTo(CopySequenceNumber sequence); On 2015/06/09 13:30:56, reveman wrote: > is this used? Done. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:113: void ScheduleCheckForCompletedCopyOperationsWithLockAcquired( On 2015/06/09 13:30:55, reveman wrote: > used? Done. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:115: void CheckForCompletedCopyOperations(bool wait_if_needed); On 2015/06/09 13:30:55, reveman wrote: > used? Done. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:138: base::Lock lock_; On 2015/06/09 13:30:56, reveman wrote: > why do we still need this? we will need it, to get GpuMemoryBuffer right ? in playback. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:140: CopyOperation::Deque pending_copy_operations_; On 2015/06/09 13:30:56, reveman wrote: > used? Done. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:141: base::TimeTicks last_check_for_completed_copy_operations_time_; On 2015/06/09 13:30:56, reveman wrote: > used? Done. https://codereview.chromium.org/1157943004/diff/60001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/60001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:330: do we need all the validation code, that we do in ResourceProvider::CopyResources, here ?
https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (left): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:103: const int kMaxCopyOperations = 32; On 2015/06/09 at 15:59:23, sohanjg wrote: > On 2015/06/09 13:30:55, reveman wrote: > > we still need this > > i cant find it being used in this file, now that we have removed the sync code. am i missing something ? We still want to limit the number of copy operations that can be in flight. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.h (right): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:65: void PlaybackAndCopyOnWorkerThread( On 2015/06/09 at 15:59:24, sohanjg wrote: > On 2015/06/09 13:30:55, reveman wrote: > > Is this function needed? Can't RasterBuffer::Playback do all the work now as > > it's supposed to? > > Hmm..except for max_bytes_per_copy_operation_ we can get all values here. Do you want me to plumb the settings here ? Let's keep PlaybackAndCopyOnWorkerThread until the patch is closer to ready. This might be the best way to throttle copy operations. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:138: base::Lock lock_; On 2015/06/09 at 15:59:24, sohanjg wrote: > On 2015/06/09 13:30:56, reveman wrote: > > why do we still need this? > > we will need it, to get GpuMemoryBuffer right ? in playback. GpuMemoryBuffer allocation is thread safe so you don't need it for that but you probably need it to throttle copy operations. https://codereview.chromium.org/1157943004/diff/60001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/60001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:330: On 2015/06/09 at 15:59:24, sohanjg wrote: > do we need all the validation code, that we do in ResourceProvider::CopyResources, here ? You'll need the commands completed queries as a start. Otherwise we have no idea when the copy operation has completed and the resource can be reused.
Please update IsZeroCopyUploadEnabled in compositor_util.cc as part of this change.
On 2015/06/10 04:38:28, reveman wrote: > Please update IsZeroCopyUploadEnabled in compositor_util.cc as part of this > change. OK. few questions. 1. Is it meaningful to add read_lock_fence member in resource class ? like in ResourceProvider resource struct or maintain only in func scope ? 2. same for gl_read_lock_query_id ? 3. CopyTextureFence how should we share from resource_provider.h ? Maintain separate file ? Pull it inside ResourceProvider class ? 4. what will be the throttling logic ?
Can you please take a look. thanks. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (left): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:103: const int kMaxCopyOperations = 32; On 2015/06/09 18:00:30, reveman wrote: > On 2015/06/09 at 15:59:23, sohanjg wrote: > > On 2015/06/09 13:30:55, reveman wrote: > > > we still need this > > > > i cant find it being used in this file, now that we have removed the sync > code. am i missing something ? > > We still want to limit the number of copy operations that can be in flight. Acknowledged. https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.h (right): https://codereview.chromium.org/1157943004/diff/20001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.h:65: void PlaybackAndCopyOnWorkerThread( On 2015/06/09 18:00:30, reveman wrote: > On 2015/06/09 at 15:59:24, sohanjg wrote: > > On 2015/06/09 13:30:55, reveman wrote: > > > Is this function needed? Can't RasterBuffer::Playback do all the work now as > > > it's supposed to? > > > > Hmm..except for max_bytes_per_copy_operation_ we can get all values here. Do > you want me to plumb the settings here ? > > Let's keep PlaybackAndCopyOnWorkerThread until the patch is closer to ready. > This might be the best way to throttle copy operations. Acknowledged. https://codereview.chromium.org/1157943004/diff/60001/cc/raster/one_copy_tile... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/60001/cc/raster/one_copy_tile... cc/raster/one_copy_tile_task_worker_pool.cc:330: On 2015/06/09 18:00:30, reveman wrote: > On 2015/06/09 at 15:59:24, sohanjg wrote: > > do we need all the validation code, that we do in > ResourceProvider::CopyResources, here ? > > You'll need the commands completed queries as a start. Otherwise we have no idea > when the copy operation has completed and the resource can be reused. Done.
I think we need to take a step back and make sure we understand how this is supposed to work. Here are some questions that you need to answer before you can implement this: 1. What context will own the "commands completed" queries? 2. How are we going to check if a staging resource is available for reuse and assign it to a task? 3. How and where are we going to block when too many copy operations are in-flight? https://codereview.chromium.org/1157943004/diff/80001/content/browser/gpu/com... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1157943004/diff/80001/content/browser/gpu/com... content/browser/gpu/compositor_util.cc:248: ; typo?
Patchset #6 (id:120001) has been deleted
Hi reveman@, As i couldn't reach you last week, i discussed the CL with piman@. As per that, we should be doing, 1. on the compositor thread, with the compositor context, we the LazyCreate and create/acquire the GpuMemoryBuffer and the dest resource. We are already doing a lot of it in RasterBufferImpl Ctor we create the source gpu mem buffer, and the dest we already have created in RasterTask creation. 2. on the worker thread, with the worker context, we do the raster into the GpuMemoryBuffer, then we unmap it, and we do the copy. We are already doing all of this in PlaybackAndCopyOnWorkerThread. 3. on the compositor thread we create the image and set up the image_id and read_lock_fence etc. We are already doing part of of this in RasterBufferImpl Dtor. I haven't yet gone completely in the staging and re-using part. But, doing the basic steps, it doesn't render anything on the screen. The problem with the current CL, seems to be the copy op in worker context doesnt have the texture ref, for the gl id's passed to it. Even though we have generated textures all right on the cc context. Even when i try and bind in RasterBufferImpl Ctor, group_->bind_generates_resource() check fails. Can we really access textures generated in cc context from worker context ? (I tried putting an OrderingBarrier in RasterBufferImpl Ctor, but no luck :/) Please let me know, what i am missing here.
sohan.jyoti@samsung.com changed reviewers: + piman@chromium.org
On Tue, Jun 30, 2015 at 7:26 AM, <sohan.jyoti@samsung.com> wrote: > Hi reveman@, > > As i couldn't reach you last week, i discussed the CL with piman@. As per > that, > we should be doing, > 1. on the compositor thread, with the compositor context, we the > LazyCreate and > create/acquire the GpuMemoryBuffer and the dest resource. > > We are already doing a lot of it in RasterBufferImpl Ctor we create the > source > gpu mem buffer, and the dest we already have created in RasterTask > creation. > > 2. on the worker thread, with the worker context, we do the raster into the > GpuMemoryBuffer, then we unmap it, and we do the copy. > > We are already doing all of this in PlaybackAndCopyOnWorkerThread. > > 3. on the compositor thread we create the image and set up the image_id and > read_lock_fence etc. > > We are already doing part of of this in RasterBufferImpl Dtor. > > > I haven't yet gone completely in the staging and re-using part. But, doing > the > basic steps, it doesn't render anything on the screen. > > The problem with the current CL, seems to be the copy op in worker context > doesnt have the texture ref, for the gl id's passed to it. Even though we > have > generated textures all right on the cc context. > Even when i try and bind in RasterBufferImpl Ctor, > group_->bind_generates_resource() check fails. > > Can we really access textures generated in cc context from worker context > ? (I > tried putting an OrderingBarrier in RasterBufferImpl Ctor, but no luck :/) > You need to use mailboxes to move textures between contexts. > > Please let me know, what i am missing here. > > > https://codereview.chromium.org/1157943004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
thanks Dana. will try that. piman@, i have updated as per your comments offline. Please take a look. Still nothing on-screen though :( thanks.
On Tue, Jun 30, 2015 at 11:51 AM, Dana Jansens <danakj@chromium.org> wrote: > On Tue, Jun 30, 2015 at 7:26 AM, <sohan.jyoti@samsung.com> wrote: > >> Hi reveman@, >> >> As i couldn't reach you last week, i discussed the CL with piman@. As >> per that, >> we should be doing, >> 1. on the compositor thread, with the compositor context, we the >> LazyCreate and >> create/acquire the GpuMemoryBuffer and the dest resource. >> >> We are already doing a lot of it in RasterBufferImpl Ctor we create the >> source >> gpu mem buffer, and the dest we already have created in RasterTask >> creation. >> >> 2. on the worker thread, with the worker context, we do the raster into >> the >> GpuMemoryBuffer, then we unmap it, and we do the copy. >> >> We are already doing all of this in PlaybackAndCopyOnWorkerThread. >> >> 3. on the compositor thread we create the image and set up the image_id >> and >> read_lock_fence etc. >> >> We are already doing part of of this in RasterBufferImpl Dtor. >> >> >> I haven't yet gone completely in the staging and re-using part. But, >> doing the >> basic steps, it doesn't render anything on the screen. >> >> The problem with the current CL, seems to be the copy op in worker context >> doesnt have the texture ref, for the gl id's passed to it. Even though we >> have >> generated textures all right on the cc context. >> Even when i try and bind in RasterBufferImpl Ctor, >> group_->bind_generates_resource() check fails. >> >> Can we really access textures generated in cc context from worker context >> ? (I >> tried putting an OrderingBarrier in RasterBufferImpl Ctor, but no luck :/) >> > > You need to use mailboxes to move textures between contexts. > You don't have to use mailboxes in this case, because the worker context and the compositor context are in the same share group. That's consistent with the GPU raster pool. > >> >> Please let me know, what i am missing here. >> >> >> https://codereview.chromium.org/1157943004/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry for not being able to review this recently. I think the crux is resource handling. ResourcePool was designed to be used on the compositor thread using the main context. Maybe we should make the ResourcePool class thread safe and able to use a worker context.
A bunch of comments / direction suggestion, but reveman@ should take a closer look. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... File cc/raster/one_copy_tile_task_worker_pool.cc (left): https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:388: ScheduleCheckForCompletedCopyOperationsWithLockAcquired(wait_if_needed); I think we still want some form of throttling, though I'm not sure what form it should take. (it's not strictly necessary for correct rendering, but needed for smoothness/scheduling etc.). https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:68: raster_resource_->size().height()); I don't think you want to do all that for the source texture. This is done today in LazyAllocate in CopyResource, but only if !resource->allocated. However ScopedWriteLockGpuMemoryBuffer sets resource->allocated to true after setting the image_id - which you will do on the worker thread, before the copy. Basically, the LazyCreate in ScopedWriteLockGpuMemoryBufferForThread is all you should need for the raster resource. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:70: gl->BindTexture(GL_TEXTURE_2D, output_resource_->id()); That id is the id in the ResourceProvider, not the GL id - so this won't do what you want. What you're trying to do here is LazyAllocate the resource. I think the best way to do it is to grab a ScopedWriteLockGL for the output resource, which will do the LazyAllocate, and get you the texture_id that you'll need on the worker thread. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:78: gl->OrderingBarrierCHROMIUM(); I don't think you need this one, because you're adding one in ScheduleTask below. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:363: gl->CreateImageCHROMIUM(gpu_memory_buffer->AsClientBuffer(), You need to create the image lazily. Maybe the best way is to have a ScopedWriteLockGpuMemoryBufferForThread::CreateAndBindImage which will do all of this logic (create the image lazily, set allocated, etc.) as well as the logic below wrt ReleaseTexImage2DCHROMIUM/BindTexImage2DCHROMIUM What this really means is that you probably don't need to keep each field of the Resource inside the ScopedWriteLockGpuMemoryBufferForThread. All you need a single bool ("did_bind_image" ?) of whether you called CreateAndBindImage image, since that one resets everything to a known state: - allocated = true - dirty_image = false - read_lock_fences_enabled = true - bound_image_id = image_id You can set all of these in the ScopedWriteLockGpuMemoryBufferForThread destructor if did_bind_image is true. If it's not, then you just don't touch any of these fields in the resource. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:407: rows_to_copy, false, false, false); We need to add the query logic, so that we know when it's safe to write to the GpuMemoryBuffer again in a later frame. My suggestion is to add a couple of calls to the ScopedWriteLockGpuMemoryBufferForThread, something like a BeginCopy/EndCopy. - in the constructor, we would create the query id (if needed and if using sync_query) - BeginCopy would do the BeginQueryEXT (if using sync_query), - EndCopy would do the EndQueryEXT (if using sync_query), - in the destructor, we would create the CopyTextureFence out of the query id (if using sync_query) or create the SynchronousFence (if not using sync_query), and set that fence on the resource_->read_lock_fence https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.cc:1010: image_id_ = 0; I think you want to initialize with resource_->image_id here, so that you reuse the image if it was already created. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.cc:1013: read_lock_fences_enabled_ = false; Same here for all 3, you may need to get the values from resource_. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.cc:1023: if (!resource_->image_id) { you don't need / want this test any more if you initialize image_id_ with resource_->image_id in the constructor. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.cc:1040: resource_provider_->use_persistent_map_for_gpu_memory_buffers() If this is called on the worker thread, it's not safe to call into resource_provider_. You probably want to figure out the usage value in the constructor. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.h:374: unsigned dest_gl_id_; These 2 fields are not initialized. For that matter, dest_gl_id_ doesn't belong here. And source_gl_id() should really be resource_->gl_id.
Finaaally we have rendered with this patchset! thanks piman@ :) Please take a look. We would need to address the throttling logic next maybe. thanks again. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... File cc/raster/one_copy_tile_task_worker_pool.cc (left): https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:388: ScheduleCheckForCompletedCopyOperationsWithLockAcquired(wait_if_needed); On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > I think we still want some form of throttling, though I'm not sure what form it > should take. > (it's not strictly necessary for correct rendering, but needed for > smoothness/scheduling etc.). hmm..yes we need some mechanism to block too many in-flight copies..reveman@ had some suggestion about this, using the resource pool busy resources count. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:68: raster_resource_->size().height()); On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > I don't think you want to do all that for the source texture. This is done today > in LazyAllocate in CopyResource, but only if !resource->allocated. However > ScopedWriteLockGpuMemoryBuffer sets resource->allocated to true after setting > the image_id - which you will do on the worker thread, before the copy. > Basically, the LazyCreate in ScopedWriteLockGpuMemoryBufferForThread is all you > should need for the raster resource. Done. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:70: gl->BindTexture(GL_TEXTURE_2D, output_resource_->id()); On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > That id is the id in the ResourceProvider, not the GL id - so this won't do what > you want. > > What you're trying to do here is LazyAllocate the resource. I think the best way > to do it is to grab a ScopedWriteLockGL for the output resource, which will do > the LazyAllocate, and get you the texture_id that you'll need on the worker > thread. Done. Ahh..i was assuming that TileManager::CreateRasterTask, already has done a GenTex in CreateManagedResource, to get the gl_id. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:78: gl->OrderingBarrierCHROMIUM(); On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > I don't think you need this one, because you're adding one in ScheduleTask > below. Done. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:363: gl->CreateImageCHROMIUM(gpu_memory_buffer->AsClientBuffer(), On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > You need to create the image lazily. Maybe the best way is to have a > ScopedWriteLockGpuMemoryBufferForThread::CreateAndBindImage which will do all of > this logic (create the image lazily, set allocated, etc.) as well as the logic > below wrt ReleaseTexImage2DCHROMIUM/BindTexImage2DCHROMIUM > > What this really means is that you probably don't need to keep each field of the > Resource inside the ScopedWriteLockGpuMemoryBufferForThread. All you need a > single bool ("did_bind_image" ?) of whether you called CreateAndBindImage image, > since that one resets everything to a known state: > - allocated = true > - dirty_image = false > - read_lock_fences_enabled = true > - bound_image_id = image_id > > You can set all of these in the ScopedWriteLockGpuMemoryBufferForThread > destructor if did_bind_image is true. If it's not, then you just don't touch any > of these fields in the resource. Done. thanks! this makes it a lot cleaner. https://codereview.chromium.org/1157943004/diff/160001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:407: rows_to_copy, false, false, false); On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > We need to add the query logic, so that we know when it's safe to write to the > GpuMemoryBuffer again in a later frame. > > My suggestion is to add a couple of calls to the > ScopedWriteLockGpuMemoryBufferForThread, something like a BeginCopy/EndCopy. > - in the constructor, we would create the query id (if needed and if using > sync_query) > - BeginCopy would do the BeginQueryEXT (if using sync_query), > - EndCopy would do the EndQueryEXT (if using sync_query), > - in the destructor, we would create the CopyTextureFence out of the query id > (if using sync_query) or create the SynchronousFence (if not using sync_query), > and set that fence on the resource_->read_lock_fence Done. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.cc:1010: image_id_ = 0; On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > I think you want to initialize with resource_->image_id here, so that you reuse > the image if it was already created. Done. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.cc:1013: read_lock_fences_enabled_ = false; On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > Same here for all 3, you may need to get the values from resource_. Done. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.cc:1023: if (!resource_->image_id) { On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > you don't need / want this test any more if you initialize image_id_ with > resource_->image_id in the constructor. Done. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.cc:1040: resource_provider_->use_persistent_map_for_gpu_memory_buffers() On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > If this is called on the worker thread, it's not safe to call into > resource_provider_. You probably want to figure out the usage value in the > constructor. Done. https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/1157943004/diff/160001/cc/resources/resource_... cc/resources/resource_provider.h:374: unsigned dest_gl_id_; On 2015/07/01 22:11:05, piman (Very slow to review) wrote: > These 2 fields are not initialized. For that matter, dest_gl_id_ doesn't belong > here. And source_gl_id() should really be resource_->gl_id. Done.
Some comments, but really need reveman@ to take a look and we do need some form of throttling before this can go in. https://codereview.chromium.org/1157943004/diff/180001/cc/raster/one_copy_til... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/180001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:56: output_resource_->id()); So, it's only valid to write to (copy to) the resource while the lock is taken, in other words, you need to keep this lock until the worker thread task is done - similarly to the ScopedWriteLockGpuMemoryBufferForThread, so you need to keep it as a field in the RasterBufferImpl and not release it until the destructor. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1017: dest_gl_id_ = dest_texture_id; I don't think this belongs to the ScopedWriteLockGpuMemoryBufferForThread, it's not used at all in this class. Instead, you should keep around the ScopedWriteLockGL in the RasterBufferImpl (see comments there too) and get the texture id from it. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1019: query_id_ = 0; nit: always initialize all fields (i.e. you want to initialize query_id_ even if !resource_provider_->use_sync_query_) here, I think we want to reuse the query from a previous copy to the same texture, i.e. we should take resource_->gl_read_lock_query_id, and store it back in the destructor. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1020: using_sync_query_ = true; In particular, this one needs to be initialized to false in the !resource_provider_->use_sync_query_ case. So how about always initializing: query_id_ = resource_->gl_read_lock_query_id; using_sync_query_ = resource_provider_->use_sync_query_; https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1037: make_scoped_refptr(new CopyTextureFence(gl, query_id_)); As mentioned above, you need to save the query for later reuse: resource_->gl_read_lock_query_id = query_id_ https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1044: resource_provider_->synchronous_fence_->has_synchronized()) nit: needs {} per style guide https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1081: read_lock_fences_enabled_ = true; You don't need those 2 as a field, you can just set them to true on the resource in the if (did_bind_image_) block in the destructor (l.1054/1058). https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1087: dirty_image_ = false; Same for this, don't need it as a field, you can just set to false on the resource on l.1055.
PTAL, waiting for reveman@ to have a look. and then finalize the throttling logic. thanks. https://codereview.chromium.org/1157943004/diff/180001/cc/raster/one_copy_til... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/180001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:56: output_resource_->id()); On 2015/07/08 23:12:13, piman (Very slow to review) wrote: > So, it's only valid to write to (copy to) the resource while the lock is taken, > in other words, you need to keep this lock until the worker thread task is done > - similarly to the ScopedWriteLockGpuMemoryBufferForThread, so you need to keep > it as a field in the RasterBufferImpl and not release it until the destructor. Done. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1017: dest_gl_id_ = dest_texture_id; On 2015/07/08 23:12:13, piman (Very slow to review) wrote: > I don't think this belongs to the ScopedWriteLockGpuMemoryBufferForThread, it's > not used at all in this class. Instead, you should keep around the > ScopedWriteLockGL in the RasterBufferImpl (see comments there too) and get the > texture id from it. Done. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1019: query_id_ = 0; On 2015/07/08 23:12:13, piman (Very slow to review) wrote: > nit: always initialize all fields (i.e. you want to initialize query_id_ even if > !resource_provider_->use_sync_query_) > > here, I think we want to reuse the query from a previous copy to the same > texture, i.e. we should take resource_->gl_read_lock_query_id, and store it back > in the destructor. Done. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1020: using_sync_query_ = true; On 2015/07/08 23:12:13, piman (Very slow to review) wrote: > In particular, this one needs to be initialized to false in the > !resource_provider_->use_sync_query_ case. > > So how about always initializing: > query_id_ = resource_->gl_read_lock_query_id; > using_sync_query_ = resource_provider_->use_sync_query_; Done. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1037: make_scoped_refptr(new CopyTextureFence(gl, query_id_)); On 2015/07/08 23:12:13, piman (Very slow to review) wrote: > As mentioned above, you need to save the query for later reuse: > resource_->gl_read_lock_query_id = query_id_ Done. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1044: resource_provider_->synchronous_fence_->has_synchronized()) On 2015/07/08 23:12:13, piman (Very slow to review) wrote: > nit: needs {} per style guide Done. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1081: read_lock_fences_enabled_ = true; On 2015/07/08 23:12:13, piman (Very slow to review) wrote: > You don't need those 2 as a field, you can just set them to true on the resource > in the if (did_bind_image_) block in the destructor (l.1054/1058). Done. https://codereview.chromium.org/1157943004/diff/180001/cc/resources/resource_... cc/resources/resource_provider.cc:1087: dirty_image_ = false; On 2015/07/08 23:12:13, piman (Very slow to review) wrote: > Same for this, don't need it as a field, you can just set to false on the > resource on l.1055. Done.
https://codereview.chromium.org/1157943004/diff/200001/cc/raster/one_copy_til... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/200001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:83: reusing_raster_resource, lock_.Pass(), gl_lock_.Pass(), Oh, actually, something I missed in previous reviews. This would cause the ScopedWriteLockGpuMemoryBufferForThread and the ScopedWriteLockGL to be released on the worker thread, but they need to be released on the compositor thread. The best I think is to keep those as owned by the RasterBufferImpl, and pass them by pointer rather than via Passed. The RasterBufferImpl will be destroyed on the compositor thread once the RasterTask is done. https://codereview.chromium.org/1157943004/diff/200001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1157943004/diff/200001/cc/resources/resource_... cc/resources/resource_provider.cc:1051: resource_->dirty_image = true; = false
PTAL. thanks. And reg throttling, is it safe to access resource pool, and do CheckBusyResources. Or maintain a counter on worker context, and limit copy there.(to 32 as before?) https://codereview.chromium.org/1157943004/diff/200001/cc/raster/one_copy_til... File cc/raster/one_copy_tile_task_worker_pool.cc (right): https://codereview.chromium.org/1157943004/diff/200001/cc/raster/one_copy_til... cc/raster/one_copy_tile_task_worker_pool.cc:83: reusing_raster_resource, lock_.Pass(), gl_lock_.Pass(), On 2015/07/10 02:25:09, piman (Very slow to review) wrote: > Oh, actually, something I missed in previous reviews. This would cause the > ScopedWriteLockGpuMemoryBufferForThread and the ScopedWriteLockGL to be released > on the worker thread, but they need to be released on the compositor thread. > > The best I think is to keep those as owned by the RasterBufferImpl, and pass > them by pointer rather than via Passed. The RasterBufferImpl will be destroyed > on the compositor thread once the RasterTask is done. Done. https://codereview.chromium.org/1157943004/diff/200001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1157943004/diff/200001/cc/resources/resource_... cc/resources/resource_provider.cc:1051: resource_->dirty_image = true; On 2015/07/10 02:25:09, piman (Very slow to review) wrote: > = false Acknowledged.
https://codereview.chromium.org/1157943004/diff/220001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1157943004/diff/220001/cc/resources/resource_... cc/resources/resource_provider.cc:1032: make_scoped_refptr(new CopyTextureFence(gl, query_id_)); How does is this supposed to work? We have to create a fence for the context that we used to perform the copy operation and not the cc thread. We also want to use these fences to block on the worker context. I think we need to create the query for the worker context on the worker thread as part of issuing the copy command. This also means that you can't use the "busy" login in the resource pool to check for completion. I'm wondering if it wouldn't be much easier to avoid using the ResourcePool class and ResourceProvider for staging resources all together and just manage staging resources inside the OneCopyTileTaskWorkerPool.
https://codereview.chromium.org/1157943004/diff/220001/cc/resources/resource_... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1157943004/diff/220001/cc/resources/resource_... cc/resources/resource_provider.cc:1032: make_scoped_refptr(new CopyTextureFence(gl, query_id_)); On 2015/07/10 13:50:34, reveman wrote: > How does is this supposed to work? We have to create a fence for the context > that we used to perform the copy operation and not the cc thread. We also want > to use these fences to block on the worker context. > > I think we need to create the query for the worker context on the worker thread > as part of issuing the copy command. The query is created on the worker context around the copy (see BeginCopyTexture/EndCopyTexture), but you're right that we need to use the worker context here rather than the compositor context, but causes further problems since queries are not shared across contexts. > This also means that you can't use the > "busy" login in the resource pool to check for completion. > > I'm wondering if it wouldn't be much easier to avoid using the ResourcePool > class and ResourceProvider for staging resources all together and just manage > staging resources inside the OneCopyTileTaskWorkerPool. I think that's a really reasonable approach.
On 2015/07/10 at 17:12:36, piman wrote: > https://codereview.chromium.org/1157943004/diff/220001/cc/resources/resource_... > File cc/resources/resource_provider.cc (right): > > https://codereview.chromium.org/1157943004/diff/220001/cc/resources/resource_... > cc/resources/resource_provider.cc:1032: make_scoped_refptr(new CopyTextureFence(gl, query_id_)); > On 2015/07/10 13:50:34, reveman wrote: > > How does is this supposed to work? We have to create a fence for the context > > that we used to perform the copy operation and not the cc thread. We also want > > to use these fences to block on the worker context. > > > > I think we need to create the query for the worker context on the worker thread > > as part of issuing the copy command. > > The query is created on the worker context around the copy (see BeginCopyTexture/EndCopyTexture), but you're right that we need to use the worker context here rather than the compositor context, but causes further problems since queries are not shared across contexts. > > > This also means that you can't use the > > "busy" login in the resource pool to check for completion. > > > > I'm wondering if it wouldn't be much easier to avoid using the ResourcePool > > class and ResourceProvider for staging resources all together and just manage > > staging resources inside the OneCopyTileTaskWorkerPool. > > I think that's a really reasonable approach. I'll try to put together a simple patch that shows what this direction would look like.
On 2015/07/10 at 19:56:07, reveman wrote: > On 2015/07/10 at 17:12:36, piman wrote: > > https://codereview.chromium.org/1157943004/diff/220001/cc/resources/resource_... > > File cc/resources/resource_provider.cc (right): > > > > https://codereview.chromium.org/1157943004/diff/220001/cc/resources/resource_... > > cc/resources/resource_provider.cc:1032: make_scoped_refptr(new CopyTextureFence(gl, query_id_)); > > On 2015/07/10 13:50:34, reveman wrote: > > > How does is this supposed to work? We have to create a fence for the context > > > that we used to perform the copy operation and not the cc thread. We also want > > > to use these fences to block on the worker context. > > > > > > I think we need to create the query for the worker context on the worker thread > > > as part of issuing the copy command. > > > > The query is created on the worker context around the copy (see BeginCopyTexture/EndCopyTexture), but you're right that we need to use the worker context here rather than the compositor context, but causes further problems since queries are not shared across contexts. > > > > > This also means that you can't use the > > > "busy" login in the resource pool to check for completion. > > > > > > I'm wondering if it wouldn't be much easier to avoid using the ResourcePool > > > class and ResourceProvider for staging resources all together and just manage > > > staging resources inside the OneCopyTileTaskWorkerPool. > > > > I think that's a really reasonable approach. > > I'll try to put together a simple patch that shows what this direction would look like. Something like this seem to work well: https://codereview.chromium.org/1230203007 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
