Re: [filter-effects] New syntax proposal for 'custom' filter function

Overall, looks good!  I really like how it lets you specify default
values for arguments, and otherwise just stash information into a
single location rather than spamming it across your stylesheet.

GLSL is still a required part of the Filters spec, though, right?  I
strongly don't support making GLSL optional, even if this proposal
does make it marginally easier to use two versions.

Some comments inline:

On Thu, Nov 15, 2012 at 2:04 PM, Dirk Schulze <dschulze@adobe.com> wrote:
> The formal syntax for the 'custom' function would be:
>
>         custom([ none | IDENT ]+ [, <params>]*)
>
> * IDENT is the name of a custom-filter at-rule.
> * <params> is a definition of a shader parameter as defined in the current working draft[3].

Throughout your examples you've used strings, not idents.  I agree
with IDENT, but I want to make sure that you actually meant that.

> custom-filter at-rule
> ===
>
> To address the needs on the defined goals, we came up with a new custom-filter at-rule. The custom-filter at-rule defines a shader program with the following required properties:
>
>         format: IDENT

As long as we have control over the format's name, I agree with IDENT
here as well.  If the name comes from somewhere external, we should
use <string> instead.

Once again, all of your examples are using strings, not idents.

This shouldn't be required.  It should default to "glsl" if not
specified, since that's the required shading language.

>         mesh: <integer>{0,2} <box>? [ detached | attached ]?

I don't see any reason to be strict about ordering here.  The grammar should be:

mesh: <integer>{1,2} || <box> || [ detached | attached ]

(Plus, the grammar as written was incorrect - it technically allows
"mesh:;" as valid.)

Also, this shouldn't be required.  Apparently they all have defaults,
so allow it to be omitted and just set those defaults.

>         mix: <compositing> && <blending>

Again, no need to make these required.  The obvious defaults are
"src-over" and "normal".  Given that, the grammar should also be
changed, to:

mix: <compositing> || <blending>

What's the origin of the name "mix"?  Do we think a similar property
will show up in the Compositing & Blending spec (as a shorthand, I
guess)?  If not, we shouldn't innovate here - just use separate
'alpha-compositing' and 'blend-mode' descriptors.

> The UA should take the value of 'format' into account to identify if the shader language is supported. If the author does not specify this property, the UA is recommended to interpret the shader as GLSL shader.

Okay, so you are requiring the format to be assumed to be GLSL if it's
not specified.  This means that 'format' is not a required descriptor.
 Also, "recommended" isn't an RFC 2119 keyword.  Do you mean MUST or
SHOULD?  (The former, please.)

> Other shading languages may require additional properties like 'geometry' for a geometry shader. This specification explicitly allows the extension of the custom-filter function by new properties, as the shading language requires. The property set must not be extended, if the format is specified as "glsl". A future version of this specification may define further identifiers and properties.

This can be rephrased - simply state that unknown descriptors do *not*
invalidate the rule, but that implementations MUST NOT extend the set
of descriptors accepted by the "glsl" format except as specified by
this specification or future versions.

> UAs must not accept 'format' identifier if the associated shading language is not supported.

Rephrase to say that unknown shading language idents make the entire
rule invalid.

> If the parsing of a custom-filter at-rule fails, the parsing of the referenced shader files fail, or the format identifier is not accepted by the UA, this rule is treated as invalid. The custom function must proceed with the next custom-filter at-rule in the custom-filter at-rule list. Shader files of invalid custom-filter at-rule are not downloaded.

Be *very* careful how you throw around the term "invalid".  Parsing
the referenced shader *can not* make the rule invalid, in the sense
that it's thrown away in the stylesheet.  Define a separate "invalid
shader" concept, have a failed parse flag the shader as an invalid
shader, and then have the fallback algorithm pay attention to whether
it's an "invalid shader" as well as whether the shader itself exists.

> Properties in the custom-filter at-rule are not animatable.

Things in at-rules are descriptors, not properties, and are never
animatable in the first place.  No need for this as a special case.


> Animation of custom function
> ====
>
> To avoid repetition of values, parameters can be set in the custom-filter at-rule. If a parameter does not need to be animated, authors should define the parameter in the custom-filter at-rule. Otherwise the parameter should be specified in the custom function.

Just define that the computed value of a custom filter contains all
the params, with params missing in the specified value filled in from
the defaults.  You already have a section on how to animate custom()
and its special new value types, so you're fine.

~TJ

Received on Thursday, 15 November 2012 22:44:15 UTC