Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a layer and filter interface in the 2D canvas #9537

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

graveljp
Copy link
Contributor

@graveljp graveljp commented Jul 19, 2023

This adds new beginLayer and endLayer functions to open and close layers
in the canvas. While layers are active, draw calls operate on a separate
texture that gets composited to the parent output bitmap when the layer
is closed. An optional filter can be specified in beginLayer, allowing
effects to be applied to the layer's texture when it's composited its
parent.

Fixes #8476

(See WHATWG Working Mode: Changes for more details.)


/acknowledgements.html ( diff )
/canvas.html ( diff )
/index.html ( diff )

Copy link
Member

@Kaiido Kaiido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive work on this.
I still have some concerns about the layer rendering state being picked from the global settings instead of being defined along the filter as a parameter, and about the "when a canvas is presented" concept, but otherwise it all seems to make sense, I think.

Also, I've been a bit quick over the CanvasFilters part as I suppose it's mostly what had already been reviewed on #6763 by Aaron.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
has one, is always just an alias to a <code>canvas</code> element's bitmap. When layers are
opened, implementations must behave as if draw calls operate on a separate <span>output
bitmap</span> that gets composited to the parent <span>output bitmap</span> when the layer is
closed. If the <code>canvas</code> element's bitmap needs to be presented while layers are opened,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"presented" is unfortunately a bit unclear as a concept, should it be clearly defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this has actually caused me some confusion as I was trying to understand how the canvas gets consumed by the event loop. I added a "concept-canvas-presented" definition above to clarify this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.
Hopefully if/when the "update the rendering" steps are rewritten to finally match the implementations we'll be able to add a line about that in there too?
My only remaining concern about this would be about other canvas consumers that don't go through the "check the usability of the image" algo. On the top of my head I can only think of the mediacapture-fromelement (canvas.captureStream()), where they never defined when the canvas frame should be moved to the stream.
I guess having this definition would help them to specify the apparent implementation which seems to be to wait for the next "update the rendering". (c.f. w3c/mediacapture-fromelement#94 and linked).

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved

<div w-nodev>

<p>Before any operations could access the <code>canvas</code> element's bitmap pixels, the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that reading operations do throw if a layer is open, this leaves only the ill-defined "when the canvas is presented" case that could reach it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the intent. But I removed this section. The CanvasRenderingContext2D now has a top level output bitmap and a current output bitmap. The top level output bitmap can always be presented to the user even when layers are opened. When layers are opened, draw operations are performed in a separate bitmap via current output bitmap.

source Outdated
Comment on lines 64618 to 64630
was flushed by running the steps for <dfn>restoring the drawing state stack</dfn>:</p>

<ol>
<li><p><span data-x="list iterate">For each</span> <var>stateEntry</var> of
<var>stateBackup</var>:</p></li>

<ol>
<li><p><span data-x="stack push">Push</span> <var>stateEntry</var> onto the <span>drawing state
stack</span>.</p></li>
</ol>

<li><p>Restore the context's current <span>drawing state</span> by running the <code
data-x="dom-context-2d-restore">restore()</code> method steps.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this doesn't restore any of the bitmaps, right? This means that if we have something like

ctx.globalAlpha = 0.2;
ctx.beginLayer();
ctx.fillRect(0, 0, 50, 50);

The rectangle will keep getting more opaque every time the canvas is "presented". Which is... every frame while it's visible on the page? Or every time the page has to be repainted? Or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section has been removed.

The idea was that when presenting a frame, all the layers would have been closed, the draw calls would have been rasterized, and then, all layers would have been re-opened. This algorithm was describing the logic for re-opening the layers. Only the states were to be re-populated, not the draw calls. On later frame, only new draw calls would get drawn.

This was all replaced with an alternative strategy where unclosed layers are never presented. Instead, only the draw calls up to the first beginLayer() gets rasterized. The content of the layer is preserved for the next frame, where layers can be closed and rasterized as a whole.

source Outdated
Comment on lines 64319 to 64320
<li><p>Set all current <span data-x="drawing state">drawing states</span> to the values they have
in <var>parentDrawingStates</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably exclude the canvas layer state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is tricky to phrase. Let me know what you think.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<span>CanvasFilterInput</span>? <dfn data-x="dom-context-2d-beginLayer-options-filter">filter</dfn> = null;
};

interface mixin <dfn interface>CanvasLayers</dfn> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will/should PaintRenderingContext2D also get this mixin?

Copy link
Contributor Author

@graveljp graveljp Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should probably. Since it's spec'ed in a different place, I can file a ticket to track this separately. Would that make sense?

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated

<li><p>Set all current <span data-x="drawing state">drawing states</span> to the values they have
in <var>previousDrawingStates</var>.</p></li>
</ol>

<p>The <dfn method for="CanvasState"><code data-x="dom-context-2d-reset">reset()</code></dfn>
method steps are to <span>reset the rendering context to its default state</span>.</p>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resetting a context should also close/discard all currently opened layers right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But I think that's already covered in step 3 and 4 below:

"To reset the rendering context to its default state:
....
3 - Clear the context's drawing state stack
4 - Set the context's layer-count to zero."

Isn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that should be done first. Here canvas's bitmap in step 1 may still be an alias to an open layer, and stating clearly what happens to pending layers may be a good thing too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was interpreting "canvas's bitmap" to be the "canvas element's bitmap" (for element canvas), which is different than the "context output bitmap". For instance, in section 4.12.5.1.1, we have:
"The top level output bitmap of a rendering context, when it has one, is always just an alias to a canvas element's bitmap."

But you are right that it whould be made clear that the "context's current output bitmap", which changes when layers are opened, should be restored to point to the canvas element bitmap.

Copy link

@tuankiet65 tuankiet65 Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I interpret step 3 and 4 as: literally clear the drawing state stack and not unwinding the stack (like say, if the drawing state stack is a stack variable, just call some clear() method to clear it), and set the layer-count variable to 0 without unwinding opened layers. I think for clarity purposes, it'd be helpful to explicitly specify it like "close all opened layers", or "while layer count is not zero, perform the steps in endLayer()" or even "discard all opened layers".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, closing all layers is redundant if you are to clear the state stack and reset all states to their default value. In practice, implementations would not close all layers, compositing up all layer outputs and applying filters, just to discard everything in the end. The current spec already uses the step 3 (clear the context's drawing state stack). It's not saying "call restore() in a loop until the state stack is empty.

Maybe I can add a non-normative note saying that these steps have the effect of closing all layers?

Copy link
Member

@Kaiido Kaiido Aug 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be an observable difference though: flushing the layers (or calling endLayer) would update the origin-clean flag of the canvas bitmap, while if you don't something like the below snippet would not taint the canvas:

ctx.beginLayer()
ctx.drawImage(crossOriginImage, x, y)
ctx.reset()

I'm not quite sure what security risks this would involve (since the cross-origin resource would technically never have touched the bitmap), but that would be observable.

(Ps: Testing on the current Canary's implementation the above snippet does taint the canvas, so somehow at least part of the flushing might be done?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a bullet point specifying that the current output bitmap has to be reset to point to the top level output bitmap.

@Kaiido, thanks for pointing out the origin-clean issue. You are correct that closing all layers would give a different result than just resetting the state stack.

The reset() spec should actually say that origin-clean must be set to true. The context does reset the origin-clean flag if the canvas is reset by a size change:

“The flag can be reset in certain situations; for example, when changing the value of the width or the height content attribute of the canvas element to which a CanvasRenderingContext2D is bound, the bitmap is cleared and its origin-clean flag is reset.”
https://html.spec.whatwg.org/multipage/canvas.html#security-with-canvas-elements

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I never noticed that sentence before, but I agree with your reading.
FWIW, this is not interoperable today, only Chrome does reset the flag when changing the canvas size. Can probably be left for another issue.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 13, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 14, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 16, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 18, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 20, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4939633
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Reviewed-by: Fernando Serboncini <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212692}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 20, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4939633
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Reviewed-by: Fernando Serboncini <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212692}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 20, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4939633
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Reviewed-by: Fernando Serboncini <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212692}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 20, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

putImageData however is supposed to write to the canvas bitmap
wholesale, bypassing all render states. This means that we can't write
to the layer's content because the written pixels would then get
filtered when the layer is closed. We can't write to the main canvas
either because these pixels would later be overwritten by the layer's
result with draw calls that potentially happened before (e.g. in the
sequence `beginLayer(); fillRect(); putImageData(); endLayer();`,
`fillRect()` would write over `putImageData()`.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Change-Id: I266a3155c32919a68dbbb093e4aff9b1dd13a3b5
Bug: 1484741
aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 20, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

putImageData however is supposed to write to the canvas bitmap
wholesale, bypassing all render states. This means that we can't write
to the layer's content because the written pixels would then get
filtered when the layer is closed. We can't write to the main canvas
either because these pixels would later be overwritten by the layer's
result with draw calls that potentially happened before (e.g. in the
sequence `beginLayer(); fillRect(); putImageData(); endLayer();`,
`fillRect()` would write over `putImageData()`.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Change-Id: I266a3155c32919a68dbbb093e4aff9b1dd13a3b5
Bug: 1484741
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4943172
Reviewed-by: Fernando Serboncini <[email protected]>
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212741}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 20, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

putImageData however is supposed to write to the canvas bitmap
wholesale, bypassing all render states. This means that we can't write
to the layer's content because the written pixels would then get
filtered when the layer is closed. We can't write to the main canvas
either because these pixels would later be overwritten by the layer's
result with draw calls that potentially happened before (e.g. in the
sequence `beginLayer(); fillRect(); putImageData(); endLayer();`,
`fillRect()` would write over `putImageData()`.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Change-Id: I266a3155c32919a68dbbb093e4aff9b1dd13a3b5
Bug: 1484741
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4943172
Reviewed-by: Fernando Serboncini <[email protected]>
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212741}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 20, 2023
This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

putImageData however is supposed to write to the canvas bitmap
wholesale, bypassing all render states. This means that we can't write
to the layer's content because the written pixels would then get
filtered when the layer is closed. We can't write to the main canvas
either because these pixels would later be overwritten by the layer's
result with draw calls that potentially happened before (e.g. in the
sequence `beginLayer(); fillRect(); putImageData(); endLayer();`,
`fillRect()` would write over `putImageData()`.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Change-Id: I266a3155c32919a68dbbb093e4aff9b1dd13a3b5
Bug: 1484741
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4943172
Reviewed-by: Fernando Serboncini <[email protected]>
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212741}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 1, 2023
…map if canvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in transferToImageBitmap if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4939633
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Reviewed-by: Fernando Serboncini <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212692}

--

wpt-commits: 3375400712353d2c9b011ed3dbb24c8d756b784f
wpt-pr: 42544
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 1, 2023
…nvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in putImageData if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

putImageData however is supposed to write to the canvas bitmap
wholesale, bypassing all render states. This means that we can't write
to the layer's content because the written pixels would then get
filtered when the layer is closed. We can't write to the main canvas
either because these pixels would later be overwritten by the layer's
result with draw calls that potentially happened before (e.g. in the
sequence `beginLayer(); fillRect(); putImageData(); endLayer();`,
`fillRect()` would write over `putImageData()`.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Change-Id: I266a3155c32919a68dbbb093e4aff9b1dd13a3b5
Bug: 1484741
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4943172
Reviewed-by: Fernando Serboncini <[email protected]>
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212741}

--

wpt-commits: 6e6f9fbb0746983001d7fe80ab717567c2f73bfc
wpt-pr: 42662
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Nov 2, 2023
…map if canvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in transferToImageBitmap if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4939633
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Reviewed-by: Fernando Serboncini <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212692}

--

wpt-commits: 3375400712353d2c9b011ed3dbb24c8d756b784f
wpt-pr: 42544
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Nov 2, 2023
…nvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in putImageData if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

putImageData however is supposed to write to the canvas bitmap
wholesale, bypassing all render states. This means that we can't write
to the layer's content because the written pixels would then get
filtered when the layer is closed. We can't write to the main canvas
either because these pixels would later be overwritten by the layer's
result with draw calls that potentially happened before (e.g. in the
sequence `beginLayer(); fillRect(); putImageData(); endLayer();`,
`fillRect()` would write over `putImageData()`.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Change-Id: I266a3155c32919a68dbbb093e4aff9b1dd13a3b5
Bug: 1484741
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4943172
Reviewed-by: Fernando Serboncini <[email protected]>
Commit-Queue: Jean-Philippe Gravel <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1212741}

--

wpt-commits: 6e6f9fbb0746983001d7fe80ab717567c2f73bfc
wpt-pr: 42662
Copy link
Contributor Author

@graveljp graveljp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I uploaded a commit addressing all review comments. I also made these important changes:

  • beginLayer now accepts CSS filter strings.

  • The CanvasRenderingContext2D now has a "top level output bitmap" and a "current output bitmap". The "top level output bitmap" can always be presented to the user, even when layers are open. The content of layers only gets rasterized to the "top level output bitmap" when all layers are closed. This is done by having layers replace and restore the "current output bitmap" to which context draws.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<span>CanvasFilterInput</span>? <dfn data-x="dom-context-2d-beginLayer-options-filter">filter</dfn> = null;
};

interface mixin <dfn interface>CanvasLayers</dfn> {
Copy link
Contributor Author

@graveljp graveljp Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should probably. Since it's spec'ed in a different place, I can file a ticket to track this separately. Would that make sense?

source Outdated Show resolved Hide resolved
source Outdated
keeping track of the number of opened nested layers. To access the content of the context's
<span>output bitmap</span>, the <span>steps for reading the context output bitmap</span> must be
used.

<p>The <span>output bitmap</span> has an <span
data-x="concept-canvas-origin-clean">origin-clean</span> flag, which can be set to true or false.
Initially, when one of these bitmaps is created, its <span
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layers behave the same as other draw calls for {alpha: false} contexts. The layer content and the layer itself is alpha-blended up to the point where it gets written to the top level output bitmap. I updated the paragraph below to clarify this, and added WPT tests validating this (https://crrev.com/c/5006000).

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
Comment on lines 64618 to 64630
was flushed by running the steps for <dfn>restoring the drawing state stack</dfn>:</p>

<ol>
<li><p><span data-x="list iterate">For each</span> <var>stateEntry</var> of
<var>stateBackup</var>:</p></li>

<ol>
<li><p><span data-x="stack push">Push</span> <var>stateEntry</var> onto the <span>drawing state
stack</span>.</p></li>
</ol>

<li><p>Restore the context's current <span>drawing state</span> by running the <code
data-x="dom-context-2d-restore">restore()</code> method steps.</p></li>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section has been removed.

The idea was that when presenting a frame, all the layers would have been closed, the draw calls would have been rasterized, and then, all layers would have been re-opened. This algorithm was describing the logic for re-opening the layers. Only the states were to be re-populated, not the draw calls. On later frame, only new draw calls would get drawn.

This was all replaced with an alternative strategy where unclosed layers are never presented. Instead, only the draw calls up to the first beginLayer() gets rasterized. The content of the layer is preserved for the next frame, where layers can be closed and rasterized as a whole.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 8, 2023
…map if canvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in transferToImageBitmap if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4939633
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Reviewed-by: Fernando Serboncini <fserbchromium.org>
Cr-Commit-Position: refs/heads/main{#1212692}

--

wpt-commits: 3375400712353d2c9b011ed3dbb24c8d756b784f
wpt-pr: 42544

UltraBlame original commit: 5dcacd6ceadca3d78dab262fdd726b9646edbe7e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 8, 2023
…nvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in putImageData if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

putImageData however is supposed to write to the canvas bitmap
wholesale, bypassing all render states. This means that we can't write
to the layer's content because the written pixels would then get
filtered when the layer is closed. We can't write to the main canvas
either because these pixels would later be overwritten by the layer's
result with draw calls that potentially happened before (e.g. in the
sequence `beginLayer(); fillRect(); putImageData(); endLayer();`,
`fillRect()` would write over `putImageData()`.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Change-Id: I266a3155c32919a68dbbb093e4aff9b1dd13a3b5
Bug: 1484741
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4943172
Reviewed-by: Fernando Serboncini <fserbchromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Cr-Commit-Position: refs/heads/main{#1212741}

--

wpt-commits: 6e6f9fbb0746983001d7fe80ab717567c2f73bfc
wpt-pr: 42662

UltraBlame original commit: 685e2ff9c9f305fea2dfd442a95067594d05a6d1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 8, 2023
…map if canvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in transferToImageBitmap if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4939633
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Reviewed-by: Fernando Serboncini <fserbchromium.org>
Cr-Commit-Position: refs/heads/main{#1212692}

--

wpt-commits: 3375400712353d2c9b011ed3dbb24c8d756b784f
wpt-pr: 42544

UltraBlame original commit: 5dcacd6ceadca3d78dab262fdd726b9646edbe7e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 8, 2023
…nvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in putImageData if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

putImageData however is supposed to write to the canvas bitmap
wholesale, bypassing all render states. This means that we can't write
to the layer's content because the written pixels would then get
filtered when the layer is closed. We can't write to the main canvas
either because these pixels would later be overwritten by the layer's
result with draw calls that potentially happened before (e.g. in the
sequence `beginLayer(); fillRect(); putImageData(); endLayer();`,
`fillRect()` would write over `putImageData()`.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Change-Id: I266a3155c32919a68dbbb093e4aff9b1dd13a3b5
Bug: 1484741
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4943172
Reviewed-by: Fernando Serboncini <fserbchromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Cr-Commit-Position: refs/heads/main{#1212741}

--

wpt-commits: 6e6f9fbb0746983001d7fe80ab717567c2f73bfc
wpt-pr: 42662

UltraBlame original commit: 685e2ff9c9f305fea2dfd442a95067594d05a6d1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 8, 2023
…map if canvas layers are opened, a=testonly

Automatic update from web-platform-tests
Throw an exception in transferToImageBitmap if canvas layers are opened

This API is incompatible with how the 2D canvas is rasterized when
it contains unclosed layers. Because layers can have filters that get
applied on their final content, they can't be presented until they are
closed. Instead, we normally keep the layer content alive after a
flush, so that it can be presented in a later frame when the layer is
finally closed.

OffscreenCanvas.transferToImageBitmap however is supposed to release
the canvas content, leaving the offscreen canvas empty. We cannot
release the recording if layers are incomplete, and if we kept the
layer content alive for later, we would not be leaving the canvas
empty as the spec requires.

This behavior is part of the current 2D Canvas Layer spec draft:
Explainer: https://github.com/fserb/canvas2D/blob/master/spec/layers.md
Spec draft: whatwg/html#9537

Bug: 1484741
Change-Id: Ic770b51a0343faf0b2c7477624d69f59187ce97f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4939633
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Reviewed-by: Fernando Serboncini <fserbchromium.org>
Cr-Commit-Position: refs/heads/main{#1212692}

--

wpt-commits: 3375400712353d2c9b011ed3dbb24c8d756b784f
wpt-pr: 42544

UltraBlame original commit: 5dcacd6ceadca3d78dab262fdd726b9646edbe7e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 15, 2024
…ons filters tentative, a=testonly

Automatic update from web-platform-tests
Make WPT tests validating BeginLayerOptions filters tentative

The beginLayer's BeginLayerOption parameter was removed from the canvas
layer PR at whatwg/html#9537. We might pursue
this as a followup. In the meantime, the tests validating filters
specified via BeginLayerOptions should be marked as tentative.

Bug: 40249439
Change-Id: I3d9a009abc04658395b403a8752cf065644fc7f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5903952
Reviewed-by: Andres Ricardo Perez <andresrperezchromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Cr-Commit-Position: refs/heads/main{#1364414}

--

wpt-commits: 13948ad5eb4121e95d7137e54d666eafe413e72f
wpt-pr: 48477

UltraBlame original commit: b6e8051aa3fb22c636d6ae835976f2b791569b08
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 15, 2024
…tions tentative, a=testonly

Automatic update from web-platform-tests
Make WPT tests validating layers with options tentative

The beginLayer's BeginLayerOption parameter was removed from the
canvas layer PR at whatwg/html#9537. We might
pursue this as a followup. In the meantime, the tests validating
BeginLayerOptions should be marked as tentative.

To prevent losing test coverage, this CL adds test variants
using context filters in addition to layer filters.

Bug: 40249439
Change-Id: Ic06dfa911969075bee42a20f0518beab2bc98fa7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5904350
Reviewed-by: Andres Ricardo Perez <andresrperezchromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Cr-Commit-Position: refs/heads/main{#1365604}

--

wpt-commits: bf7c101faf285e73a2e35a8a4ae26e3d805b9548
wpt-pr: 48522

UltraBlame original commit: 5d5bbdf6ce6d994eb36ab954ece44a5c3aa0d3e4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 15, 2024
…ons filters tentative, a=testonly

Automatic update from web-platform-tests
Make WPT tests validating BeginLayerOptions filters tentative

The beginLayer's BeginLayerOption parameter was removed from the canvas
layer PR at whatwg/html#9537. We might pursue
this as a followup. In the meantime, the tests validating filters
specified via BeginLayerOptions should be marked as tentative.

Bug: 40249439
Change-Id: I3d9a009abc04658395b403a8752cf065644fc7f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5903952
Reviewed-by: Andres Ricardo Perez <andresrperezchromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Cr-Commit-Position: refs/heads/main{#1364414}

--

wpt-commits: 13948ad5eb4121e95d7137e54d666eafe413e72f
wpt-pr: 48477

UltraBlame original commit: b6e8051aa3fb22c636d6ae835976f2b791569b08
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 15, 2024
…tions tentative, a=testonly

Automatic update from web-platform-tests
Make WPT tests validating layers with options tentative

The beginLayer's BeginLayerOption parameter was removed from the
canvas layer PR at whatwg/html#9537. We might
pursue this as a followup. In the meantime, the tests validating
BeginLayerOptions should be marked as tentative.

To prevent losing test coverage, this CL adds test variants
using context filters in addition to layer filters.

Bug: 40249439
Change-Id: Ic06dfa911969075bee42a20f0518beab2bc98fa7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5904350
Reviewed-by: Andres Ricardo Perez <andresrperezchromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Cr-Commit-Position: refs/heads/main{#1365604}

--

wpt-commits: bf7c101faf285e73a2e35a8a4ae26e3d805b9548
wpt-pr: 48522

UltraBlame original commit: 5d5bbdf6ce6d994eb36ab954ece44a5c3aa0d3e4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 15, 2024
…ons filters tentative, a=testonly

Automatic update from web-platform-tests
Make WPT tests validating BeginLayerOptions filters tentative

The beginLayer's BeginLayerOption parameter was removed from the canvas
layer PR at whatwg/html#9537. We might pursue
this as a followup. In the meantime, the tests validating filters
specified via BeginLayerOptions should be marked as tentative.

Bug: 40249439
Change-Id: I3d9a009abc04658395b403a8752cf065644fc7f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5903952
Reviewed-by: Andres Ricardo Perez <andresrperezchromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Cr-Commit-Position: refs/heads/main{#1364414}

--

wpt-commits: 13948ad5eb4121e95d7137e54d666eafe413e72f
wpt-pr: 48477

UltraBlame original commit: b6e8051aa3fb22c636d6ae835976f2b791569b08
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 15, 2024
…tions tentative, a=testonly

Automatic update from web-platform-tests
Make WPT tests validating layers with options tentative

The beginLayer's BeginLayerOption parameter was removed from the
canvas layer PR at whatwg/html#9537. We might
pursue this as a followup. In the meantime, the tests validating
BeginLayerOptions should be marked as tentative.

To prevent losing test coverage, this CL adds test variants
using context filters in addition to layer filters.

Bug: 40249439
Change-Id: Ic06dfa911969075bee42a20f0518beab2bc98fa7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5904350
Reviewed-by: Andres Ricardo Perez <andresrperezchromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravelchromium.org>
Cr-Commit-Position: refs/heads/main{#1365604}

--

wpt-commits: bf7c101faf285e73a2e35a8a4ae26e3d805b9548
wpt-pr: 48522

UltraBlame original commit: 5d5bbdf6ce6d994eb36ab954ece44a5c3aa0d3e4
@annevk
Copy link
Member

annevk commented Nov 12, 2024

This needs rebasing, but it's also not entirely clear to me this has support from WebKit. After double checking colleagues we see value in the CSS-based mechanism to specify filters, but not the XML-derived object model. We also gave this feedback last year in the corresponding issue.

@graveljp
Copy link
Contributor Author

we see value in the CSS-based mechanism to specify filters, but not the XML-derived object model

This has been addressed already. The XML filter support was dropped in #9537 (comment), in favor of supporting only CSS filters. After receiving more feedback from WebKit, I then dropped the layer's filter parameter altogether, in favor of using the context filter. See #9537 (comment).

@annevk
Copy link
Member

annevk commented Nov 12, 2024

My apologies, I was led to believe that feedback was still unaddressed. If you are willing to resolve the branch conflicts I can take care of review by Nov 22 (aiming for early next week). Hopefully @Kaiido can also make some time as they often have useful insights.

graveljp and others added 12 commits November 14, 2024 18:06
This adds new beginLayer and endLayer functions to open and close layers
in the canvas. While layers are active, draw calls operate on a separate
texture that gets composited to the parent output bitmap when the layer
is closed. An optional filter can be specified in beginLayer, allowing
effects to be applied to the layer's texture when it's composited its
parent.

Tests:
 https://github.com/web-platform-tests/wpt/tree/master/html/canvas/element/layers
 https://github.com/web-platform-tests/wpt/tree/master/html/canvas/offscreen/layers

Fixes whatwg#8476
This update addresses 3 main things:
- Support for CSS filter strings was added to the beginLayer API
- Unclosed layers are now never rasterized when the canvas is presented
  to the user. Instead, the content of the layer is preserved and will
  be rasterized in a later frame, if/when the layer is closed.
- Replied to the first round of review comments.
This will be moved to a separate pull request.
With this change, the context filters now applies to the layer's output
bitmap and is resetted to "none" when entering layers.
This removes the filter argument of the beginLayer API.
BeginLayerOptions can possibliy be added to the specification as a
follow-up.
The HTMLCanvasElement's bitmap was linkified to clarify to which bitmap
each uses of the word "bitmap" refers to (either the element's bitmap,
the context's top level output bitmap or the context's current output
bitmap).
This means that when entering a layer, the current transform now is
reset to the identity matrix. setTransform and getTransform are now
local to the current layer, meaning that calling setTransform sets a
matrix relative to the parent layer.
With how the spec is written now, the context always has a top level and
a current output bitmap, irrespective of how the context's bitmap is
bound or used by an HTMLCanvasElement or OffscreenCanvas. Thus, the
paragraph modified by this commit wasn't correct. It's more useful to
describe how the state stack as a whole relates with the context's
state.
Most uses were plural already. Making them all plural is more consistent
and allows to remove the data-x that was needed to reconcile the
differences.
@graveljp
Copy link
Contributor Author

I rebased the PR and added a few minor fixes.

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
<p>Objects that implement the <code>CanvasState</code> interface maintain a stack of drawing
states. <dfn data-x="drawing state">Drawing states</dfn> consist of:</p>
<p>Objects that implement the <code>CanvasState</code> interface maintain a <dfn data-x="drawing
states stack">stack of drawing states</dfn>. <dfn>Drawing states</dfn> consist of:</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By removing data-x you have changed the ID as well. I guess removing data-x is okay, but then you need to explicitly state the ID so it does not get changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by removing data-x while explicitly stating the ID? Isn't the ID specified with a data-x?

For context, I was changing the ID to make it plural because most uses of drawing states was plural already and the singular uses felt inconsistent. I could not consider drawing state as a structure (which would have had a singular name) because it was defined as a list of current context states. drawing states was conflating current context states and states we save in the state stack. Thus, I opted for the plural form, not as a structure name but more like a list of related concepts invoked in different places of the spec.

I now updated this section to try to alleviate the confusion. I separated the concept of a drawing state struct that we save in the state stack from the set of current states the context operates on. Drawing state now contains "A clipping region", as opposed to "The current clipping region". The context now has a current drawing state (which indirectly implies it has a current clipping region). I think that this is much cleaner. But it caused trouble with the current transformation matrix. If the drawing state contains a current transformation matrix, then the context would be using a current current transformation matrix. To address this, I renamed this matrix to layer transformation matrix, that is, it's the transformation matrix active in a layer. I originally wanted to name it like that but didn't only because I was trying to minimize the scope of the change (renaming the current transformation matrix might have ripple effects, like having to update the MDN documentation to refer to the current layer's transformation matrix for instance).

What do you think of these changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, data-x specifies the cross-reference term. The ID is derived from that, but can also be set independently through the id attribute.

These changes sound good. In general the 2D canvas section has been much too hand-wavy. And in general I wouldn't worry too much about implications for everything that is downstream from the specification, except for incoming links. The most important thing is that we describe the model well, as everything else flows from that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. I wasn't aware we could control the id separately from the data-x. So, we would want to preserve the ID untouched to prevent breaking external links to the spec?

The spec has many dfns not specifying a data-x, like <dfn>output bitmap</dfn>. From a previous discussion with @domenic, I understood that these IDs are internal to the spec. I assume that changing these wouldn't require that we preserve the original ID? Isn't the "drawing state" ID falling in that category?

I'm asking because I've added a couple of dfns in this PR and I opted for the more readable no data-x format. This results in IDs that that are not specific to the canvas at all, for instance, "current output bitmap", "total transformation matrix" or "transformation". I was working with the assumption that we can change these later if they ever conflict with another part of the spec. Is this not the case? Should I instead name these new definitions with a canvas-specific ID prefix?

Regarding the question around "drawing state", my previous update reverted the ID to its previous value, so that case is OK for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IDs are indeed internal to the specification, but people will come to depend on them as they are part of its public API if you will. It doesn't really matter what the ID ends up being as long as it continues to refer to the same concept. So if we rename any of these terms in the future, ideally we keep the ID the same. So say we could rename "output bitmap" to "display bitmap" so we can use "output bitmap" for something else elsewhere using <dfn id=output-bitmap>display bitmap</dfn> and that would be fine.

So yeah, it's fine to introduce <dfn>s without data-x. We just have to be mindful when changing an ID-less <dfn>'s data-x or its contents when it lacks a data-x so the ID remains the same.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
<li><p>Draw <var>layerOutputBitmap</var> onto <span>this</span>'s <span>current output
bitmap</span> using the steps outlined in the <span>drawing model</span>.</p></li>

<li><p>Decrement <span>this</span>'s <span data-x="concept-canvas-layer-count">layer count</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be concept-context-2d-layer-count here and elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind changing this, but note that I used concept-canvas because lots of other nearby dfns use that prefix: concept-canvas-origin-clean, concept-canvas-global-alpha, concept-canvas-context-mode, etc. On the other hand, not a single dfn uses concept-context-2d prefix anywhere in the spec.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can continue to use canvas then, but it seems a bit confusing that it houses both canvas element and 2d context concepts.

source Show resolved Hide resolved
Main changes include clarifications in the `drawing state` definition
and renaming `current transformation matrix` to `layer transformation
matrix`.
Copy link
Contributor Author

@graveljp graveljp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

source Show resolved Hide resolved
source Show resolved Hide resolved
<p>Objects that implement the <code>CanvasState</code> interface maintain a stack of drawing
states. <dfn data-x="drawing state">Drawing states</dfn> consist of:</p>
<p>Objects that implement the <code>CanvasState</code> interface maintain a <dfn data-x="drawing
states stack">stack of drawing states</dfn>. <dfn>Drawing states</dfn> consist of:</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by removing data-x while explicitly stating the ID? Isn't the ID specified with a data-x?

For context, I was changing the ID to make it plural because most uses of drawing states was plural already and the singular uses felt inconsistent. I could not consider drawing state as a structure (which would have had a singular name) because it was defined as a list of current context states. drawing states was conflating current context states and states we save in the state stack. Thus, I opted for the plural form, not as a structure name but more like a list of related concepts invoked in different places of the spec.

I now updated this section to try to alleviate the confusion. I separated the concept of a drawing state struct that we save in the state stack from the set of current states the context operates on. Drawing state now contains "A clipping region", as opposed to "The current clipping region". The context now has a current drawing state (which indirectly implies it has a current clipping region). I think that this is much cleaner. But it caused trouble with the current transformation matrix. If the drawing state contains a current transformation matrix, then the context would be using a current current transformation matrix. To address this, I renamed this matrix to layer transformation matrix, that is, it's the transformation matrix active in a layer. I originally wanted to name it like that but didn't only because I was trying to minimize the scope of the change (renaming the current transformation matrix might have ripple effects, like having to update the MDN documentation to refer to the current layer's transformation matrix for instance).

What do you think of these changes?

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
<li><p>Draw <var>layerOutputBitmap</var> onto <span>this</span>'s <span>current output
bitmap</span> using the steps outlined in the <span>drawing model</span>.</p></li>

<li><p>Decrement <span>this</span>'s <span data-x="concept-canvas-layer-count">layer count</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind changing this, but note that I used concept-canvas because lots of other nearby dfns use that prefix: concept-canvas-origin-clean, concept-canvas-global-alpha, concept-canvas-context-mode, etc. On the other hand, not a single dfn uses concept-context-2d prefix anywhere in the spec.

Thoughts?

source Show resolved Hide resolved

<p>The user agent must use a square pixel density consisting of one pixel of image data per
coordinate space unit for the bitmaps of a <code>canvas</code> and its rendering contexts.</p>
coordinate space unit for the <span data-x="canvas-bitmap">bitmaps</span> of a <code>canvas</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A canvas only has a single [canvas-]bitmap, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I suppose that the plural made sense given that here, "bitmaps" referred to the bitmap of the canvas and the contexts. By adding the link though, it's incorrect to associate canvas-bitmap with the contexts. I'm not sure what the best solution here. I went with: "the canvas element's canvas-bitmap and the bitmaps of its rendering context". I made the second "bitmaps" plural because contexts can now have multiple bitmaps, one per layer. I made "context" singular because a canvas can only have one context at a time. The second "bitmaps" isn't linkified because different context types have different bitmap IDs (e.g. "top-level output bitmap", "offscreencanvas-bitmap", "concept-ImageBitmapRenderingContext-output-bitmap")

An alternative here could be to omit the context part altogether because the context bitmaps are defined as being an alias to the element bitmap.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If just referencing the canvas bitmap is sufficient I guess I'd go with that.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved

<li><p>Let <var>layerOutputBitmap</var> be a newly created <span>output bitmap</span>,
initialized with an <span data-x="concept-canvas-origin-clean">origin-clean</span> flag set to
true.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be sure, these layers are completely isolated and cannot impact each other? But also, do we want to maintain this state on a per-bitmap basis or should this instead be done at a higher-level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be sure, these layers are completely isolated and cannot impact each other?

They are.

But also, do we want to maintain this state on a per-bitmap basis or should this instead be done at a higher-level?

The spec was somewhat hand-wavy about how the origin-clean flag works. It was mostly written with the idea that it's hierarchical though:

  • The Canvas element bitmaps and the context bitmaps are first defined as having an origin-clean flag:

The bitmaps of canvas elements, the bitmaps of ImageBitmap objects, as well as some of the bitmaps of rendering contexts [...], have an origin-clean flag.

From here, the original spec is a bit broken:

  • When a CanvasPattern is used as fillStyle or strokeStyle, the context itself gets tainted by the pattern. Same for drawImage(). Here, the spec should actually refer to the context's bitmap's origin-clean flag.
  • Then, the original spec doesn't really explain how the context's origin-clean flag ties back to the canvas element, to close the loop.

For this PR, I felt it was natural to build on this hierarchical design. With this PR, we have:

  • Each layers have their own output bitmap, and each output bitmap has an origin-clean flag.
  • When a CanvasPattern is used as fillStyle/strokeStyle, or if a drawImage is called, the current layer's output bitmap's origin-clean gets tainted by the image source.
  • When closing a layer, the layer's origin-clean flag taints the parent's.
  • The origin-clean taints propagates from layers to their parent, all the way to the top-level output bitmap.
  • The top-level output bitmap is defined as an alias of the canvas element's bitmap, closing the loop.

I suppose that the alternative to this would be to get rid of the origin-clean flag of the context's output bitmap and instead have drawImage, fillStyle and strokeStyle taint the canvas element's output bitmap's origin-clean flag directly. But this would mean that some bitmaps are taintable (the canvas bitmap) but some are not (the context's). This feels a bit weird to me.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the question is really what objects need to hold this state. I don't think we ever pass a rendering context as a standalone entity around, but we do pass the canvas element or ImageBitmap objects around and then we care about the state of this bit. So instead of bitmaps holding this state I'd probably put it directly on canvas elements, OffscreenCanvas objects, etc. It seems that would make the overall setup a little less fragile with less state to forward and a centralized place of authority.

If we spread the bits around as this PR does people might think that has some kind of meaning and infer grand theories from what is an inefficient design (as far as I can tell).

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@Kaiido Kaiido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me so long.

I think the transform matrix model works well, with one caveat being getTransform() (see below comment).

Also, I noticed that the current Chromium implementation does handle canvas.captureStream() differently than what is presented to the page, not presenting the open layers content at all. (https://jsfiddle.net/djvnhagu/) I don't think here is the place where this should be handled but maybe it should be tested anyway?

source Show resolved Hide resolved
arguments as described below.</p>
</dd>

<dt><code data-x=""><var>matrix</var> = <var>context</var>.<span subdfn data-x="dom-context-2d-getTransform">getTransform</span>()</code></dt>

<dd>
<p>Returns a copy of the <span>current transformation matrix</span>, as a newly created
<p>Returns a copy of the <span>layer transformation matrix</span>, as a newly created
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests and implementation use the total transform here.
But that's actually something I'm not quite sure about: Both might be needed depending on the use case... E.G. hit testing would need the global matrix, relative transform would require the local one. Maybe new methods are needed? Or an option to getTransform?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests and implementation use the total transform here.

You are correct! I still havent updated Chromium to follow this new version of the spec. I encountered some technical debt along the way and need to find the time to complete this change. Then, I was hoping to have a functional reference implementation to update the WPT test. I suppose I could update the tests beforehand, but I wouldn't be able to validate them.

But that's actually something I'm not quite sure about: Both might be needed depending on the use case... E.G. hit testing would need the global matrix, relative transform would require the local one. Maybe new methods are needed? Or an option to getTransform?

This is an interesting observation. What do you mean by hit testing here? The methods isPointInPath() and isPointInStroke() should work correctly since they are spec'ed as operating on the top-level coordinate frame (the x and y coordinates are "treated as coordinates in the canvas coordinate space unaffected by transformations"). Do you mean that web pages wouldn't be able to do hit testing manually inside a layer by getting the transform and manually transforming coordinates? Are there use cases where isPointInPath() would't be enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I indeed mean hit testing made by the page authors. There are many cases where it's more efficient to do it yourself, using simple trigonometry rather than using the builtins, but it also may be to e.g. calculate a direction set by the pointer. The object defining the origin might be in a layer and they'd need to be able to convert the global pointer event's coords to the layer's matrix.

source Outdated
Comment on lines 68405 to 68407
<p>If this <var>image</var>'s rendering context's <span
data-x="concept-canvas-layer-count">layer count</span> different than zero, then throw an
<span>"<code>InvalidStateError</code>"</span> <code>DOMException</code>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe "this" in "this image" is a bit misleading here as it could be read to this, and there is an "is" missing.
Also given the other comments from Anne, I guess it should be "is not 0" instead of "different than zero".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Interestingly, there are several other instances of "this" being used this way. But I see that the paragraph above says: "If image's readyState ...", so I suppose I could say "If image's rendering context's layer count ...".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Canvas2D layers with filter support
7 participants