Closed Bug 1284837 Opened 8 years ago Closed 8 years ago

Disallow implicit conversion from floating point to integer values in IntSize and IntPoint constructors

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(3 files, 2 obsolete files)

Disallow implicit conversions when constructing IntSize and IntPoint (carrying r=kas, botond)
(deleted), patch
Details | Diff | Splinter Review
Add IntPoint::Round(const Point&) etc. overrides for consistency.
(deleted), patch
botond
: review+
Details | Diff | Splinter Review
Disallow implicit conversions when constructing IntRect
(deleted), patch
botond
: review+
Details | Diff | Splinter Review 
I am investigating an off-by-one error at tile boundaries. I believe there is a good chance that it is caused by an IntPoint somewhere being constructed by from floating points without taking in consideration that casting from floats to integers doesn't behave in a way that usually makes sense towards zero from a geometrical point of view.

I think that a good way to avoid this kind of issue is to disallow IntPoint, IntSize and friends to have their constructors invoked with floats. The constructors would only accept combinations of integer types and special static methods (IntPoint::Round(x, y), IntPoint::Floor(x, y), IntPoint::Ceil(x, y) and IntPoint::Cast(x, y)) would be used with floating point values to make sure that there is a conscious and explicit choice in the way to convert from floating point to integer values.

Heads up, the change is a bit invasive.
I didn't think it *was* possible to make an IntPoint/IntSize using floats! Looking at the IntSizeTyped constructors, I see only two [1] and neither of them accept floats. Do you know how it's happening?

[1] http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/gfx/2d/Point.h#175
Here is a patch that builds on Linux. There are probably some build errors due to disallowed conversions in platform-specific code, but I'd like to get some feedback on the approach before finishing this up.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> I didn't think it *was* possible to make an IntPoint/IntSize using floats!
> Looking at the IntSizeTyped constructors, I see only two [1] and neither of
> them accept floats. Do you know how it's happening?

C++ simply converts from floats to ints if there is no ambiguity.

> auto s = IntSize(1.0, 2.0);

is equivalent to

> int a = 1.0;
> int b = 2.0;
> auto s = IntSize(a, b);
Thanks. For the record, -Wfloat-conversion will warn on this kind of stuff (as will -Wconversion, which enables -Wfloat-conversion). I tried turning this on and compiled Unified_cpp_gfx_layers1.cpp and it gave me 42 instances of this warning. I think your patch is a good step in the right direction.
I definitely support the idea of forcing lossy conversions like float -> int to be explicit.

We have some existing functions to convert floating-point quantities to integer quantities: RoundedToInt [1] and TruncatedToInt [2]. Would it be possible to merge these with the proposed interfaces? For example, if IntPointTyped::Round() had an overload that accepted a PointTyped, that could replace RoundedToInt() (or the other way around), and we'd have a more consistent set of interfaces.

