Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1653983002:
cc: Avoid extra flush calls to Skia GrContext (Closed)
|
Created:
4 years, 10 months ago by Kimmo Kinnunen Modified:
4 years, 10 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Avoid extra flush calls to Skia GrContext
Avoid calling GrContext::flush() upon every tile raster. The correct
way to ensure rendering has been submitted to GPU API level is by
"preparing for external io" the render target.
This fixes the problem where Skia GPU resource cache resources grow
old too quickly. Each flush will age the resources, and after
becoming too old (64), the resources will be redundantly destroyed upon
the next resource allocation. The flush is intended to be called at the
rate the order of frames. Before this fix, the flush was called at least
the amount relative to the amount of tiles. For MSAA, flush was called
more than one time per tile. This fix reduces the amount of flushes.
Also, after this fix the effect of potential extra flushes can be fixed
in Skia.
BUG=582858
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/adbacd950053d80419e4cfec6f998ccb470fc029
Cr-Commit-Position: refs/heads/master@{#375152}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 2
Patch Set 3 : address review comment #Patch Set 4 : eh #
Total comments: 7
Patch Set 5 : remove the filter hunk #
Depends on Patchset: Messages
Total messages: 43 (13 generated)
Description was changed from ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. BUG=582858 ========== to ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1653983002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1653983002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at the rate relative to the amount of tiles. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
kkinnunen@nvidia.com changed reviewers: + bsalomon@google.com, ericrk@chromium.org
ericrk@ for the review bsalomon@ for to confirm Skia design related to the aging mechanism. Maybe it would be also good to document the cases when is it correct to call GrContext::flush ? (is it ever?)
Description was changed from ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at the rate relative to the amount of tiles. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at the rate relative to the amount of tiles. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at the rate relative to the amount of tiles. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at least the amount relative to the amount of tiles. For MSAA, flush was called more than one time per tile. This fix reduces the amount of flushes. Also, after this fix the effect of potential extra flushes can be fixed in Skia. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://chromiumcodereview.appspot.com/1653983002/diff/1/cc/raster/gpu_raster... File cc/raster/gpu_rasterizer.cc (right): https://chromiumcodereview.appspot.com/1653983002/diff/1/cc/raster/gpu_raster... cc/raster/gpu_rasterizer.cc:82: // This is undocumented: What is undocumented? Are we missing something on the Skia side?
On 2016/02/01 14:55:59, bsalomon wrote: > https://chromiumcodereview.appspot.com/1653983002/diff/1/cc/raster/gpu_raster... > File cc/raster/gpu_rasterizer.cc (right): > > https://chromiumcodereview.appspot.com/1653983002/diff/1/cc/raster/gpu_raster... > cc/raster/gpu_rasterizer.cc:82: // This is undocumented: > What is undocumented? Are we missing something on the Skia side? That you must call SkCanvas::flush in order to do SkGpuDevice deferred clear, undocumented in the sense that deferred clear is not done if not flushed, not even when canvas and gpudevice goes out of scope. That you can call SkCanvas::flush in order to get prepare for external io (api says that it only "flushes")
On 2016/02/01 08:47:36, Kimmo Kinnunen wrote: > ericrk@ for the review > bsalomon@ for to confirm Skia design related to the aging mechanism. Maybe it > would be also good to document the cases when is it correct to call > GrContext::flush ? (is it ever?) Yes, that is the current implementation of aging. Longer term I'd like to get a frame pulse from Chrome (and other clients) rather than rely on flush. We will probably remove GrContext::flush eventually and would prefer clients use SkCanvas::flush (or eventually a replacement API on SkSurface). I'm working on a CL to update docs and test/fix deferred clears for GPU canvases.
On 2016/02/01 22:23:24, bsalomon wrote: > On 2016/02/01 08:47:36, Kimmo Kinnunen wrote: > > ericrk@ for the review > > bsalomon@ for to confirm Skia design related to the aging mechanism. Maybe it > > would be also good to document the cases when is it correct to call > > GrContext::flush ? (is it ever?) > > Yes, that is the current implementation of aging. Longer term I'd like to get a > frame pulse from Chrome (and other clients) rather than rely on flush. We will > probably remove GrContext::flush eventually and would prefer clients use > SkCanvas::flush (or eventually a replacement API on SkSurface). I'm working on a > CL to update docs and test/fix deferred clears for GPU canvases. Right, removed the comment. PTAL.
kkinnunen@nvidia.com changed reviewers: + senorblanco@google.com
Is there a test or performance benchmark which shows this bug? Ideally, we should add a Telemetry test to exercise it.
On 2016/02/02 13:44:55, Stephen White wrote: > Is there a test or performance benchmark which shows this bug? Ideally, we > should add a Telemetry test to exercise it. Yeah, I'd like that, especially eventually. The UI interaction is just panning, or then zooming and then panning, and then observing the resource creation/destruction cycles, when there ought not to be any. The UI might be a bit jankier as a result, but it's does not change from "janky" to "not janky". This comes up (in my testing) when gl path rendering is enabled with the special flag. Naturally the bug is there also for texture resources, but it might not be that visible. If you have a test commandline in mind that would exercise this, I could confirm this manually. For adding a totally new telemetry test: I feel that me digging through how to use telemetry correctly, running a special version of it with --enable-gl-path-rendering, writing an extra telemetry test for it, digging through the gpu bots to find if the proper HW is available is quite tall order for me. I'm not familiar with the infrastructure and I as third-party engineer communicating with US timezone Chromium engineers takes fairly big amount of time. Given the above, it'd be rather inconvenient to block fixing this on the grounds of not having a telemetry test. Especially as part of the problem above (not having gl path rendering on by default) is blocked by this bug (gl path rendering slower than it should be because of resource cache issues). The long term plan would be that changes similar to this patch would have effect on telemetry tests and everything would be just business as usual..
On 2016/02/02 14:08:08, Kimmo Kinnunen wrote: > On 2016/02/02 13:44:55, Stephen White wrote: > > Is there a test or performance benchmark which shows this bug? Ideally, we > > should add a Telemetry test to exercise it. > > Yeah, I'd like that, especially eventually. And of course, if it is a requirement to get a telemetry benchmark showing benefit of this patch, I'm happy to work towards adding that.
On 2016/02/02 15:31:36, Kimmo Kinnunen wrote: > On 2016/02/02 14:08:08, Kimmo Kinnunen wrote: > > On 2016/02/02 13:44:55, Stephen White wrote: > > > Is there a test or performance benchmark which shows this bug? Ideally, we > > > should add a Telemetry test to exercise it. > > > > Yeah, I'd like that, especially eventually. > > And of course, if it is a requirement to get a telemetry benchmark showing > benefit of this patch, I'm happy to work towards adding that. It's not a requirement, since the change is fairly small. If Brian's ok with it, we can just land it. But having a benchmark that we can track in Telemetry will ensure that it won't regress. Does the Chalkboard demo show any impact, for example? (https://web.archive.org/web/20150611004841/http://ie.microsoft.com/testdrive/...) That one's already in Telemetry.
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... File cc/raster/gpu_rasterizer.cc (right): https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... cc/raster/gpu_rasterizer.cc:82: multi_picture_draw.draw(true); This means we'll be calling SkCanvas::flush() on each multi_picture_draw tile, even in the non-MSAA case. Brian, does that seem acceptable to you?
On 2016/02/02 17:00:02, Stephen White wrote: > https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... > File cc/raster/gpu_rasterizer.cc (right): > > https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... > cc/raster/gpu_rasterizer.cc:82: multi_picture_draw.draw(true); > This means we'll be calling SkCanvas::flush() on each multi_picture_draw tile, > even in the non-MSAA case. Brian, does that seem acceptable to you? As I understand it this reduces the number of flushes. Is that not correct?
On 2016/02/02 17:08:02, bsalomon wrote: > On 2016/02/02 17:00:02, Stephen White wrote: > > > https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... > > File cc/raster/gpu_rasterizer.cc (right): > > > > > https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... > > cc/raster/gpu_rasterizer.cc:82: multi_picture_draw.draw(true); > > This means we'll be calling SkCanvas::flush() on each multi_picture_draw tile, > > even in the non-MSAA case. Brian, does that seem acceptable to you? > > As I understand it this reduces the number of flushes. Is that not correct? If the gr_context->flush() happening in ScopedGpuRaster::EndGpuRaster() was also per-tile, then yes, we'd be trading one-for-one (GrContext::flush for SkCanvas::flush) in the non-MSAA case. Maybe ericrk@ could verify that? If that's true, non-MSAA goes from from N GrContext::flush() to N SkCanvas::flush(), and MSAA goes from N GrContext::flush() plus N SkCanvas::flush() to N SkCanvas::flush(). Does that sound right?
On 2016/02/01 22:23:24, bsalomon wrote: > On 2016/02/01 08:47:36, Kimmo Kinnunen wrote: > > ericrk@ for the review > > bsalomon@ for to confirm Skia design related to the aging mechanism. Maybe it > > would be also good to document the cases when is it correct to call > > GrContext::flush ? (is it ever?) > > Yes, that is the current implementation of aging. Longer term I'd like to get a > frame pulse from Chrome (and other clients) rather than rely on flush. We will > probably remove GrContext::flush eventually and would prefer clients use > SkCanvas::flush (or eventually a replacement API on SkSurface). I'm working on a > CL to update docs and test/fix deferred clears for GPU canvases. The deferred clear mechanism has been removed and the comment on SkCanvas::flush updated.
https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... File cc/raster/gpu_rasterizer.cc (right): https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... cc/raster/gpu_rasterizer.cc:82: multi_picture_draw.draw(true); On 2016/02/02 17:00:01, Stephen White wrote: > This means we'll be calling SkCanvas::flush() on each multi_picture_draw tile, > even in the non-MSAA case. Brian, does that seem acceptable to you? We were already flushing due to the ScopedGpuRaster - we'd call flush when this scope exited. On the other hand, do we need to flush here before releasing the surface via the write_lock? or can we just pass false here, and handle the flush via the scoped_gpu_raster? It seems like we may be able to achieve the same effect by just changing this to false and not modifying gl_rasterizer.cc or scoped_gpu_raster.cc - I might prefer that.
Description was changed from ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. Instead of doing only "prepare for external io" the render target, "flush" the SkCanvas. This prepares as well as prevents other potential future bug, omitting to apply deferred clear in SkGpuDevice. Though currently, deferred clear is always applied due to somewhat unrelated call sequences. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at least the amount relative to the amount of tiles. For MSAA, flush was called more than one time per tile. This fix reduces the amount of flushes. Also, after this fix the effect of potential extra flushes can be fixed in Skia. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at least the amount relative to the amount of tiles. For MSAA, flush was called more than one time per tile. This fix reduces the amount of flushes. Also, after this fix the effect of potential extra flushes can be fixed in Skia. For filters, flush the drawing before unlocking the filtered source texture. This fixes the bugs related to overwriting the source texture before Skia uses it. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
On 2016/02/08 18:08:19, ericrk wrote: > It seems like we may be able to achieve the same effect by just changing this to > false and not modifying gl_rasterizer.cc or scoped_gpu_raster.cc - I might > prefer that. Right, I of course understand why you would prefer it. It's just a bit more confusing, due to existence of SkCanvas::flush(). I changed it to your preference. Also added a comment to gl_renderer.cc
https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... cc/resources/resource_provider.cc:992: gr_surface_ = Why this change? Is there something missing from the NewWrappedRenderTarget API that is available via wrapBackendTexture()? I'd like to eventually make all external textures and FBOs be imported via SkSurface and SkImage APIs and remove the methods from GrTextureProvider.
https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... cc/resources/resource_provider.cc:992: gr_surface_ = On 2016/02/09 14:55:56, bsalomon wrote: > Why this change? Is there something missing from the NewWrappedRenderTarget API > that is available via wrapBackendTexture()? I'd like to eventually make all > external textures and FBOs be imported via SkSurface and SkImage APIs and remove > the methods from GrTextureProvider. To get a GrSurface reference. Maybe I'm missing an accessor that is there? https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... cc/resources/resource_provider.cc:1002: gr_surface_->prepareForExternalIO(); For this call, we need GrSurface.
https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... cc/resources/resource_provider.cc:1002: gr_surface_->prepareForExternalIO(); On 2016/02/09 18:13:44, Kimmo Kinnunen wrote: > For this call, we need GrSurface. Or are you hinting it'd be better just call sk_surface->getCanvas()->flush(); ?
https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... cc/resources/resource_provider.cc:992: gr_surface_ = On 2016/02/09 18:13:44, Kimmo Kinnunen wrote: > On 2016/02/09 14:55:56, bsalomon wrote: > > Why this change? Is there something missing from the NewWrappedRenderTarget > API > > that is available via wrapBackendTexture()? I'd like to eventually make all > > external textures and FBOs be imported via SkSurface and SkImage APIs and > remove > > the methods from GrTextureProvider. > > To get a GrSurface reference. Maybe I'm missing an accessor that is there? Oh, of course. https://codereview.chromium.org/1653983002/diff/60001/cc/resources/resource_p... cc/resources/resource_provider.cc:1002: gr_surface_->prepareForExternalIO(); On 2016/02/09 18:17:27, Kimmo Kinnunen wrote: > On 2016/02/09 18:13:44, Kimmo Kinnunen wrote: > > For this call, we need GrSurface. > > Or are you hinting it'd be better just call sk_surface->getCanvas()->flush(); ? I think that'd be preferable. I had been thinking that flush() should probably be moved to SkSurface and perhaps renamed. If that would make this more readable I'm happy to land a CL in Skia.
On 2016/02/09 18:22:12, bsalomon wrote: > I think that'd be preferable. I had been thinking that flush() should probably > be moved to SkSurface and perhaps renamed. If that would make this more readable > I'm happy to land a CL in Skia. I think that would be clearer. If you can remove it from SkCanvas, maybe that would be even better :)
https://chromiumcodereview.appspot.com/1653983002/diff/60001/cc/raster/gpu_ra... File cc/raster/gpu_rasterizer.cc (right): https://chromiumcodereview.appspot.com/1653983002/diff/60001/cc/raster/gpu_ra... cc/raster/gpu_rasterizer.cc:82: multi_picture_draw.draw(false); Not sure I follow the change here. I was saying that you may be able to just pass false here and leave the flush in EndGpuRaster... just setting false here I think misses necessary flushes. Feel free to move back to your previous approach.
Description was changed from ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at least the amount relative to the amount of tiles. For MSAA, flush was called more than one time per tile. This fix reduces the amount of flushes. Also, after this fix the effect of potential extra flushes can be fixed in Skia. For filters, flush the drawing before unlocking the filtered source texture. This fixes the bugs related to overwriting the source texture before Skia uses it. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at least the amount relative to the amount of tiles. For MSAA, flush was called more than one time per tile. This fix reduces the amount of flushes. Also, after this fix the effect of potential extra flushes can be fixed in Skia. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
On 2016/02/09 19:31:46, ericrk wrote: > https://chromiumcodereview.appspot.com/1653983002/diff/60001/cc/raster/gpu_ra... > File cc/raster/gpu_rasterizer.cc (right): > > https://chromiumcodereview.appspot.com/1653983002/diff/60001/cc/raster/gpu_ra... > cc/raster/gpu_rasterizer.cc:82: multi_picture_draw.draw(false); > Not sure I follow the change here. I was saying that you may be able to just > pass false here and leave the flush in EndGpuRaster... just setting false here I > think misses necessary flushes. No, it's not missing flushes. This was discussed already in the commit message. > Feel free to move back to your previous > approach. Hmm.. Right. The previous approach really was there already, should you have had desire to look at it. So it seems this is pretty hard to get in, for some reason. I separated the filter hunk to another patch. PTAL, when you have time. I also provide 2 alternatives to this patch. Please choose your preference.
On 2016/02/02 at 17:19:23, senorblanco wrote: > On 2016/02/02 17:08:02, bsalomon wrote: > > On 2016/02/02 17:00:02, Stephen White wrote: > > > > > https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... > > > File cc/raster/gpu_rasterizer.cc (right): > > > > > > > > https://codereview.chromium.org/1653983002/diff/20001/cc/raster/gpu_rasterize... > > > cc/raster/gpu_rasterizer.cc:82: multi_picture_draw.draw(true); > > > This means we'll be calling SkCanvas::flush() on each multi_picture_draw tile, > > > even in the non-MSAA case. Brian, does that seem acceptable to you? > > > > As I understand it this reduces the number of flushes. Is that not correct? > > If the gr_context->flush() happening in ScopedGpuRaster::EndGpuRaster() was also per-tile, then yes, > we'd be trading one-for-one (GrContext::flush for SkCanvas::flush) in the non-MSAA case. Maybe ericrk@ could verify that? > > If that's true, non-MSAA goes from from N GrContext::flush() to N SkCanvas::flush(), and > MSAA goes from N GrContext::flush() plus N SkCanvas::flush() to N SkCanvas::flush(). > > Does that sound right? This sounds right. I think this is strictly a win.
On 2016/02/11 at 17:02:09, kkinnunen wrote: > On 2016/02/09 19:31:46, ericrk wrote: > > https://chromiumcodereview.appspot.com/1653983002/diff/60001/cc/raster/gpu_ra... > > File cc/raster/gpu_rasterizer.cc (right): > > > > https://chromiumcodereview.appspot.com/1653983002/diff/60001/cc/raster/gpu_ra... > > cc/raster/gpu_rasterizer.cc:82: multi_picture_draw.draw(false); > > Not sure I follow the change here. I was saying that you may be able to just > > pass false here and leave the flush in EndGpuRaster... just setting false here I > > think misses necessary flushes. > > No, it's not missing flushes. This was discussed already in the commit message. > > > Feel free to move back to your previous > > approach. > > Hmm.. Right. The previous approach really was there already, should you have had desire to look at it. > > So it seems this is pretty hard to get in, for some reason. > > I separated the filter hunk to another patch. PTAL, when you have time. > > I also provide 2 alternatives to this patch. Please choose your preference. I see what we're doing now - I missed some of the details in the last patch. LGTM.
The CQ bit was checked by kkinnunen@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1653983002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1653983002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at least the amount relative to the amount of tiles. For MSAA, flush was called more than one time per tile. This fix reduces the amount of flushes. Also, after this fix the effect of potential extra flushes can be fixed in Skia. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Avoid extra flush calls to Skia GrContext Avoid calling GrContext::flush() upon every tile raster. The correct way to ensure rendering has been submitted to GPU API level is by "preparing for external io" the render target. This fixes the problem where Skia GPU resource cache resources grow old too quickly. Each flush will age the resources, and after becoming too old (64), the resources will be redundantly destroyed upon the next resource allocation. The flush is intended to be called at the rate the order of frames. Before this fix, the flush was called at least the amount relative to the amount of tiles. For MSAA, flush was called more than one time per tile. This fix reduces the amount of flushes. Also, after this fix the effect of potential extra flushes can be fixed in Skia. BUG=582858 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/adbacd950053d80419e4cfec6f998ccb470fc029 Cr-Commit-Position: refs/heads/master@{#375152} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/adbacd950053d80419e4cfec6f998ccb470fc029 Cr-Commit-Position: refs/heads/master@{#375152} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
