[FPF-CI] Track triggers when canvas is transferred to offscreen [chromium/src : main]
Tom Van Goethem (Gerrit)
Tom Van Goethem added 1 comment![]( "Open in Gerrit")
This CL is mostly ready, except for the tests - which I can't quite seem to figure out. See details in comment.
Related details
- Jean-Philippe Gravel
- John Kim
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Van Goethem (Gerrit)
Tom Van Goethem added 1 comment![]( "Open in Gerrit")
// This is failing because the resource provider is a
// CanvasResourceProviderBitmap instead of a
// CanvasResourceProviderSharedImage. This is because the context is not
// accelerated.@jpgr...@chromium.org - I was wondering if you could possibly point me in the right direction here? I've been doing some debugging, and it seems that the resource provider of the offscreen canvas is a `CanvasResourceProviderBitmap`, whereas I was expecting it to have a `CanvasResourceProviderSharedImage`. I presume that this is because the canvas is not accelerated (I'm running tests on a device without a GPU). Is there some other way that I can still get the CanvasResource that is created by the offscreen canvas? Or should I change the way that I'm trying to test things?
Related details
- Jean-Philippe Gravel
- John Kim
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 1 comment![]( "Open in Gerrit")
// This is failing because the resource provider is a
// CanvasResourceProviderBitmap instead of a
// CanvasResourceProviderSharedImage. This is because the context is not
// accelerated.@jpgr...@chromium.org - I was wondering if you could possibly point me in the right direction here? I've been doing some debugging, and it seems that the resource provider of the offscreen canvas is a `CanvasResourceProviderBitmap`, whereas I was expecting it to have a `CanvasResourceProviderSharedImage`. I presume that this is because the canvas is not accelerated (I'm running tests on a device without a GPU). Is there some other way that I can still get the CanvasResource that is created by the offscreen canvas? Or should I change the way that I'm trying to test things?
We have unit tests validating GPU accelerated code paths. [See how I enabled GPU acceleration in canvas_noise_test.cc](https://crrev.com/c/6438556) ;)
What happens here is that a software resource provider was used because GPU compositing was disabled ([see here](https://crsrc.org/c/third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc;l=646;drc=c06fea974510acde08228ce5a0dd6927436b4deb)).
You'll need to add this at the beginning of the test:
```
ScopedTestingPlatformSupport<GpuMemoryBufferTestPlatform> platform;
scoped_refptr<viz::TestContextProvider> test_context_provider =
viz::TestContextProvider::CreateRaster();
InitializeSharedGpuContextRaster(test_context_provider.get());
test_context_provider->GetTestRasterInterface()->set_gpu_rasterization(true);
```
and this at the end:
```
SharedGpuContext::Reset();
```
Related details
- John Kim
- Tom Van Goethem
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Van Goethem (Gerrit)
Tom Van Goethem added 1 comment![]( "Open in Gerrit")
// This is failing because the resource provider is a
// CanvasResourceProviderBitmap instead of a
// CanvasResourceProviderSharedImage. This is because the context is not
// accelerated.Jean-Philippe Gravel@jpgr...@chromium.org - I was wondering if you could possibly point me in the right direction here? I've been doing some debugging, and it seems that the resource provider of the offscreen canvas is a `CanvasResourceProviderBitmap`, whereas I was expecting it to have a `CanvasResourceProviderSharedImage`. I presume that this is because the canvas is not accelerated (I'm running tests on a device without a GPU). Is there some other way that I can still get the CanvasResource that is created by the offscreen canvas? Or should I change the way that I'm trying to test things?
We have unit tests validating GPU accelerated code paths. [See how I enabled GPU acceleration in canvas_noise_test.cc](https://crrev.com/c/6438556) ;)
What happens here is that a software resource provider was used because GPU compositing was disabled ([see here](https://crsrc.org/c/third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc;l=646;drc=c06fea974510acde08228ce5a0dd6927436b4deb)).
You'll need to add this at the beginning of the test:
```
ScopedTestingPlatformSupport<GpuMemoryBufferTestPlatform> platform;
scoped_refptr<viz::TestContextProvider> test_context_provider =
viz::TestContextProvider::CreateRaster();
InitializeSharedGpuContextRaster(test_context_provider.get());
test_context_provider->GetTestRasterInterface()->set_gpu_rasterization(true);
```and this at the end:
```
SharedGpuContext::Reset();
```
Ahhh, I should've remembered that :) That seems to have fixed the test. Thanks a lot!
Related details
- Jean-Philippe Gravel
- John Kim
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Kim (Gerrit)
John Kim voted and added 1 comment![]( "Open in Gerrit")
- Jean-Philippe Gravel
- Tom Van Goethem
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 2 comments![]( "Open in Gerrit")
When a canvas is transferred to offscreen, we should keep track of the operations that are made on them such that we can determine whether the canvas should be noised.I don't quite follow. When transferring to offscreen, the original canvas becomes a placeholder and all of the state tracking and rendering is handled by an `OffscreenCanvas` and an `OffscreenCanvasRenderingContext2D` object. That context knows it's own intervention triggers and the offscreen canvas can already noise it's output, without having to synchronize additional states.
When reading back from the placeholder canvas, [the offscreen resource is read](https://crsrc.org/c/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;l=1345;drc=13075886efc851f658b6b14500272e62e5521614), and this happens [before the noise is applied](https://crsrc.org/c/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;l=1504,1508;drc=13075886efc851f658b6b14500272e62e5521614).
It seems to me that noise should already be applied correctly. Or am I missing something?
(Host() && Host()->HasTriggerForIntervention());I don't think that this delegation via the host make sense, or is needed. There is only ever a single context for a given canvas.
If a 2D HTMLCanvasElement is transferred to offscreen, the HTMLCanvasElement becomes a placeholder canvas and [it will never have an associated context of it's own](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/htmlcanvas/html_canvas_element_module.cc;l=22-28;drc=13075886efc851f658b6b14500272e62e5521614).
Thus, the only context at play will be an `OffscreenCanvasRenderingContext2D`, which knows it's own trigger intervention status. No need to query the host.
Related details
- Tom Van Goethem
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 1 comment![]( "Open in Gerrit")
When a canvas is transferred to offscreen, we should keep track of the operations that are made on them such that we can determine whether the canvas should be noised.I don't quite follow. When transferring to offscreen, the original canvas becomes a placeholder and all of the state tracking and rendering is handled by an `OffscreenCanvas` and an `OffscreenCanvasRenderingContext2D` object. That context knows it's own intervention triggers and the offscreen canvas can already noise it's output, without having to synchronize additional states.
When reading back from the placeholder canvas, [the offscreen resource is read](https://crsrc.org/c/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;l=1345;drc=13075886efc851f658b6b14500272e62e5521614), and this happens [before the noise is applied](https://crsrc.org/c/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;l=1504,1508;drc=13075886efc851f658b6b14500272e62e5521614).
It seems to me that noise should already be applied correctly. Or am I missing something?
Ha, here's something I overlooked. [When reading back a placeholder canvas, `context_` will be nullptr](https://crsrc.org/c/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;l=1509;drc=13075886efc851f658b6b14500272e62e5521614) and thus, noise won't be applied. But this CL isn't addressing this issue.
Tom Van Goethem (Gerrit)
Tom Van Goethem added 2 comments![]( "Open in Gerrit")
When a canvas is transferred to offscreen, we should keep track of the operations that are made on them such that we can determine whether the canvas should be noised.Jean-Philippe GravelI don't quite follow. When transferring to offscreen, the original canvas becomes a placeholder and all of the state tracking and rendering is handled by an `OffscreenCanvas` and an `OffscreenCanvasRenderingContext2D` object. That context knows it's own intervention triggers and the offscreen canvas can already noise it's output, without having to synchronize additional states.
When reading back from the placeholder canvas, [the offscreen resource is read](https://crsrc.org/c/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;l=1345;drc=13075886efc851f658b6b14500272e62e5521614), and this happens [before the noise is applied](https://crsrc.org/c/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;l=1504,1508;drc=13075886efc851f658b6b14500272e62e5521614).
It seems to me that noise should already be applied correctly. Or am I missing something?
Ha, here's something I overlooked. [When reading back a placeholder canvas, `context_` will be nullptr](https://crsrc.org/c/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc;l=1509;drc=13075886efc851f658b6b14500272e62e5521614) and thus, noise won't be applied. But this CL isn't addressing this issue.
Yeah, you're right. The context indeed is aware which triggers happened. I've updated the code and commit message to mainly take into account `context_` being a nullptr when transferring to an offscreen canvas.
(Host() && Host()->HasTriggerForIntervention());I don't think that this delegation via the host make sense, or is needed. There is only ever a single context for a given canvas.
If a 2D HTMLCanvasElement is transferred to offscreen, the HTMLCanvasElement becomes a placeholder canvas and [it will never have an associated context of it's own](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/htmlcanvas/html_canvas_element_module.cc;l=22-28;drc=13075886efc851f658b6b14500272e62e5521614).
Thus, the only context at play will be an `OffscreenCanvasRenderingContext2D`, which knows it's own trigger intervention status. No need to query the host.
Updated. I've moved the checks to `BaseRenderingContext2D`, which I think makes more sense.
Related details
- Jean-Philippe Gravel
- John Kim
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |