923512 - Introduce strongly typed coordinate classes

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

Our strongly typed point, size, rect, and margin classes have been very useful for catching bugs caused by mismatched units.

One hole in this safety net is single coordinates, for which we still use plain floats. 'Axis' in particular uses single coordinates extensively. Strong typing for these could cause bugs like bug 923431 to be discovered sooner.

I propose that we introduce a new Coord class, templated on a pixel type, which represents a single coordinate. I think we can add this without disturbing any other code for now, and just use it in Axis. We can then gradually introduce it into other code if desired.

Comment 1

[Botond Ballo [:botond]]

Bas, what do you think about this idea?

Comment 2

[Bas Schouten (:bas.schouten)]

I think it might certainly be a valid idea. Although since this will be mainly used in layout I suppose the layout team's opinion might matter more :).

Comment 3

[Botond Ballo [:botond]]

Attachment: WIP patch

Attached is a WIP patch that:

   - introduces BaseCoord<>, IntCoordTyped<> and CoordTyped<> class templates,
     in similar vein to BasePoint<>, IntPointTyped<> and PointTyped<>

   - modifies APZ's Axis class to use these instead of plain floats

   - make necessary changes to other parts of the codebase to keep it
     compilable

This currently compiles for me locally, with a Linux desktop build, except for an internal compiler error in nsEventStateManager.cpp.

Comment 4

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #3)
> This currently compiles for me locally, with a Linux desktop build, except
> for an internal compiler error in nsEventStateManager.cpp.

I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.

Comment 5

[(no longer active)]

(In reply to comment #4)
> (In reply to Botond Ballo [:botond] from comment #3)
> > This currently compiles for me locally, with a Linux desktop build, except
> > for an internal compiler error in nsEventStateManager.cpp.
> 
> I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.

Living on the edge.  :-)

Comment 6

[Botond Ballo [:botond]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> > I reported the ICE at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959.
> 
> Living on the edge.  :-)

I posted a minimal example that reproduces the crash at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61959#c4. It doesn't do anything novel, it just extends a pattern we already used for one concept (BasePoint/PointTyped) to another (BaseCoord/CoordTyped).

Comment 7

[Botond Ballo [:botond]]

I found a workaround for the ICE: not deriving 'TypedCoord<units>' and 'IntTypedCoord<units>' from 'units'.

The purpose of deriving 'TypedPoint<units>' and 'IntTypedPoint<units>' from 'units' is so that we can do things like 'LayoutDevicePoint::FromUntyped(point)' (with 'FromUntyped' being defined by 'LayoutDevicePixel'). I think we can live with not being able to call functions defined in the pixel classes via 'LayoutDeviceCoord::' and such.

Comment 8

[Botond Ballo [:botond]]

Attachment: WIP patch

Updated patch to fix ICE. Patch now compiles locally.

Now, let's see if it compilers on other platforms: https://tbpl.mozilla.org/?tree=Try&rev=2306c0af7b54

Comment 9

[Botond Ballo [:botond]]

Attachment: Introduce strongly-typed coordinate classes

Cleaned up the patch a bit. Will wait for Try results before posting for review.

Comment 10

[Botond Ballo [:botond]]

Fixed some Werror and Windows bustage. ReTrying (build-only): https://tbpl.mozilla.org/?tree=Try&rev=f8f1c55dc774

Comment 11

[Botond Ballo [:botond]]

Fixed more Windows bustage. Trying again, Windows builds only this time: https://tbpl.mozilla.org/?tree=Try&rev=3189dc876731

Comment 12

[Botond Ballo [:botond]]

Attachment: Introduce strongly-typed coordinate classes

Updated patch will all bustages fixed. Now builds on all platforms.

Posting for review. A few design notes:

   - I transitioned the 'x' and 'y' fields of the Point classes
     to be Coords rather than plain numbers, because this was
     needed for using coordinate classes effectively in Axis
     (which is my poster child for this change). This had
     repercussions in the rest of the codebase, but they were
     relatively limited, i.e. I only had to make changes in
     a handful of places.

     I plan to similarly transition Size, Rect, and Margin,
     but I propose doing this as a follow-up, and possibly
     after some other parts of the code have been transitioned
     to use Coord, because doing it now would involve a lot
     of changes in many places.

   - Coords are currently implicitly convertible to the numeric
     type they wrap, and I provide operator overloads for most
     mixed-type operations on Coords (e.g. adding a Coord and
     a float). This, again, is to ease the transition to Coords.
     Once enough of the codebase has been transitioned to use
     Coords, most of these can be removed, and the benefits of
     strong typing realized more fully.

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #12)
> Posting for review.

Suggested review order:

  - BaseCoord.h, Coord.h, BasePoint.h, Point.h, Units.h 
    (and moz.build, GfxMessageUtils.h)
    These are the core changes.

  - Axis.h/cpp, AsyncPanZoomController.cpp, APZCTreeManager.cpp
    These demonstrate the coordinate classes in action.

  - Everything else is fallout from changing the types of Point's
    x and y coordinates.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8465016 [details] [diff] [review]
Introduce strongly-typed coordinate classes

Review of attachment 8465016 [details] [diff] [review]:
-----------------------------------------------------------------

Bas should review the changes in gfx/2d/. Minusing for the Truncate[d] issue, but generally this looks ok to me. Do you know what kind of impact this will have on performance?

::: gfx/2d/Coord.h
@@ +104,5 @@
> +  IntCoordTyped<units> Rounded() const {
> +    return IntCoordTyped<units>(int32_t(floor(this->value + 0.5)));
> +  }
> +  IntCoordTyped<units> Truncated() const {
> +    return IntCoordTyped<units>(int32_t(this->value));

Truncate() and Truncated() will give you different results for negative numbers. floor(-3.5) will give -4 but int32_t(-3.5) will give you -3. At least that's what happens on my mac.

::: gfx/layers/apz/src/Axis.cpp
@@ +22,1 @@
>  namespace mozilla {

Put blank line back. Also in general better to separate the cosmetic changes from the functional changes in this file.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +760,5 @@
>    // target to stop tests failing - this is only used by the Android browser
>    // UI for its dynamic toolbar.
>    IntPoint origin;
>    if (!mTarget) {
> +    origin = -RoundedToInt(mRenderOffset.ToUnknownPoint());

This adds a rounding where there was none previously. Intentional?

Comment 15

[Botond Ballo [:botond]]

Attachment: Introduce strongly-typed coordinate classes

Updated patch; addresses review comments.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> Do you know what kind of impact this will have on performance?

I would expect no performance impact, as the compiler should easily be able to optimize away the extra class wrapper we're adding.

> ::: gfx/2d/Coord.h
> @@ +104,5 @@
> > +  IntCoordTyped<units> Rounded() const {
> > +    return IntCoordTyped<units>(int32_t(floor(this->value + 0.5)));
> > +  }
> > +  IntCoordTyped<units> Truncated() const {
> > +    return IntCoordTyped<units>(int32_t(this->value));
> 
> Truncate() and Truncated() will give you different results for negative
> numbers. floor(-3.5) will give -4 but int32_t(-3.5) will give you -3. At
> least that's what happens on my mac.

Good catch! I changed both to be int32_t(value) to be consistent.

> ::: gfx/layers/apz/src/Axis.cpp
> @@ +22,1 @@
> >  namespace mozilla {
> 
> Put blank line back. Also in general better to separate the cosmetic changes
> from the functional changes in this file.

Eek! I must have invoked Eclipse's auto-format shortcut, and then saved without realizing it. Fixed.

> ::: gfx/layers/opengl/CompositorOGL.cpp
> @@ +760,5 @@
> >    // target to stop tests failing - this is only used by the Android browser
> >    // UI for its dynamic toolbar.
> >    IntPoint origin;
> >    if (!mTarget) {
> > +    origin = -RoundedToInt(mRenderOffset.ToUnknownPoint());
> 
> This adds a rounding where there was none previously. Intentional?

There was an implicit truncation previously. To preserve the behaviour I changed this to be an (explicit) truncation.

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8465556 [details] [diff] [review]
Introduce strongly-typed coordinate classes

Not sure why this had Kats marked for review twice.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8465556 [details] [diff] [review]
Introduce strongly-typed coordinate classes

Review of attachment 8465556 [details] [diff] [review]:
-----------------------------------------------------------------

I looked at all the stuff outside gfx/2d/ and the {Base,}{Point,Coord}.h files inside gfx/2d/. Didn't look at the other changes in gfx/2d/.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +614,5 @@
>      gfx3DMatrix transformToGecko;
>      GetInputTransforms(apzc, transformToApzc, transformToGecko);
>      gfx3DMatrix outTransform = transformToApzc * transformToGecko;
> +    aOutTransformedPoint->x = LayoutDeviceIntCoord(aPoint.x);
> +    aOutTransformedPoint->y = LayoutDeviceIntCoord(aPoint.y);

Already getting into bad habits of casting from one pixel type to another without explanation! :p

::: gfx/layers/apz/src/Axis.h
@@ +54,5 @@
>    /**
>     * Notify this Axis that a new touch has been received, including a timestamp
>     * for when the touch was received. This triggers a recalculation of velocity.
>     */
> +  void UpdateWithTouchAtDevicePoint(ScreenIntCoord aPos, uint32_t aTimestampMs);

We should probably change the ScreenIntCoords in this file to ScreenCoords. No sense rounding/truncating unless needed. That should be deferred to a follow-up bug though.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +271,2 @@
>    const gfx::Point layerTransform = GetScrollData(aLayer);
> +  const gfx::Point layerScroll = scrollOffset.ToUnknownPoint() - layerTransform;

Check with BenWa to make sure the rounding that was here before isn't intentional. It looks kind of suspicious.

Comment 18

[Botond Ballo [:botond]]

Attachment: Introduce strongly-typed coordinate classes

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +614,5 @@
> >      gfx3DMatrix transformToGecko;
> >      GetInputTransforms(apzc, transformToApzc, transformToGecko);
> >      gfx3DMatrix outTransform = transformToApzc * transformToGecko;
> > +    aOutTransformedPoint->x = LayoutDeviceIntCoord(aPoint.x);
> > +    aOutTransformedPoint->y = LayoutDeviceIntCoord(aPoint.y);
> 
> Already getting into bad habits of casting from one pixel type to another
> without explanation! :p

I believe the code is correct, since we are applying a transform to the point to get it into LayoutDevice space.

One day, I hope to introduce strongly-typed matrix classes so that we can encode into the type of 'outTransform' that it converts from Screen space to LayoutDevice space.

In the meantime, I rewrote the code to use TransformTo<>(), which makes it look a bit better. This involved introducing an overload of TransformTo<>() that applies to IntPoints. Re-requesting review for these bits.

> ::: gfx/layers/apz/src/Axis.h
> @@ +54,5 @@
> >    /**
> >     * Notify this Axis that a new touch has been received, including a timestamp
> >     * for when the touch was received. This triggers a recalculation of velocity.
> >     */
> > +  void UpdateWithTouchAtDevicePoint(ScreenIntCoord aPos, uint32_t aTimestampMs);
> 
> We should probably change the ScreenIntCoords in this file to ScreenCoords.
> No sense rounding/truncating unless needed. That should be deferred to a
> follow-up bug though.

Filed bug 1046993 for this.

> ::: gfx/layers/composite/ContainerLayerComposite.cpp
> @@ +271,2 @@
> >    const gfx::Point layerTransform = GetScrollData(aLayer);
> > +  const gfx::Point layerScroll = scrollOffset.ToUnknownPoint() - layerTransform;
> 
> Check with BenWa to make sure the rounding that was here before isn't
> intentional. It looks kind of suspicious.

Checked with Mason and BenWa; the rounding wasn't intentional.

Comment 19

[Bas Schouten (:bas.schouten)]

I've already looked a little bit at the Moz2D parts and haven't seen anything particularly objectionable there, why are there a few places where we need to .value though?

Also, could you please split Moz2D changes up in a separate patch that goes in the queue before the rest? That makes everything a little cleaner.

Comment 20

[Botond Ballo [:botond]]

(In reply to Bas Schouten (:bas.schouten) from comment #19)
> I've already looked a little bit at the Moz2D parts and haven't seen
> anything particularly objectionable there, why are there a few places where
> we need to .value though?

Sometimes we do things like 'sqrt(point.x * point.x + point.y * point.y)'. I don't want to define an operator*(Coord, Coord) until we have a meaningful strongly-typed result type that would make sense for it (like an Area or something).

> Also, could you please split Moz2D changes up in a separate patch that goes
> in the queue before the rest? That makes everything a little cleaner.

Will do.

Comment 21

[Botond Ballo [:botond]]

Attachment: Part 1 - Moz2D changes

Comment 22

[Botond Ballo [:botond]]

Attachment: Part 2 - Changes to everything else

Carrying r+ from Kats. Bas, if you'd like to review this part as well, please feel free.

Comment 23

[Bas Schouten (:bas.schouten)]

Comment on attachment 8465754 [details] [diff] [review]
Part 1 - Moz2D changes

Review of attachment 8465754 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not 100% sure I agree with not allowing multiplication. But I'm also sensitive to your argument of it being nice to have a valid unit for those. For now let's just get this stuff in.

Comment 24

[Botond Ballo [:botond]]

Attachment: Part 0 - Transition the TransformTo<>() functions from gfx3DMatrix to Matrix4x4

This additional bit became necessary during rebasing.

Comment 25

[Botond Ballo [:botond]]

Attachment: Part 1 - Moz2D changes

Rebased, carrying r+

Comment 26

[Botond Ballo [:botond]]

Attachment: Part 2 - Changes to everything else

Rebased, carrying r+

Comment 27

[Botond Ballo [:botond]]

Another build-only Try push to make sure nothing has broken since last time:

https://tbpl.mozilla.org/?tree=Try&rev=982e5d1e578e

Comment 28

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dff51863663c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f431232084b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8720d81b74f

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d577815636c6 for mochitest-2 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=45371969&tree=Mozilla-Inbound


Reftest failed once on this push with https://tbpl.mozilla.org/php/getParsedLog.php?id=45369171&tree=Mozilla-Inbound but I don't know yet if it was just a one-off failure or is related to this patch.

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 1 - Moz2D changes

The test failures caught a legitimate, and tricky, bug!

The problem was that the mixed strongly-typed/primitive operations were improperly returning the primitive type as their result type. This is wrong when e.g. the strongly-typed argument is a float-valued Coord, and the primitive argument is an integer, because then the result gets truncated to an integer, so that e.g. ScreenCoord(50) / 100 would return 0 instead of 0.5.

The fix is to return the "common type" of the Coord's underlying type and the primitive argument type, i.e. the same type that C++ would return for the same operation performed on primitive types.

The only modification is to the return types in CoordOperationsHelper, and the addition of CommonType to help compute them. Asking for re-review for these changes.

I'd Try, but Try is closed.

Comment 31

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #30)
> I'd Try, but Try is closed.

https://tbpl.mozilla.org/?tree=Try&rev=6017ad4b667e

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8469600 [details] [diff] [review]
Part 1 - Moz2D changes

Review of attachment 8469600 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review back since Bas is on PTO or something. The changes look ok to me.

Comment 33

[Botond Ballo [:botond]]

Doing another build-only Try push since some time has elapsed since the last one:

https://tbpl.mozilla.org/?tree=Try&rev=67e9c0e7ee35

Comment 34

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4df5daa7ca34
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9f66a511d2

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4df5daa7ca34
https://hg.mozilla.org/mozilla-central/rev/6a9f66a511d2