Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769573002:
Return overlay resources to renderers in larger batches. (Closed)
DescriptionReturn overlay resources to renderers in larger batches.
This allows the ResourceProvider to create a single sync token for all
the resources, and makes it more likely that a single IPC will be sent
to return all of them. For YUV frames all the planes should have the
same release sync token so VideoFrame::UpdateReleaseSyncToken won't
have to wait on the multiple existing tokens and generate a completely
new one.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2769573002
Cr-Commit-Position: refs/heads/master@{#459930}
Committed: https://chromium.googlesource.com/chromium/src/+/f4c12323bc9ef46575de81489ce7afbf57484d5c
Patch Set 1 #Patch Set 2 : add test #Patch Set 3 : rename #
Total comments: 3
Patch Set 4 : add ScopedBatchReturnResources #
Total comments: 1
Patch Set 5 : make friend #
Messages
Total messages: 40 (25 generated)
Description was changed from ========== Return overlay resources to renderers in larger batches. This allows the ResourceProvider create a single sync token for all the resources, and makes it more likely that a single IPC will be sent to return all of them. For YUV frames all the planes should have the same release sync token so VideoFrame::UpdateReleaseSyncToken won't have to wait on the multiple existing tokens and generate a completely new one. ========== to ========== Return overlay resources to renderers in larger batches. This allows the ResourceProvider create a single sync token for all the resources, and makes it more likely that a single IPC will be sent to return all of them. For YUV frames all the planes should have the same release sync token so VideoFrame::UpdateReleaseSyncToken won't have to wait on the multiple existing tokens and generate a completely new one. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Return overlay resources to renderers in larger batches. This allows the ResourceProvider create a single sync token for all the resources, and makes it more likely that a single IPC will be sent to return all of them. For YUV frames all the planes should have the same release sync token so VideoFrame::UpdateReleaseSyncToken won't have to wait on the multiple existing tokens and generate a completely new one. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Return overlay resources to renderers in larger batches. This allows the ResourceProvider to create a single sync token for all the resources, and makes it more likely that a single IPC will be sent to return all of them. For YUV frames all the planes should have the same release sync token so VideoFrame::UpdateReleaseSyncToken won't have to wait on the multiple existing tokens and generate a completely new one. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jbauman@chromium.org changed reviewers: + ccameron@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ccameron@chromium.org changed reviewers: + reveman@chromium.org
This lgtm, to the extent that I understand it -- it may be a good idea to have reveman take a look as well.
reveman@chromium.org changed reviewers: + dcastagna@chromium.org
+dcastagna
What's the use case this is optimizing?
On 2017/03/23 22:45:27, reveman wrote: > What's the use case this is optimizing? On windows when we promote an NV12 texture to an overlay we currently return the UV plane immediately when the next frame is aggregated, because there's no ScopedReadLock associated with it. The Y plane is returned after the next frame swaps, so each plane has a different sync token. The VideoFrame in the renderer has to wait on both sync tokens and generate a new one (because it can only hold one sync token at a time). This means that 2 unnecessary sync tokens are generated every frame, which requires 2 unnecessary sync IPCs to the GPU process. I also suspect that we generate a new sync token for every overlay tile resource that's returned on mac, though I haven't tested that.
Ok, I guess I'm missing some context for why UV plane is returned immediately but assuming that's what we want long term then this lgtm as long as it doesn't delay the return of resources on ChromeOS.
On 2017/03/24 01:30:09, reveman wrote: > Ok, I guess I'm missing some context for why UV plane is returned immediately > but assuming that's what we want long term then this lgtm as long as it doesn't > delay the return of resources on ChromeOS. This patch also prevents the UV plane from being returned immediately (that's what the changes to dc_layer_overlay.cc are for).
On 2017/03/24 at 01:32:41, jbauman wrote: > On 2017/03/24 01:30:09, reveman wrote: > > Ok, I guess I'm missing some context for why UV plane is returned immediately > > but assuming that's what we want long term then this lgtm as long as it doesn't > > delay the return of resources on ChromeOS. > > This patch also prevents the UV plane from being returned immediately (that's what the changes to dc_layer_overlay.cc are for). Why wouldn't we want to always batch returning resources then? Is that going to delay when we return resources?
https://codereview.chromium.org/2769573002/diff/40001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/2769573002/diff/40001/cc/output/gl_renderer.c... cc/output/gl_renderer.cc:2663: resource_provider_->SetBatchReturnResources(true); Should we have something like ScopedBacthReturnResources instead? Otherwise it seems like it could be easy to forget to set it back to false and that would cause issues. https://codereview.chromium.org/2769573002/diff/40001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/2769573002/diff/40001/cc/resources/resource_p... cc/resources/resource_provider.cc:1675: DCHECK(child_it != children_.end()); nit: DCHECK_NE? https://codereview.chromium.org/2769573002/diff/40001/cc/resources/resource_p... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/2769573002/diff/40001/cc/resources/resource_p... cc/resources/resource_provider.h:786: std::map<int, ResourceIdArray> batched_returning_resources_; How many child ids do you expect to have? Should we use SmallMap instead?
On 2017/03/24 01:38:56, Daniele Castagna wrote: > On 2017/03/24 at 01:32:41, jbauman wrote: > > On 2017/03/24 01:30:09, reveman wrote: > > > Ok, I guess I'm missing some context for why UV plane is returned > immediately > > > but assuming that's what we want long term then this lgtm as long as it > doesn't > > > delay the return of resources on ChromeOS. > > > > This patch also prevents the UV plane from being returned immediately (that's > what the changes to dc_layer_overlay.cc are for). > > Why wouldn't we want to always batch returning resources then? Is that going to > delay when we return resources? Yeah, we want to just batch within a call stack or else returning resources could be delayed by quite a bit.
On 2017/03/24 at 01:46:49, jbauman wrote: > On 2017/03/24 01:38:56, Daniele Castagna wrote: > > On 2017/03/24 at 01:32:41, jbauman wrote: > > > On 2017/03/24 01:30:09, reveman wrote: > > > > Ok, I guess I'm missing some context for why UV plane is returned > > immediately > > > > but assuming that's what we want long term then this lgtm as long as it > > doesn't > > > > delay the return of resources on ChromeOS. > > > > > > This patch also prevents the UV plane from being returned immediately (that's > > what the changes to dc_layer_overlay.cc are for). > > > > Why wouldn't we want to always batch returning resources then? Is that going to > > delay when we return resources? > > Yeah, we want to just batch within a call stack or else returning resources could be delayed by quite a bit. Yep, noticed that looking at the rest of the code, that's why I suggested a Scoped thing. Btw, on CrOS dealying returning resources is a big issue. We composite/schedule for overlays buffers from Android/SurfaceFlinger. They usually have a bufferqueue with size 3, holding onto a buffer for a few ms more can mean stalling the android app.
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. On 2017/03/24 01:44:43, Daniele Castagna wrote: > https://codereview.chromium.org/2769573002/diff/40001/cc/output/gl_renderer.cc > File cc/output/gl_renderer.cc (right): > > https://codereview.chromium.org/2769573002/diff/40001/cc/output/gl_renderer.c... > cc/output/gl_renderer.cc:2663: > resource_provider_->SetBatchReturnResources(true); > Should we have something like ScopedBacthReturnResources instead? > Otherwise it seems like it could be easy to forget to set it back to false and > that would cause issues. Done. > > https://codereview.chromium.org/2769573002/diff/40001/cc/resources/resource_p... > File cc/resources/resource_provider.cc (right): > > https://codereview.chromium.org/2769573002/diff/40001/cc/resources/resource_p... > cc/resources/resource_provider.cc:1675: DCHECK(child_it != children_.end()); > nit: DCHECK_NE? This can't be done because DCHECK_NE can't print arbitrary iterators when the test fails. > > https://codereview.chromium.org/2769573002/diff/40001/cc/resources/resource_p... > File cc/resources/resource_provider.h (right): > > https://codereview.chromium.org/2769573002/diff/40001/cc/resources/resource_p... > cc/resources/resource_provider.h:786: std::map<int, ResourceIdArray> > batched_returning_resources_; > How many child ids do you expect to have? > Should we use SmallMap instead? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. https://codereview.chromium.org/2769573002/diff/60001/cc/resources/resource_p... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/2769573002/diff/60001/cc/resources/resource_p... cc/resources/resource_provider.h:541: void SetBatchReturnResources(bool aggregate); nit: Can this be moved away from the public ResourceProvider API? Maybe it could be private and friend of the ScopedBatchReturnResources, WDYT?
The CQ bit was checked by jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, ccameron@chromium.org, dcastagna@chromium.org Link to the patchset: https://codereview.chromium.org/2769573002/#ps80001 (title: "make friend")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490650244703170,
"parent_rev": "b1b0096d4e30422d3084c0e238684e45ff61083c", "commit_rev":
"f4c12323bc9ef46575de81489ce7afbf57484d5c"}
Message was sent while issue was closed.
Description was changed from ========== Return overlay resources to renderers in larger batches. This allows the ResourceProvider to create a single sync token for all the resources, and makes it more likely that a single IPC will be sent to return all of them. For YUV frames all the planes should have the same release sync token so VideoFrame::UpdateReleaseSyncToken won't have to wait on the multiple existing tokens and generate a completely new one. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Return overlay resources to renderers in larger batches. This allows the ResourceProvider to create a single sync token for all the resources, and makes it more likely that a single IPC will be sent to return all of them. For YUV frames all the planes should have the same release sync token so VideoFrame::UpdateReleaseSyncToken won't have to wait on the multiple existing tokens and generate a completely new one. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2769573002 Cr-Commit-Position: refs/heads/master@{#459930} Committed: https://chromium.googlesource.com/chromium/src/+/f4c12323bc9ef46575de81489ce7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f4c12323bc9ef46575de81489ce7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
