googlefonts / colr-gradients-spec

63 stars 8 forks source link

Change overflow guidance re saturation to 'should' #350

Closed PeterConstable closed 3 years ago

behdad commented 3 years ago

To repeat: it is insane and ridiculous to prescribe how to deal with overflows when the data types themselves are unspecified and down to implementations.

For a rotation specified in F2DOT14 for example, overflow produces the correct behavior; saturation doesn't.

@rsheeter can we just remove this nonsense?

PeterConstable commented 3 years ago

overflow produces the correct behavior

Overflow only refers to a state in which an arithmetic operation has exceeded the capacity of a data type, but doesn't imply what result or effect is produced in an implementation. But it seems you are using "overflow" here to mean wrapping (from min to max, or vice versa).

it is ... ridiculous to prescribe... For a rotation specified in F2DOT14 for example, overflow produces the correct behavior

Rotations are---potentially---an exception; for any other context in which variation could occur (translations of contour points or elements, scaling...), wrapping would result in a large discontinuity. Given a general pattern with one (potential) exception, I don't think this can be considered ridiculous.

But is it actually true that, for rotations, "produces the correct behavior"?

First, we'd need to agree on what is "correct" behaviour for that scenario. If the wrap resulted in the same visual result that would occur with a larger data type that didn't overflow, so there was no visual discontinuity, it'd be reasonable to consider that "correct". I assume that's what you have in mind, and can agree with that.

That "correct" behaviour would only occur on wrap if the derived rotation angle were a multiple of 360°. The representation in the font for PaintRotate or PaintSweepGradient uses F2DOT14 with values scaled so that 1.0 = 180°; so 2.0 = 360°. It happens to be the case for F2DOT14 that a the difference between wrapping or not is a multiple of 2.0.

But we don't know what the internal representation is in any implementation. For instance, if they convert the font data values to direct representation in degrees using float, will the difference between wrapping or not be a multiple of 360? Unlikely.

So, in most cases of variable data a wrap would result in a large discontinuity, but in the exception cases of PaintRotate or PaintSweepGradient a wrap might not result in a discontinuity depending on the internal memory representation used. If we're so worried about better behaviour for rotations, we should be adding more guidance for those cases rather then avoiding guidance for the majority of cases.

behdad commented 3 years ago

But we don't know what the internal representation is in any implementation.

Exactly. So you CANNOT specify how to deal with overflows. That's what I've been repeating.

Let's step back and look.

The spec is unspecified in many many ways. That's widely understood. That's also undesirable.

However, what does NOT follow is that you can just jot down one way to do things. That wouldn't suddenly make the spec well-specified and useful. It makes it less useful, because you are adding words to the spec that are dead on arrival; that are known wrong before even being added. As such you are making the spec worse.

What do I mean wrong? Eg. HarfBuzz does NOT use saturation, and will NOT. HarfBuzz implements all these concepts correctly. But doesn't follow what you say shall or should happen. Because what you are prescribing is based on a wrong understanding of the concepts.

Let me repeat: you cannot just add random words to the spec to make it more specified, without making sure such words match actual implementations. In many places, the correct way to improve the spec currently is to say "so and so is unspecified / implementation-defined / undefined".

Again, maybe instead of fixating in how to handle overflows, you should think about how to formulate a prescription of what data types to use. TrueType bytecode hinting for example, specifies that math is to be done with 6bit subpixel precision. That's the kind of spec that is respectable.

Problem with trying to specify overflow behavior is that, overflow is inevitable, but many times when it happens, we might not care. For example, a small implementation for embedded systems might choose to use 16bit values for font-space coordinates (matching FWORD). One can build fonts that would overflow on that system. That's fine; it's just that such system doesn't support such fonts. We don't need to specify how to deal with it.

Contrary to your belief, specifying how to deal with overflow (eg. saturating) instead of calling it undefined, is WRONG. Based on your "saturate" definition, such a small embedded implementation is spec-compliant with respect to such font, because it's saturating instead of overflowing. Yet it's producing wrong results, compared to implementations that use a wider data-type.

Even a less constrained implementation, like HarfBuzz, uses 32bit values, with some of those bits used for sub-unit precision (eg. 6, 8, or 10, depending on the client). If a client uses HarfBuzz with 8bits of subpixel precision, that leaves 24 bits for spatial coordinates. OT allows for 16bit FUnit values. Even if you add words to spec to constraint variations of FUnit to be 16bit-only, the spec allows up to, whatever, 65k deltasets to be applied to a value. So you can CLEARLY still overflow; but only in constructed corner cases that we don't care about. We (HarfBuzz) are NOT going to add a SATURATED_ADD overhead to our math just because you chose to write it into the spec.

That's what I mean.

PeterConstable commented 3 years ago

Part of this debate seems to arise from different understanding of the term "overflow":

you cannot just add random words to the spec to make it more specified, without making sure such words match actual implementations.

In fact, I did investigate this some:

On reading more elsewhere about this topic, though, I see there is quite a bit of variation in how operations that exceed capacity of a type are handled. E.g.,

It's not my intent to force implementations to add costly (perf-wise) checks, but only to minimize potential inconsistencies of behaviour of fonts when used in different contexts. So, based on the variation in how this might naturally be handled in different implementations (but not based on assertions that this is "insane" or "nonsense"), I'm open to revising the wording.

I think something should still be called out*, but will work on different wording that I hope you can live with.

* The wikipedia article on integer overflow provides some interesting examples of things that can go badly wrong when implementations don't anticipate the possibility of exceeding the capacity of a data type. E.g.,

behdad commented 3 years ago

I think something should still be called out

Sure. That's the part we agree on. Previous I was of the opinion that it's enough to say something like: "note that the result of adding variations to a font scalar might exceed the limits of the data type such scalar is encoded with..."

But to be more thorough, I suggest something like this (added as a note), which should cover a lot:

When applying variation deltas to a scalar, extra care should be taken. In particular: 1) Contribution of each delta can be partial. As such, even if the base scalar and all the deltas are integers (pretty much most of the font format is this way), the contributions of each delta can be other than whole integer. As a minimum, the contributions of all applicable deltas should be added before any possible rounding happens; 2) The combined value of base value plus delta contributions might lead to a value outside the original range of the data type used to encode base value. That is, for example, even though a base value might be encoded as eg. 16bit integer, when delta contributions are added it might need more bits to be represented.

With above points in mind, the best way to implement variations is to use a floating-point value of at least 32bits internally. Failing that, using at least 32bit integers, and possibly at least 6 bits of fractional precision would be recommended.

If overflow is inevitable, saturation arithmetic might be used to mitigate the inevitable error artifacts.

PeterConstable commented 3 years ago

Contribution of each delta can be partial. As such, even if the base scalar and all the deltas are integers (pretty much most of the font format is this way), the contributions of each delta can be other than whole integer.

The draft currently conveys that, but it could be stated more explicitly.

As a minimum, the contributions of all applicable deltas should be added before any possible rounding happens;

The draft already says something equivalent: "If required for the internal representation, rounding should be done only when the final result is used...".

With above points in mind, the best way to implement variations is to use a floating-point value of at least 32bits internally. Failing that, using at least 32bit integers, and possibly at least 6 bits of fractional precision would be recommended.

The recommendation for at least 32 bits isn't currently stated, but can be added.

Since OT 1.8, a recommendation for >= 16 fractional bits precision was made at least in relation to axis coordinate normalization. Since the normalized coordinate is necessarily in the range [0, 1], scalars and scaled-delta calculation in general requires fractional representation. However, the recommendation for >=16 fractional bits wasn't stated later in the section on calculation of instance values. For the current revision, the draft already adds that recommendation into that section: "In calculation of scalars (S, AS) and of interpolated values (scaledDelta, netAjustment, interpolatedValue), at least 16 fractional bits of precision should be maintained."

I'll revise a bit more to incorporate your suggstions.

behdad commented 3 years ago

Thanks Peter. The new wording is much more clear and consistent with all implementations I know of.