[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. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 11 comments![]( "Open in Gerrit")
I left comments based on the current state of the code, but I however have doubt around the use of the context to propagate the intervention flag.
The placeholder never depended directly on the offscreen canvas. Adding a new dependency between the two could have implications around garbage collection and object destruction order. More importantly however, I think that using the context to propagate the flag opens the door to bugs.
Consider the following:
```C++
offscreen_canvas = canvas.transferControlToOffscreen();
context = offscreen_canvas.getContext('2d');
context.fillText(...); // Taints the context.
// Wait for the frame to propagate to the placeholder.
await new Promise(resolve => requestAnimationFrame(resolve));
// Reset the context, this removes the taint.
context.reset();
// Read back the frame.
// The context is no longer tainted, but the placeholder still holds
// the frame with drawn text.
url = canvas.toDataURL();
```
I think that propagating the intervention flag via the `CanvasResource` (what the CL originally did) might be preferable. This is how the origin-clean flag is propagated. It does make sense for a canvas resource to carry the taint because that's what hold the image.
Member<OffscreenCanvas> controlling_offscreen_canvas_;Logically, this would belong to the `OffscreenCanvasPlaceholder` parent class. Move the attribute that the setter there?
if (HasOffscreenCanvasFrame()) {It might be safer to check `controlling_offscreen_canvas_` instead of `HasOffscreenCanvasFrame()`? Both should be equivalent, but checking the pointer offers stronger guaranties that it's valid before we use it.
context = controlling_offscreen_canvas_->RenderingContext();If we move `controlling_offscreen_canvas_` to the parent class (as suggested in other comment), perhaps a adding a getter for `controlling_offscreen_canvas_` or `controlling_offscreen_canvas_ ? controlling_offscreen_canvas_->RenderingContext() : nullptr` in `OffscreenCanvasPlaceholder ` might be useful to centralize and isolate accesses to `controlling_offscreen_canvas_`.
return triggers_for_intervention_;Why not keep using `GetTriggersForIntervention()`, like `CanvasRenderingContext2D::GetCanvasTriggerOperations` originally did, and similarly to how `ShouldTriggerIntervention()` above isn't reading `triggers_for_intervention_` directly either? Reading this attribute directly pulls a member from the parent class, breaking encapsulation.
canvas.SetControllingOffscreenCanvas(offscreen_canvas);I think that this should be moved into `TransferControlToOffscreenInternal`, essentially next to (or leveraging?) `canvas.RegisterPlaceholderCanvas()`. If another call was ever added to `TransferControlToOffscreenInternal`, we wouldn't what the transfer to exclude this new code.
OffscreenCanvas* offscreen_canvas =The lack of line break makes it hard to see the different parts of the tests. Good tests should be structured with a setup, act and expectation section. Maybe add a line break here and another before the first call to `SetCanvasInterventionsForceDisabled` to make these sections stand out better?
auto* script_state = ToScriptStateForMainWorld(GetDocument().GetFrame());Maybe just write `ScriptState*` explicitly? From the style guide:
“Do not use [auto] merely to avoid the inconvenience of writing an explicit type.”
https://google.github.io/styleguide/cppguide.html#Type_deduction
auto* script_state = ToScriptStateForMainWorld(GetDocument().GetFrame());
context->setFillStyle(script_state->GetIsolate(),
ToV8Traits<IDLString>::ToV8(script_state, "red"),
exception_state);
context->fillRect(0, 0, 50, 50);What's this for? Wasn't the `fillText` sufficient to trigger the behavior we want?
context->PushFrame();
CanvasResourceProvider* provider = context->GetResourceProviderForCanvas2D();
scoped_refptr<CanvasResource> canvas_resource =
provider->ProduceCanvasResource(FlushReason::kTesting);
viz::ResourceIdGenerator id_generator;
canvas_element().SetOffscreenCanvasResource(std::move(canvas_resource),
id_generator.GenerateNextId());This looks like a leftover from an earlier version of the CL. It shouldn't be needed now that the placeholder canvas directly accesses the offscreen context.
#include "third_party/blink/renderer/core/canvas_interventions/canvas_interventions_enums.h"Remove?
Related details
- John Kim
- 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. |
Tom Van Goethem (Gerrit)
Tom Van Goethem added 11 comments![]( "Open in Gerrit")
I left comments based on the current state of the code, but I however have doubt around the use of the context to propagate the intervention flag.
The placeholder never depended directly on the offscreen canvas. Adding a new dependency between the two could have implications around garbage collection and object destruction order. More importantly however, I think that using the context to propagate the flag opens the door to bugs.
Consider the following:
```C++
offscreen_canvas = canvas.transferControlToOffscreen();
context = offscreen_canvas.getContext('2d');
context.fillText(...); // Taints the context.// Wait for the frame to propagate to the placeholder.
await new Promise(resolve => requestAnimationFrame(resolve));// Reset the context, this removes the taint.
context.reset();// Read back the frame.
// The context is no longer tainted, but the placeholder still holds
// the frame with drawn text.
url = canvas.toDataURL();
```I think that propagating the intervention flag via the `CanvasResource` (what the CL originally did) might be preferable. This is how the origin-clean flag is propagated. It does make sense for a canvas resource to carry the taint because that's what hold the image.
For now I've kept the reference to OffscreenCanvas in HTMLCanvasElement, but also added the taint to CanvasResource and StaticBitmapImage (which will be needed anyway for tainting `drawImage()`). I added an additional check for this in `ShouldApplyNoise`.
I'm considering redesigning some parts of `CanvasInterventionsHelper::MaybeNoiseSnapshot` though; I believe it should be sufficient to check whether there are any triggers (and only create the triggers when these are made in an accelerated context). In that case, no rendering context would be needed. Would it be fine to do that in a follow-up CL?
Member<OffscreenCanvas> controlling_offscreen_canvas_;Logically, this would belong to the `OffscreenCanvasPlaceholder` parent class. Move the attribute that the setter there?
That's a bit tricky. The `OffscreenCanvasPlaceholder` parent class is in `platform/` whereas `OffscreenCanvas` is in `core/` so the former can't depend on the latter. It's also not possible to forward declare `OffscreenCanvas` as that doesn't work with `Trace()`.
I've kept `controlling_offscreen_canvas_` in `HTMLCanvasElement` for now - please let me know if I should find a workaround.
It might be safer to check `controlling_offscreen_canvas_` instead of `HasOffscreenCanvasFrame()`? Both should be equivalent, but checking the pointer offers stronger guaranties that it's valid before we use it.
Done
context = controlling_offscreen_canvas_->RenderingContext();If we move `controlling_offscreen_canvas_` to the parent class (as suggested in other comment), perhaps a adding a getter for `controlling_offscreen_canvas_` or `controlling_offscreen_canvas_ ? controlling_offscreen_canvas_->RenderingContext() : nullptr` in `OffscreenCanvasPlaceholder ` might be useful to centralize and isolate accesses to `controlling_offscreen_canvas_`.
Done
Why not keep using `GetTriggersForIntervention()`, like `CanvasRenderingContext2D::GetCanvasTriggerOperations` originally did, and similarly to how `ShouldTriggerIntervention()` above isn't reading `triggers_for_intervention_` directly either? Reading this attribute directly pulls a member from the parent class, breaking encapsulation.
Done
canvas.SetControllingOffscreenCanvas(offscreen_canvas);I think that this should be moved into `TransferControlToOffscreenInternal`, essentially next to (or leveraging?) `canvas.RegisterPlaceholderCanvas()`. If another call was ever added to `TransferControlToOffscreenInternal`, we wouldn't what the transfer to exclude this new code.
I've moved it into `RegisterPlaceholderCanvas` (and setting back to nullptr in `UnregisterPlaceholderCanvas`).
The lack of line break makes it hard to see the different parts of the tests. Good tests should be structured with a setup, act and expectation section. Maybe add a line break here and another before the first call to `SetCanvasInterventionsForceDisabled` to make these sections stand out better?
Done
auto* script_state = ToScriptStateForMainWorld(GetDocument().GetFrame());Maybe just write `ScriptState*` explicitly? From the style guide:
“Do not use [auto] merely to avoid the inconvenience of writing an explicit type.”
https://google.github.io/styleguide/cppguide.html#Type_deduction
script_state was not needed any more.
auto* script_state = ToScriptStateForMainWorld(GetDocument().GetFrame());
context->setFillStyle(script_state->GetIsolate(),
ToV8Traits<IDLString>::ToV8(script_state, "red"),
exception_state);
context->fillRect(0, 0, 50, 50);What's this for? Wasn't the `fillText` sufficient to trigger the behavior we want?
I added it while debugging things but forgot to remove it. Done.
context->PushFrame();
CanvasResourceProvider* provider = context->GetResourceProviderForCanvas2D();
scoped_refptr<CanvasResource> canvas_resource =
provider->ProduceCanvasResource(FlushReason::kTesting);
viz::ResourceIdGenerator id_generator;
canvas_element().SetOffscreenCanvasResource(std::move(canvas_resource),
id_generator.GenerateNextId());This looks like a leftover from an earlier version of the CL. It shouldn't be needed now that the placeholder canvas directly accesses the offscreen context.
It is needed to get the CanvasResource to the HTMLCanvasElement; without this part the result of `toDataURL()` is a blank image.
#include "third_party/blink/renderer/core/canvas_interventions/canvas_interventions_enums.h"Tom Van GoethemRemove?
Done
Related details
- Jean-Philippe Gravel
- John Kim
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 2 comments![]( "Open in Gerrit")
Tom Van GoethemI left comments based on the current state of the code, but I however have doubt around the use of the context to propagate the intervention flag.
The placeholder never depended directly on the offscreen canvas. Adding a new dependency between the two could have implications around garbage collection and object destruction order. More importantly however, I think that using the context to propagate the flag opens the door to bugs.
Consider the following:
```C++
offscreen_canvas = canvas.transferControlToOffscreen();
context = offscreen_canvas.getContext('2d');
context.fillText(...); // Taints the context.// Wait for the frame to propagate to the placeholder.
await new Promise(resolve => requestAnimationFrame(resolve));// Reset the context, this removes the taint.
context.reset();// Read back the frame.
// The context is no longer tainted, but the placeholder still holds
// the frame with drawn text.
url = canvas.toDataURL();
```I think that propagating the intervention flag via the `CanvasResource` (what the CL originally did) might be preferable. This is how the origin-clean flag is propagated. It does make sense for a canvas resource to carry the taint because that's what hold the image.
For now I've kept the reference to OffscreenCanvas in HTMLCanvasElement, but also added the taint to CanvasResource and StaticBitmapImage (which will be needed anyway for tainting `drawImage()`). I added an additional check for this in `ShouldApplyNoise`.
I'm considering redesigning some parts of `CanvasInterventionsHelper::MaybeNoiseSnapshot` though; I believe it should be sufficient to check whether there are any triggers (and only create the triggers when these are made in an accelerated context). In that case, no rendering context would be needed. Would it be fine to do that in a follow-up CL?
added the taint to CanvasResource and StaticBitmapImage (which will be needed anyway for tainting drawImage()). I added an additional check for this in ShouldApplyNoise.
That works!
For now I've kept the reference to OffscreenCanvas in HTMLCanvasElement
Why keep this code? It seems redundant to the taint in the CanvasResource, but has lower precision. Consider the flip side of the above example:
```c++
offscreen_canvas = canvas.transferControlToOffscreen();
context = offscreen_canvas.getContext('2d');
context.fillRect(...); // Doesn't taint the context.
// Wait for the frame to propagate to the placeholder.
await new Promise(resolve => requestAnimationFrame(resolve));
context.fillText(...); // Taints the context.
// Read back the frame.
// The frame doesn't yet contain the text and doesn't need to be noised.
// The context however is tainted so the frame is incorrectly noised.
url = canvas.toDataURL();
```
context->PushFrame();
CanvasResourceProvider* provider = context->GetResourceProviderForCanvas2D();
scoped_refptr<CanvasResource> canvas_resource =
provider->ProduceCanvasResource(FlushReason::kTesting);
viz::ResourceIdGenerator id_generator;
canvas_element().SetOffscreenCanvasResource(std::move(canvas_resource),
id_generator.GenerateNextId());This is re-implementing a lot of logic from the `OffscreenCanvas` implementation which should already be taken care of. It looks You could replace all of this with:
```c++
platform->RunUntilIdle();
```
Related details
- John Kim
- 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")
context->PushFrame();
CanvasResourceProvider* provider = context->GetResourceProviderForCanvas2D();
scoped_refptr<CanvasResource> canvas_resource =
provider->ProduceCanvasResource(FlushReason::kTesting);
viz::ResourceIdGenerator id_generator;
canvas_element().SetOffscreenCanvasResource(std::move(canvas_resource),
id_generator.GenerateNextId());This is re-implementing a lot of logic from the `OffscreenCanvas` implementation which should already be taken care of. It looks You could replace all of this with:
```c++
platform->RunUntilIdle();
```
Hum. Or maybe not. After further verification, that might not be quite right. Let me investigate some more.
Tom Van Goethem (Gerrit)
Tom Van Goethem added 1 comment![]( "Open in Gerrit")
context->PushFrame();
CanvasResourceProvider* provider = context->GetResourceProviderForCanvas2D();
scoped_refptr<CanvasResource> canvas_resource =
provider->ProduceCanvasResource(FlushReason::kTesting);
viz::ResourceIdGenerator id_generator;
canvas_element().SetOffscreenCanvasResource(std::move(canvas_resource),
id_generator.GenerateNextId());Jean-Philippe GravelThis is re-implementing a lot of logic from the `OffscreenCanvas` implementation which should already be taken care of. It looks You could replace all of this with:
```c++
platform->RunUntilIdle();
```
Hum. Or maybe not. After further verification, that might not be quite right. Let me investigate some more.
Tom Van GoethemOk, I think I figured it out. These classes don't have very clean public API interfaces and boundaries, so figuring out how to properly write test without poking in the internals is tricky. But I think that the following does the trick:
```c++
offscreen_canvas->GetOrCreateResourceDispatcher()->OnBeginFrame(
/*begin_frame_args=*/{}, /*timing details*/ {},
/*resources=*/{});
test::RunPendingTasks();
```But the CL has many other issues:
- The test was drawing the text to position (0, 0), which draws outside the canvas. Thus, no frame could get generated (rendering gets optimized out).
- ShouldApplyNoise needs to be changed to work properly if no context is available (it should use the image snapshot instead).
- Your change in `CanvasResourceSharedImage::Bitmap` was in a `if (!is_accelerated_)` block. You need to also propagate the taint for accelerated images.
See my experiments here:
https://crrev.com/c/6760097(I'm sorry, I forgot to clear the `Change-Id:` in the diffbase of my CL, which means that I accidentally pushed a rebase update on your CL.)
I've done some refactoring already and rebased on top of that. I still need to do some clean up next. I'll let you know when it's ready 👍
Related details
- Jean-Philippe Gravel
- John Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Kim (Gerrit)
John Kim added 3 comments![]( "Open in Gerrit")
canvas_operations |= image_bitmap->GetCanvasTriggerOperations();What if ```image_bitmap->GetCanvasTriggerOperations() == CanvasOperationType::kNone```? This would add kNone to the total operations bitmask. Would this tamper with our metrics? Same with below.
auto canvas_operations = CanvasOperationType::kNone;nit: CanvasOperationType, for consistency with above.
CanvasOperationType triggering_canvas_operations_ =Could we add a docstring for this?
Related details
- Jean-Philippe Gravel
- 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. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
CanvasOperationType GetTriggersForIntervention() const {nit: Per the Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters, this getter should be named `TriggersForIntervention()` since the name does not conflict with a type name and does not use out-arguments.
***
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
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. |
Tom Van Goethem (Gerrit)
Tom Van Goethem added 8 comments![]( "Open in Gerrit")
After the refactoring, we're no longer keeping a reference to OffscreenCanvas.
Hey @vas...@chromium.org, could you please review this CL as JP is OOO this week?
For context: this CL aims to keep track of whether a canvas readback should be noised when the canvas is transferred to offscreen. Specifically, it will keep track of the executed high entropy canvas operations in the canvas resource.
@mk...@chromium.org, could you review audit_non_blink_usage.py?
Thanks!
Member<OffscreenCanvas> controlling_offscreen_canvas_;Tom Van GoethemLogically, this would belong to the `OffscreenCanvasPlaceholder` parent class. Move the attribute that the setter there?
That's a bit tricky. The `OffscreenCanvasPlaceholder` parent class is in `platform/` whereas `OffscreenCanvas` is in `core/` so the former can't depend on the latter. It's also not possible to forward declare `OffscreenCanvas` as that doesn't work with `Trace()`.
I've kept `controlling_offscreen_canvas_` in `HTMLCanvasElement` for now - please let me know if I should find a workaround.
No longer relevant.
canvas_operations |= image_bitmap->GetCanvasTriggerOperations();What if ```image_bitmap->GetCanvasTriggerOperations() == CanvasOperationType::kNone```? This would add kNone to the total operations bitmask. Would this tamper with our metrics? Same with below.
kNone is `0` so adding it doesn't change anything.
nit: CanvasOperationType, for consistency with above.
Removed code.
context->PushFrame();
CanvasResourceProvider* provider = context->GetResourceProviderForCanvas2D();
scoped_refptr<CanvasResource> canvas_resource =
provider->ProduceCanvasResource(FlushReason::kTesting);
viz::ResourceIdGenerator id_generator;
canvas_element().SetOffscreenCanvasResource(std::move(canvas_resource),
id_generator.GenerateNextId());Jean-Philippe GravelThis is re-implementing a lot of logic from the `OffscreenCanvas` implementation which should already be taken care of. It looks You could replace all of this with:
```c++
platform->RunUntilIdle();
```
Jean-Philippe GravelHum. Or maybe not. After further verification, that might not be quite right. Let me investigate some more.
Tom Van GoethemOk, I think I figured it out. These classes don't have very clean public API interfaces and boundaries, so figuring out how to properly write test without poking in the internals is tricky. But I think that the following does the trick:
```c++
offscreen_canvas->GetOrCreateResourceDispatcher()->OnBeginFrame(
/*begin_frame_args=*/{}, /*timing details*/ {},
/*resources=*/{});
test::RunPendingTasks();
```But the CL has many other issues:
- The test was drawing the text to position (0, 0), which draws outside the canvas. Thus, no frame could get generated (rendering gets optimized out).
- ShouldApplyNoise needs to be changed to work properly if no context is available (it should use the image snapshot instead).
- Your change in `CanvasResourceSharedImage::Bitmap` was in a `if (!is_accelerated_)` block. You need to also propagate the taint for accelerated images.
See my experiments here:
https://crrev.com/c/6760097(I'm sorry, I forgot to clear the `Change-Id:` in the diffbase of my CL, which means that I accidentally pushed a rebase update on your CL.)
I've done some refactoring already and rebased on top of that. I still need to do some clean up next. I'll let you know when it's ready 👍
Refactoring was done in the parent CL (crrev.com/c/6765542), and the 3 points mentioned above have been addressed.
CanvasOperationType GetTriggersForIntervention() const {nit: Per the Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters, this getter should be named `TriggersForIntervention()` since the name does not conflict with a type name and does not use out-arguments.
***_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Could we add a docstring for this?
- Jean-Philippe Gravel
- John Kim
- Mike West
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
'third_party/blink/renderer/modules/canvas/',Nit: Please shift this to the specific paths. Especially given that it's a test-only interface, there's no reason to allow it throughout //modules/canvas.
Related details
- Jean-Philippe Gravel
- John Kim
- Tom Van Goethem
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 1 comment![]( "Open in Gerrit")
Related details
- Jean-Philippe Gravel
- John Kim
- 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. |
Tom Van Goethem (Gerrit)
Tom Van Goethem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tom Van Goethem
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Nit: Please shift this to the specific paths. Especially given that it's a test-only interface, there's no reason to allow it throughout //modules/canvas.
- Jean-Philippe Gravel
- John Kim
- Mike West
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Jean-Philippe Gravel
- John Kim
- Tom Van Goethem
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[FPF-CI] Track triggers when canvas is transferred to offscreen
When a canvas is transferred to offscreen, we should ensure that noise
is still applied. To that end, the correct rendering context needs to be
passed when reading back as a blob or with toDataURL.
- M third_party/blink/renderer/modules/BUILD.gn
- M third_party/blink/renderer/modules/canvas/DEPS
- M third_party/blink/renderer/modules/canvas/canvas_noise_test.cc
- A third_party/blink/renderer/modules/canvas/canvas_noise_test_util.cc
- A third_party/blink/renderer/modules/canvas/canvas_noise_test_util.h
- M third_party/blink/renderer/modules/canvas/htmlcanvas/html_canvas_element_module_test.cc
- M third_party/blink/renderer/platform/graphics/canvas_resource.cc
- M third_party/blink/renderer/platform/graphics/canvas_resource.h
- M third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
- M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Code-Review: +1 by Mike West, +1 by Vasiliy Telezhnikov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |