Re: [css-paint-api] Spec feedback

Hi Benoit, thanks for taking the time to review.

On Fri, Aug 21, 2015 at 5:51 PM, Benoit Girard <bgirard@mozilla.com> wrote:

a) The spec says 'paint callbacks should be idempotent'. Should this
feature allow for, say time based, animated properties? Is Date accessible
from the PaintGlobalScope? This would interact with implementations such as
immediate tile based painting where depending on the time API used it could
drift between the first tile paint and the last. Currently the 'Paint
Invalidation' section doesn't allow this but it might be a concern for
forward compatibility.


The paint callbacks are called every time the properties they've registered
as input change. This means that you can use CSS Animations or Web
Animations to animate custom paint, or just register a custom time-value
property and modify this from script during rAF.

Date is accessible from PaintGlobalScope, but the idempotency
recommendation means that authors shouldn't use it.


b) What happens if you registerPaint more than once with the given name?
Does it replace or do the callbacks stack and if so the behavior should be
specified.


Similar to registerApply in the properties and values specification[1],
this will throw an error. Added Issue.[2] If we allow stacking here we’ll
suddenly need to provide an API for reordering callbacks which I think is
unnecessary complexity. If the author needs this type of stacking API this
would be trivial to implement on top of registerPaint.

c) I agree with issue #5, better partial-invalidation (from both side) I
think would be valuable. With immediate tile based rendering it would
useful for the user agent to tell large paint callback that it's only
interested in updating a subrect.


I'm now erring on the side that this isn't as useful, primarily from a
compatibility point of view.

For UAs that are DisplayList based, would simply require the whole area to
be re-painted. If an author created two different implementations based on
if the UA partially invalidated or not, then there could be implementation
bugs between them which would lead to compat issues.

If other UAs felt strongly about this, I feel it would be better as an
optional "advanced" feature that authors would have to explicitly opt-in to.

d) What assumptions can the paint callback make about the canvas and what
assumptions should it explicitly avoid making? For instance when doing
immediate tile based rendering, say 256^2 tiles, of a large fragment say
1024^2 the user agent may want to fire 4 callbacks for each tile. Can the
paintCallback make any assumptions about the canvas width/height,
transformation and clip? This is closely related to partial-invalidation.


This sort of highlights why I think partial-invalidation might be a bad
idea. An author may hard-code their math to assuming 256-tiling, then be
surprised when a UA changes their tiling scheme or behaviour.

To the question at hand, I would expect that the author is given the whole
canvas, and additional data that indications the region the author needs to
write to. If they write to more, the UA is allowed to throw that additional
data away.


e) Reading the CSS properties as DOMString doesn't seem ideal. Imagine
--my-property-options: <transform-list> | ... where you could transform the
results in 2.5D, you would have to re-implement the CSS parsing for
<transform-list>.


We’ve proposed working towards a better OM for CSS (typed JS objects
representing CSS values) [3]. It would make sense to use that API here.


f) Related to Issue #7, a contextType can be provided but PaintCallback
param 1 is of type CanvasRenderingContext2d.


Thanks. My gut feeling here is to remove contextType and just provide the
rendering context. If we add another context at a later stage it should be
trivial to change the IDL to a union type. Thoughts?

g) What should the paint callback do when it encounters an error parsing
the dependent css properties? The spec could say that if a paint callback
throws a javascript exception with an error message then the user agent can
report it. I'd imagine it would be useful if the paint callback can easily
bubble up to the developer tools a parsing error with a particular
dependent CSS property similar to how dev tools currently highlight errors
like 'width: SomeText'.


This hopefully won’t be an issue if we have a typed OM.

h) Similarly what should the user agent do if there was an exception thrown
during the paint callback unrelated to parsing. I'd imagine that the user
agent can simply present the rendering context with whichever operations
were completed before the error.


Added issue. [4] I think that hard failing and presenting no data would be
the better option here, but would like to see what the wider group thinks.

i) Naming nit: Shouldn't hookID be called hookHandle or
cssPaintRequestHandle?


Added Issue. [5]

j) PaintCallback is a callback. It looks like it would have access to the
outer scopes. Web workers specify a script URL where it's clear that they
don't have access to the outer scope. Should it match web workers?


registerPaint is on the PaintGlobalScope. (See the CSS Script API thread I
wrote [6]). Specifically this would mean that all paint functions run in a
separate execution context, similar to a worker, and scripts which register
paint functions are loaded from the main execution context. For example:

// Main execution context
window.paintExecutionContext.importScripts('my-painter.js');

// Paint execution context
registerPaint({name: 'paint1', ... });
registerPaint({name: 'paint2', ... });

This mean that paint callbacks wouldn't have access to the DOM, Storage,
etc APIs. Blink would not cope well with a execution context per callback
resulting in large memory usage at the moment. I suspect other JS engines
would behave similar but would love feedback regarding this.

The Script API explicitly leaves out the number of execution contexts per
phase, so if an engine would like to parallelize an run multiple contexts
per phase, they can.

k) Should the CanvasRenderingContext2d be considered write-only, similar to
'tainted' canvas. This makes both immediate rendering simpler. It also
makes deferred rendering simpler because the user agent might be equipped
to issue the rendering commands in that thread/process so it couldn't
easily provide the pixels.


Completely agree. This is why "X .... will be no-ops" is in the spec.
Specifying it as a 'tainted' canvas could be better.

That said, I believe that if we created a new interface
("WriteOnlyCanvasRenderingContext2d") with only the 'write' methods this
would be clearer to developers what was happening. An API missing is better
than an API that always produces errors. This is issue #9 in the spec.

Would the group be in support of a special "write-only" RenderingContext
for this purpose?

l) The PaintCallback can hold a reference to the CanvasRenderingContext2d
which can cause some headaches for the implementation. For example the
canvas buffer could be a reference into temporarily mapped GPU memory where
the user agent can't hold a reference. What happens if the previous
rendering context is accessed from a future invocation PaintCallback by
maintaining a reference? I see that getImageData is now a no-op. The spec
currently says that the state is not transferred to the rendering context
on the next paintcallback but it might be a different context. What happens
to the context if you keep a strong reference to it. Can the PaintCallback
maintain state at all?


So as you pointed out, if the rendering context is 'tainted' or
'write-only' then getImageData either won’t exist or throw. If a callback
retains a strong reference to the rendering context, it should be
effectively "dead" the next time the callback is invoked. You could call
methods on it, but these would do nothing.

This is why the spec says: "The paint callback is passed a new
CanvasRenderingContext2d each time it is invoked."

m) Are each PaintGlobalScope independent? Can information be shared between
PaintCallbacks in any way like postMessage? This can have implication for
the invalidation.


No, the PaintGlobalScope is shared between paint callbacks. This implies
that they can share data by storing data on the GlobalScope or similar
shared objects like Array. We need to encourage authors to write their
callbacks as "idempotent" as possible.

As I mentioned above, for V8 creating a new global scope per callback would
be high overhead, I suspect it would be for other UAs as well, but would
love to get feedback regarding this.

[1]
https://drafts.css-houdini.org/css-properties-values-api/#registering-custom-properties
[2] https://github.com/w3c/css-houdini-drafts/issues/7
[3] https://lists.w3.org/Archives/Public/public-houdini/2015Aug/0004.html
[4] https://github.com/w3c/css-houdini-drafts/issues/9
[5] https://github.com/w3c/css-houdini-drafts/issues/8
[6] https://lists.w3.org/Archives/Public/public-houdini/2015Aug/0006.html