[1] https://dxr.mozilla.org/mozilla-central/rev/95ffbc4ff63584631c408e8d9912961fcf68bb09/gfx/2d/Point.h#101
[2] https://dxr.mozilla.org/mozilla-central/rev/95ffbc4ff63584631c408e8d9912961fcf68bb09/gfx/2d/Point.h#107
(In reply to Botond Ballo [:botond] from comment #5)
> We have some existing functions to convert floating-point quantities to
> integer quantities: RoundedToInt [1] and TruncatedToInt [2]. Would it be
> possible to merge these with the proposed interfaces? For example, if
> IntPointTyped::Round() had an overload that accepted a PointTyped, that
> could replace RoundedToInt() (or the other way around), and we'd have a more
> consistent set of interfaces.

Sounds good to me, as long as the functions are where people will look for them (that is, in the interface of the thing they are trying to make).
Initial patch: this patch does not try to "fix" any cast yet, although it makes casts related to IntPoint and IntSize construction easy to find. We should go through each ::Cast call and check whether it is the right thing to do as a followup.
Attachment #8768386 - Attachment is obsolete: true
Attachment #8770035 - Flags: review?(bugmail)
Comment on attachment 8770035 [details] [diff] [review]
Disallow implicit covnersions when constructing IntSize and IntPoint

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

This looks ok to me, but I think Botond would be a better reviewer for the Point.h changes.

::: dom/base/nsDOMWindowUtils.cpp
@@ +2799,5 @@
>    if (!aLayer->GetTransform().Is2D(&transform) ||
>        transform.HasNonIntegerTranslation())
>      return false;
>    transform.NudgeToIntegers();
> +  IntPoint offset = aOffset + IntPoint::Cast(transform._31, transform._32);

I know they are typedef'd to basically the same thing, but is there a particular reason you're changing nsIntPoint to IntPoint here? You should be able to do nsIntPoint offset = aOffset + nsIntPoint::Cast(...), no?

::: dom/media/raw/RawReader.cpp
@@ +64,3 @@
>    ScaleDisplayByAspectRatio(display, pixelAspectRatio);
>    mPicture = nsIntRect(0, 0, mMetadata.frameWidth, mMetadata.frameHeight);
> +  nsIntSize frameSize(uint32_t(mMetadata.frameWidth), uint32_t(mMetadata.frameHeight));

PRUint24, wtf!

::: widget/cocoa/nsChildView.mm
@@ +3666,5 @@
>    CGContextSaveGState(aContext);
>    CGContextScaleCTM(aContext, 1.0 / scale, 1.0 / scale);
>  
>    NSSize viewSize = [self bounds].size;
> +  gfx::IntSize backingSize = gfx::IntSize::Cast(viewSize.width * scale, viewSize.height * scale);

Same here, any particular reason to convert nsIntSize to gfx::IntSize?

@@ +4046,5 @@
>        if (shouldRollup) {
>          if ([theEvent type] == NSLeftMouseDown) {
>            NSPoint point = [NSEvent mouseLocation];
>            FlipCocoaScreenCoordinate(point);
> +          gfx::IntPoint pos = gfx::IntPoint::Cast(point.x, point.y);

Ditto
Attachment #8770035 - Flags: review?(bugmail)
Attachment #8770035 - Flags: review?(botond)
Attachment #8770035 - Flags: review+
Comment on attachment 8770035 [details] [diff] [review]
Disallow implicit covnersions when constructing IntSize and IntPoint

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

r+ with the implementation of TruncatedToInt() fixed.

::: gfx/2d/Point.h
@@ +32,5 @@
>  template<> struct IsPixel<gfx::UnknownUnits> : TrueType {};
>  
>  namespace gfx {
>  
> +/// Use this as for parameters of functions to allow implicit conversions for

s/as for/for

@@ +33,5 @@
>  
>  namespace gfx {
>  
> +/// Use this as for parameters of functions to allow implicit conversions for
> +/// integer types but nor floating point types.

s/nor/not

@@ +35,5 @@
>  
> +/// Use this as for parameters of functions to allow implicit conversions for
> +/// integer types but nor floating point types.
> +/// We use this wrapper to prevent IntSize and IntPoint's constructors to
> +/// take foating point values as parameters, and not require theirs constructors

s/theirs/their

@@ +48,5 @@
> +  constexpr MOZ_IMPLICIT IntParam(unsigned int val) : value(val) {}
> +  constexpr MOZ_IMPLICIT IntParam(long val) : value(val) {}
> +  constexpr MOZ_IMPLICIT IntParam(unsigned long val) : value(val) {}
> +  constexpr MOZ_IMPLICIT IntParam(long long val) : value(val) {}
> +  constexpr MOZ_IMPLICIT IntParam(unsigned long long val) : value(val) {}

You can avoid listing all the types by using EnableIf:

template <typename T, typename U = typename EnableIf<IsIntegral<T>::value>::Type>
constexpr MOZ_IMPLICIT IntParam(T val) : value(val) {}

(Then you wouldn't need the deleted constructors either. Any type other than integral types would fail to compile.)

I'll leave it up to you to choose if you'd like to do it this way.

@@ +85,5 @@
> +  static IntPointTyped<units> Floor(float aX, float aY) {
> +    return IntPointTyped(int32_t(floorf(aX)), int32_t(floorf(aY)));
> +  }
> +
> +  static IntPointTyped<units> Cast(float aX, float aY) {

I kind of like "Truncate" as the name for this, but I'm fine with "Cast" if you prefer that.

@@ +143,5 @@
>  }
>  
>  template<class units>
>  IntPointTyped<units> TruncatedToInt(const PointTyped<units>& aPoint) {
> +  return RoundedToInt(aPoint);

The point of this function is to truncate rather than round. If we wanted to round, we'd just call RoundedToInt().

So the implementation should be something like:

  return IntPointTyped<units>::Cast(aPoint.x, aPoint.y);

::: widget/android/nsWindow.cpp
@@ +1571,5 @@
>      mBounds.width = NSToIntRound(aWidth);
>      mBounds.height = NSToIntRound(aHeight);
>  
> +    if (needSizeDispatch) {
> +        OnSizeChanged(gfx::IntSize(mBounds.width, mBounds.height));

We're changing from casting the original values, to using the rounded values. Is that intentional? If so, should it go into a separate patch?
Attachment #8770035 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Comment on attachment 8770035 [details] [diff] [review]
> > +  IntPoint offset = aOffset + IntPoint::Cast(transform._31, transform._32);
> 
> I know they are typedef'd to basically the same thing, but is there a
> particular reason you're changing nsIntPoint to IntPoint here? You should be
> able to do nsIntPoint offset = aOffset + nsIntPoint::Cast(...), no?

Yes, that works as well. When we made nsIntPoint and friends typedefs of their moz2d counterpart, I asked module owners if they preferred to use nsFoo or gfx::Foo in a dev-platform thread and the popular answer was gfx::Foo. I would not write an invasive patch to get rid of ns typedefs but for lines that we have to touch in our patches I think that it makes sense to fix them up to use the gfx names.
Here is the updated patch addressing Botond's comments. I don't have a strong opinion about Cast vs Truncate so I replaced the former with the latter. I also fixed the two behavior changes which were remnants from an earlier version of the patch where I was trying to flush out a potential off-by-one error. This version of the patch should now be free of changes of behavior.
Attachment #8770035 - Attachment is obsolete: true
As suggested by Botond. The patch also changes some of the calls to RoundToInt/TruncateToInt to use ::Round and ::Truncate to see how that looks, although I did not convert all of the code (in particular I don't know if peers of non-gfx modules like to have cosmetic changes cloud their hg history).
Attachment #8770520 - Flags: review?(botond)
Same story with IntRect. Note that gfxRect is different from gfx::Rect (it uses doubles instead of floats), and I did not add shortcuts to convert between IntRect and gfxRect. Someone may want to add that but I don't want to procrastinate too much on making all of this nice and pretty.
Attachment #8770521 - Flags: review?(botond)
Comment on attachment 8770521 [details] [diff] [review]
Disallow implicit conversions when constructing IntRect

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +1161,5 @@
>    LayerIntRegion visible;
>    PostProcessLayers(mRoot, opaque, visible, Nothing());
>  
>    nsIntRegion invalid;
> +  IntRect bounds = (0, 0, scale * pageWidth, actualHeight);

Hm, I don't know what happened there and why it built at all but it is fixed locally.
Comment on attachment 8770520 [details] [diff] [review]
Add IntPoint::Round(const Point&) etc. overrides for consistency.

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

Thanks for working on this!

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +266,5 @@
>    Maybe<LayerPoint> pointInLayerPixels = Untransform(aPoint);
>    if (!pointInLayerPixels) {
>      return HitTestResult::HitNothing;
>    }
> +  LayerIntPoint point = LayerIntPoint::Round(pointInLayerPixels.ref());

So, now that I'm looking at a call site like this, I'm not sure if this is a win, because the units of the target point (here, Layer) have to be repeated.

Might it make sense to have these helpers located in a namespace (or a nontemplate static class), and have them just deduce the units from the argument?

Something like this:

  struct Conversions {  // or insert better name here
    template <typename units>
    static IntPointTyped<units> Round(const PointTyped<units>& aPoint) {
        return IntPointTyped<units>::Round(aPoint);
    }
    ...
  };

  ...
  
  // call site:
  LayerIntPoint point = Conversions::Round(pointInLayerPixels.ref());

What do you think?
Comment on attachment 8770521 [details] [diff] [review]
Disallow implicit conversions when constructing IntRect

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

The same comment as before applies to the cases where the units are already encoded in the argument.

::: gfx/2d/Rect.h
@@ +91,5 @@
>  
>      IntRectTyped() : Super() {}
>      IntRectTyped(const IntPointTyped<units>& aPos, const IntSizeTyped<units>& aSize) :
>          Super(aPos, aSize) {}
> +    IntRectTyped(ToInt _x, ToInt _y, ToInt _width, ToInt _height) :

Might as well take the opportunity to convert to the aX, aY, ... style.

::: gfx/layers/basic/BasicPaintedLayer.cpp
@@ +31,5 @@
>  namespace layers {
>  
>  using namespace mozilla::gfx;
>  
> +static nsIntRegion&&

This is incorrect - the reference will be dangling by the time the function returns to the caller. Return it by value instead; it will still be moved rather than copied.

@@ +39,4 @@
>    nsIntRegion result;
> +  result.And(aRegion, IntRect::RoundOut(clip.X(), clip.Y(),
> +                                        clip.Width(), clip.Height()));
> +  return Move(result);

The Move() here is also unnecessary. The value will be moved automatically if the type has a move constructor. (See bug 1280296 comments 6-10 for a more detailed explanation if you're interested).

::: gfx/layers/client/TiledContentClient.cpp
@@ +1108,5 @@
>                         dirtyRect.height);
>      drawRect.Scale(mResolution);
>  
>      // Mark the newly updated area as invalid in the front buffer
> +    aTile.mInvalidFront.Or(aTile.mInvalidFront, IntRect::RoundOut(drawRect));

This looks like a behaviour change - rounding out isn't what we were doing before.
Attachment #8770521 - Flags: review?(botond) → feedback+
Attachment #8770520 - Flags: review?(botond) → feedback+
(In reply to Botond Ballo [:botond] from comment #15)
> > +  LayerIntPoint point = LayerIntPoint::Round(pointInLayerPixels.ref());
> 
> So, now that I'm looking at a call site like this, I'm not sure if this is a
> win, because the units of the target point (here, Layer) have to be repeated.
> [...]
> Something like this:
>   // call site:
>   LayerIntPoint point = Conversions::Round(pointInLayerPixels.ref());

The advantage of having the static method in the class is that it is a lot more discoverable: someone who does not work on gfx stuff daily will not know to look outside of the IntPoint definition when wondering if there is a way to go from Point to IntPoint.
If repeating the unit is a problem, we can always use auto:

>   auto point = LayerIntPoint::Round(pointInLayerPixels.ref());

We might as well keep the current functions (RoundedToInt, etc) if we are not going to unify the API since consistency is the only goal of this change.

(In reply to Botond Ballo [:botond] from comment #16)

> > +static nsIntRegion&&
> 
> This is incorrect - the reference will be dangling by the time the function
> returns to the caller. Return it by value instead; it will still be moved
> rather than copied.

Yeah I got mixed up, it didn't even compile on all platforms. I reverted this part.

> >      // Mark the newly updated area as invalid in the front buffer
> > +    aTile.mInvalidFront.Or(aTile.mInvalidFront, IntRect::RoundOut(drawRect));
> 
> This looks like a behaviour change - rounding out isn't what we were doing
> before.

True that, but in the case of tracking invalid regions, I am confident we are always better off rounding out so I sorta eagerly put it in the patch to not forget. I can revert it and leave a post-it note on my desk, that works too.
(In reply to Nicolas Silva [:nical] from comment #17)
> (In reply to Botond Ballo [:botond] from comment #15)
> > > +  LayerIntPoint point = LayerIntPoint::Round(pointInLayerPixels.ref());
> > 
> > So, now that I'm looking at a call site like this, I'm not sure if this is a
> > win, because the units of the target point (here, Layer) have to be repeated.
> > [...]
> > Something like this:
> >   // call site:
> >   LayerIntPoint point = Conversions::Round(pointInLayerPixels.ref());
> 
> The advantage of having the static method in the class is that it is a lot
> more discoverable: someone who does not work on gfx stuff daily will not
> know to look outside of the IntPoint definition when wondering if there is a
> way to go from Point to IntPoint.
> If repeating the unit is a problem, we can always use auto:
> 
> >   auto point = LayerIntPoint::Round(pointInLayerPixels.ref());
> 
> We might as well keep the current functions (RoundedToInt, etc) if we are
> not going to unify the API since consistency is the only goal of this change.

You're right, using "auto" seems to alleviate this concern for the most part.

How about we make the changes in your patch, but keep the RoundedToInt and TruncatedToInt functions around for cases where deducing the units comes in handy (your patch wasn't removing them anyways), and perhaps change their implementation to dispatch to IntPointTyped<units>::Round() and ::Truncate()?

> > >      // Mark the newly updated area as invalid in the front buffer
> > > +    aTile.mInvalidFront.Or(aTile.mInvalidFront, IntRect::RoundOut(drawRect));
> > 
> > This looks like a behaviour change - rounding out isn't what we were doing
> > before.
> 
> True that, but in the case of tracking invalid regions, I am confident we
> are always better off rounding out so I sorta eagerly put it in the patch to
> not forget. I can revert it and leave a post-it note on my desk, that works
> too.

I'm happy to r+ this change, but I'd prefer that it go into a separate patch for bisection purposes.
Comment on attachment 8770520 [details] [diff] [review]
Add IntPoint::Round(const Point&) etc. overrides for consistency.

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

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +266,5 @@
>    Maybe<LayerPoint> pointInLayerPixels = Untransform(aPoint);
>    if (!pointInLayerPixels) {
>      return HitTestResult::HitNothing;
>    }
> +  LayerIntPoint point = LayerIntPoint::Round(pointInLayerPixels.ref());

Let's use "auto" here.
Attachment #8770520 - Flags: feedback+ → review+
Comment on attachment 8770521 [details] [diff] [review]
Disallow implicit conversions when constructing IntRect

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

r+ with the Move() issue fixed and the invalid region bit moved to a separate patch.

Thanks!
Attachment #8770521 - Flags: feedback+ → review+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/550c31d39ecc
Disallow implicit conversions from float to integer when creating IntPoint and IntSize objects. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea1e25c73fc
Add conversions methods from Point and Size to IntPoint and IntSize for consistency. r=botond
Whiteboard: leave-open
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/78dd94ba93c7
Build fix. We need to to unbreak m-c and other trees builds. a=tomcat
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59f96abc40b
Disallow implicit conversions from float to integer when creating. r=botond
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: