Thinker Li [:sinker] Assignee
	Description • 9 years ago

Load following URL
http://mrdoob.com/lab/javascript/threejs/css3d/

expect to see a cube at middle, but there is bar staying at right side of the screen until next window re-sizing or moving.

see https://bugzilla.mozilla.org/attachment.cgi?id=8682531

	Thinker Li [:sinker] Assignee
	Comment 1 • 9 years ago

see also https://bugzilla.mozilla.org/show_bug.cgi?id=1210784#c32

	u279076
	Comment 2 • 9 years ago

I cannot reproduce this locally. Do you have any more details?

	Ryan VanderMeulen [:RyanVM]
	Comment 3 • 9 years ago

I'm the original reporter of the issue. I've provided details (and a layers dump) in other bugs. I'm happy to keep working on debugging this with Thinker.

	u279076
	Comment 4 • 9 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #3)
> I'm the original reporter of the issue. I've provided details (and a layers
> dump) in other bugs. I'm happy to keep working on debugging this with
> Thinker.

Thanks Ryan, is this a regression that needs to get tracked?

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 9 years ago

It's a regression from bug 1097464, so probably.

	Thinker Li [:sinker] Assignee
	Comment 7 • 9 years ago

Ryan, I guess that we don't invalidate whole subtree if a layer is removed or added from or to a tree but the root of the subtree.  With the change of bug 1097464, the root of a subtree may not cover whole visible region of all layers.

This patch would fix this problem.  Could you try this patch to see if it fixes the problem.

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 9 years ago

Still reproduces with this patch, but only when the browser is maximized. When windowed, everything works nicely AFAICT.

FWIW, it appears that hovering over buttons on the toolbar is what triggers invalidations within the content area. See the attached screencast.

	Thinker Li [:sinker] Assignee
	Comment 9 • 9 years ago

--
Attachment #8687807 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 11 • 9 years ago

Hi Ryan, need your help to get a log for the latest patch.  Thank you!

	Thinker Li [:sinker] Assignee
	Comment 12 • 9 years ago

Hi Ryan, please apply both patches.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 9 years ago

This log was generated using a Firefox profile where the browser was set to start already-maximized with the test URL set as the default start page. I then did the same button hovering as shown in the earlier screencast, which yielded equivalent behavior to what was previously shown.

	Thinker Li [:sinker] Assignee
	Comment 14 • 9 years ago

It seems a floating error.
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp?case=true&from=LayerTreeInvalidation.cpp#76
gfxUtils::GfxRectToIntRec() returns false because of the size of |rect| is out of the range of int32_t.

	Thinker Li [:sinker] Assignee
	Comment 15 • 9 years ago

single precision floating point is with a fraction of 23 bits.   Rect::MaxIntRect() is in the order of 1e+9, it needs at least 30 bits to contain it.  In Matrix4x4::TransformAndClipRect(), x or y values of normals would be down to 1e-9, while w component is single digit. That means singe precision cause a serious floating error that gfxUtils::GfxRectToInitRect() may return false.

	Thinker Li [:sinker] Assignee
	Comment 16 • 9 years ago

--
Attachment #8689408 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 17 • 9 years ago

--
Attachment #8689407 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 18 • 9 years ago

Ryan, according to comment 15, the latest patch fixes the problem at my side.  Please, check it!

	Thinker Li [:sinker] Assignee
	Comment 19 • 9 years ago

Comment on attachment 8690700 [details] [diff] [review]
Invalidate the forest of new and removed layers, v3

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

::: gfx/2d/Rect.h
@@ +167,5 @@
>        return RectTyped<units, F>(
> +        -std::numeric_limits<int32_t>::max() * 0.025,
> +        -std::numeric_limits<int32_t>::max() * 0.025,
> +        std::numeric_limits<int32_t>::max() * 0.05,
> +        std::numeric_limits<int32_t>::max() * 0.05

Robert, the size of this rect is shrink to 5% to fix floating errors mentioned at comment 15.  Another solution is to use double instead of float in Matrix4x4::TransformAndClipRect().  What do you think?

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

Comment on attachment 8690700 [details] [diff] [review]
Invalidate the forest of new and removed layers, v3

Looks good here!

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 9 years ago

Comment on attachment 8690700 [details] [diff] [review]
Invalidate the forest of new and removed layers, v3

I'm seeing very laggy/artifacty scrolling with this patch applied, though. Lots of invalidation issues it appears.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 9 years ago

Comment on attachment 8690700 [details] [diff] [review]
Invalidate the forest of new and removed layers, v3

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

::: gfx/2d/Rect.h
@@ +167,5 @@
>        return RectTyped<units, F>(
> +        -std::numeric_limits<int32_t>::max() * 0.025,
> +        -std::numeric_limits<int32_t>::max() * 0.025,
> +        std::numeric_limits<int32_t>::max() * 0.05,
> +        std::numeric_limits<int32_t>::max() * 0.05

This seems like it will return something far too small.

We just want to ensure that calling TransformAndClipRectBounds(aRect, MaxIntRect()) returns something that's always valid for input to GfxRectToIntRect. So we only need MaxIntRect to be a little bit smaller --- so that its edges, after conversion to single precision floats, are still definitely in the valid range of int32_t. Its edges should still be close to 
std::numeric_limits<int32_t>::min()/max().

MaxIntRect is only used with TransformBoundsAndClipRect. I think we should create a new method Matrix4x4::TransformToIntRectRoundOut that combines TransformAndClipRectBounds with RoundOut and GfxRectToIntRect and always succeeds, and remove MaxIntRect. In a separate patch.

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +176,5 @@
>    virtual void MoveBy(const IntPoint& aOffset);
>  
>    nsIntRegion ComputeChange(NotifySubDocInvalidationFunc aCallback,
> +                            bool& aGeometryChanged,
> +                            int aFlags = 0)

Please explain in more detail what these flags do.

@@ +273,5 @@
> +      return OldTransformedBounds();
> +    }
> +
> +    bool dummy = false;
> +    return ComputeChange(aCallback, dummy, COMPUTE_CHANGE_COLLECT_OLD);

It looks like you're changing behavior for more than just preserve-3d stuff here.

@@ +411,5 @@
>          invalidateChildsCurrentArea = true;
>        }
>        if (invalidateChildsCurrentArea) {
>          aGeometryChanged = true;
> +        ContainerLayer *childContainer = child->AsContainerLayer();

This is not used?

	Thinker Li [:sinker] Assignee
	Comment 23 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Comment on attachment 8690700 [details] [diff] [review]
> Invalidate the forest of new and removed layers, v3
> 
> Review of attachment 8690700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Rect.h
> @@ +167,5 @@
> >        return RectTyped<units, F>(
> > +        -std::numeric_limits<int32_t>::max() * 0.025,
> > +        -std::numeric_limits<int32_t>::max() * 0.025,
> > +        std::numeric_limits<int32_t>::max() * 0.05,
> > +        std::numeric_limits<int32_t>::max() * 0.05
> 
> This seems like it will return something far too small.
> 
> We just want to ensure that calling TransformAndClipRectBounds(aRect,
> MaxIntRect()) returns something that's always valid for input to
> GfxRectToIntRect. So we only need MaxIntRect to be a little bit smaller ---
> so that its edges, after conversion to single precision floats, are still

I think it is not work.  For example, if XMost of MaxIntRect is 2^30 - 1, let's say 2^30.
The normal of the plane for XMost, would be [-(2^-30), 0, 0, 1], let's call it normalXMost.
So, for any vector V, V.dotProduct(normalXMost), the result would be V.x * -(2^-30) + V.w.
If V.w is big enough and the fraction part of single precision floating point has only 23bits, it results in that V.x * -(2^-3) is almost absence from the value in all.  For the case here, V.w is 3x ~= 2^5 and V.x is 200 ~= 2^8, it means (V.x * 2^-30)/V.w ~= 2^-27.  For single precision floating point, it equals to 1.0 + 2^-27 == 1.0.  In Matrix4x4::TransformAndClipRect(), we lose all information of x component, and we are finding the point where w == 0 in fact.

> definitely in the valid range of int32_t. Its edges should still be close to 
> std::numeric_limits<int32_t>::min()/max().

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 9 years ago

OK, I misunderstood how aClipRect is used.

I suggest we do what I said:
> I think we should create a new method Matrix4x4::TransformToIntRectRoundOut that combines TransformAndClipRectBounds with
> RoundOut and GfxRectToIntRect and always succeeds, and remove MaxIntRect. In a separate patch.
that always uses the double-precision TransformAndClipRect to avoid the issues you described.

	Thinker Li [:sinker] Assignee
	Comment 25 • 9 years ago

--
Attachment #8690699 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 26 • 9 years ago

--
Attachment #8690700 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 27 • 9 years ago

Ryan, this patch seems not laggy.  Could you check it? Thank you!

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 9 years ago

With this patch, the cube doesn't render at all for me (just an empty grey viewport with the text across the top).

	Thinker Li [:sinker] Assignee
	Comment 29 • 9 years ago

I run into the same problem after rebasing.

	Thinker Li [:sinker] Assignee
	Comment 30 • 9 years ago

I just find that even the code in m-c it don't show the cube.

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 9 years ago

Filed bug 1230780 for the new issue with the cube not rendering at all.

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 9 years ago

Bug 1097464 was backed out from Fx44, which should be available on the beta channel in the next day or so. Fx45+ remain affected, however.

	Thinker Li [:sinker] Assignee
	Comment 33 • 9 years ago

--
Attachment #8693161 [details] [diff] - Attachment is obsolete: true

	Sylvestre Ledru [:Sylvestre]
	Comment 34 • 9 years ago

Tracking as we don't want to release 45 with an obvious visual regression.

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 9 years ago

Comment on attachment 8703394 [details] [diff] [review]
Invalidate the forest of new and removed layers, v5

FYI, I tried to take a look at this patch now that the demo works again, but it doesn't compile.

gfx/layers/LayerTreeInvalidation.cpp:275:63: error: invalid initialization of reference of type 'const IntRect& {aka const mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits>&}' from expression of type 'mozilla::gfx::IntRectTyped<mozilla::LayerPixel>'

gfx/layers/LayerTreeInvalidation.cpp:65:1: error: in passing argument 1 of 'mozilla::gfx::IntRect mozilla::layers::TransformRect(const IntRect&, const Matrix4x4&)'

gfx/layers/LayerTreeInvalidation.cpp:350:46: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]

FWIW, I can still reproduce the problem with a current trunk build.

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 9 years ago

Confirmed that this no longer reproduces on 45 since the backout landed.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d

	Thinker Li [:sinker] Assignee
	Comment 37 • 9 years ago

--
Attachment #8703394 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 38 • 9 years ago

--
Attachment #8713611 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 39 • 9 years ago

Comment on attachment 8713656 [details] [diff] [review]
Invalidate the forest of new and removed layers, v7

This patch majorly works on computing invalid regions by diving recursively to descendants that participate the 3d rendering context.  The old way computes invalid regions from just visible regions of the layer, but for preserve-3d, visible regions of a layer may not cover all content of descendants.

	Thinker Li [:sinker] Assignee
	Comment 40 • 9 years ago

Comment on attachment 8713656 [details] [diff] [review]
Invalidate the forest of new and removed layers, v7

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +468,4 @@
>        i++;
>      }
>  
>      if (aCallback) {

Is it making sense to call callback for extend3d layers?  Invalidation regions might not be transformed right and meaningful for extend3d layers, since effective transforms may be singular or invisible temporary.  I think we should not call callback for extend3d layers.  How do you think?

	Thinker Li [:sinker] Assignee
	Comment 41 • 9 years ago

--
Attachment #8713656 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 42 • 9 years ago

Comment on attachment 8713693 [details] [diff] [review]
Invalidate the forest of new and removed layers, v8

Add testcase

	Thinker Li [:sinker] Assignee
	Comment 43 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30acff56a97d

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 44 • 9 years ago

Comment on attachment 8713693 [details] [diff] [review]
Invalidate the forest of new and removed layers, v8

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

::: gfx/2d/Rect.h
@@ +164,5 @@
>      // point at 0,0.
>      static RectTyped<units, F> MaxIntRect()
>      {
> +      // Multiple with 0.495 & 0.99 to keep away from overfloating
> +      // caused by computational error.

This comment isn't clear enough. Please describe exactly the problem you're solving.

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +132,5 @@
>  {
> +  // Collect old transformed bounds of descendants.
> +  static const int COMPUTE_CHANGE_COLLECT_OLD = 0x1;
> +  // Collect new transformed bounds of descendants.
> +  static const int COMPUTE_CHANGE_COLLECT_NEW = 0x2;

Can you be more precise here? How exactly do these flags affect ComputeChange? Do they just add to the region returned by ComputeChnage, or do they replace its normal computation?

::: layout/base/nsPresContext.cpp
@@ +2461,5 @@
> +  int32_t x1 = ADJUST_RANGE(aRect.x);
> +  int32_t y1 = ADJUST_RANGE(aRect.y);
> +  int32_t x2 = ADJUST_RANGE(aRect.XMost());
> +  int32_t y2 = ADJUST_RANGE(aRect.YMost());
> +#undef ADJUST_RANGE

Why do we need to make this change?

	Thinker Li [:sinker] Assignee
	Comment 45 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #44)
> ::: layout/base/nsPresContext.cpp
> @@ +2461,5 @@
> > +  int32_t x1 = ADJUST_RANGE(aRect.x);
> > +  int32_t y1 = ADJUST_RANGE(aRect.y);
> > +  int32_t x2 = ADJUST_RANGE(aRect.XMost());
> > +  int32_t y2 = ADJUST_RANGE(aRect.YMost());
> > +#undef ADJUST_RANGE
> 
> Why do we need to make this change?

Without this adjustment, it has an integer overflow at following lines of device pixels to app units (*60).

	Thinker Li [:sinker] Assignee
	Comment 46 • 9 years ago

--
Attachment #8713693 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 47 • 9 years ago

--
Attachment #8714154 [details] [diff] - Attachment is obsolete: true

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 48 • 9 years ago

Comment on attachment 8714156 [details] [diff] [review]
Invalidate the forest of new and removed layers, v9

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +130,5 @@
>  {
> +  /* Following flags make ComputeChange() recursively collecting
> +   * invalidation region (bounds) of descendants that participate the
> +   * same 3D rendering context.
> +   * 

trailing whitespace

@@ +139,5 @@
> +
> +  // Collect old transformed bounds of descendants.
> +  static const int COMPUTE_CHANGE_COLLECT_OLD = 0x1;
> +  // Collect new transformed bounds of descendants.
> +  static const int COMPUTE_CHANGE_COLLECT_NEW = 0x2;

The comment isn't clear enough. For example, some questions whose answers are not clear:

Which descendants are being included --- all, or just the descendants in the same 3D rendering context? Say that here.

Are we using the layer bounds of each descendant? Or the invalidation region from each descendant?

Does "old"/"new" refer to the old/new transforms, or the old/new bounds, or both?

	Thinker Li [:sinker] Assignee
	Comment 49 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> Comment on attachment 8714156 [details] [diff] [review]
> Invalidate the forest of new and removed layers, v9
> 
> Review of attachment 8714156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/LayerTreeInvalidation.cpp
> @@ +130,5 @@
> >  {
> > +  /* Following flags make ComputeChange() recursively collecting
> > +   * invalidation region (bounds) of descendants that participate the
> > +   * same 3D rendering context.
> > +   * 
> 
> trailing whitespace
> 
> @@ +139,5 @@
> > +
> > +  // Collect old transformed bounds of descendants.
> > +  static const int COMPUTE_CHANGE_COLLECT_OLD = 0x1;
> > +  // Collect new transformed bounds of descendants.
> > +  static const int COMPUTE_CHANGE_COLLECT_NEW = 0x2;
> 
> The comment isn't clear enough. For example, some questions whose answers
> are not clear:
> 
> Which descendants are being included --- all, or just the descendants in the
> same 3D rendering context? Say that here.
> 
> Are we using the layer bounds of each descendant? Or the invalidation region
> from each descendant?

Collect layer bounds here not invalidation region.  They are improved version of OldTransformedBounds() and NewTransformedBounds().  Now, we call ComputeChange() with flags instead of calling OldTransformedBounds() and NewTransformedBounds().


> 
> Does "old"/"new" refer to the old/new transforms, or the old/new bounds, or
> both?
Both.

	Thinker Li [:sinker] Assignee
	Comment 50 • 9 years ago

--
Attachment #8714156 [details] [diff] - Attachment is obsolete: true

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 51 • 9 years ago

Comment on attachment 8715610 [details] [diff] [review]
Invalidate the forest of new and removed layers, v10

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +129,5 @@
>  struct LayerPropertiesBase : public LayerProperties
>  {
> +  /* Following flags make ComputeChange() recursively collecting
> +   * invalidation region (bounds) of descendants that participate the
> +   * same 3D rendering context.

Don't say "invalidation region" because it's not the invalidation region. Say "The following flags make ComputeChnage() recursively collect the bounds of descendants that participate in the same 3D rendering context. The bounds are added to the returned invalid region."

@@ +131,5 @@
> +  /* Following flags make ComputeChange() recursively collecting
> +   * invalidation region (bounds) of descendants that participate the
> +   * same 3D rendering context.
> +   *
> +   * The bounds of a layer with extend3d may not covers the whole

"cover"

@@ +133,5 @@
> +   * same 3D rendering context.
> +   *
> +   * The bounds of a layer with extend3d may not covers the whole
> +   * region that descendants are drawn on after being transformed.
> +   * Recursive calls are required to make sure the region is complete.

I don't understand this last sentence.

@@ +214,5 @@
> +          mLayer->GetLocalOpacity() != mOpacity ||
> +          transformChanged) {
> +        aGeometryChanged = true;
> +        AddRegion(result, OldSubtreeBounds(aCallback));
> +        AddRegion(result, NewSubtreeBounds(mLayer, aCallback));

Can't these two calls be combined into a single call to ComputeChangeInternal?

@@ +263,4 @@
>                           GetTransformForInvalidation(mLayer));
>    }
>  
>    IntRect OldTransformedBounds()

Add a comment here so its clear how this is different from OldSubtreeBounds.

Same for NewTransformedBounds.

@@ +271,5 @@
> +  /**
> +   * Collect new bounds of the subtree the given layer.
> +   *
> +   * This function would travel through the 3D rendering context of
> +   * this layer.

Say something like "... of this layer and combine the transformed bounds of its leaves".

@@ +274,5 @@
> +   * This function would travel through the 3D rendering context of
> +   * this layer.
> +   */
> +  static nsIntRegion
> +  NewSubtreeBounds(Layer* aLayer, NotifySubDocInvalidationFunc aCallback)

Can't we combine this with OldSubtreeBounds and add a flags parameter so we can compute the old and new bounds together?

@@ +361,5 @@
> +          for (Layer* child = mLayer->GetFirstChild();
> +               child;
> +               child = child->GetNextSibling()) {
> +            nsIntRegion region = NewSubtreeBounds(child, aCallback);
> +            AddRegion(result, region);

Can't we combine the two loops over the children so we call ComputeChange on each child? That would seem more efficient than using two loops.

::: layout/base/nsPresContext.cpp
@@ +2469,5 @@
> +
> +  nsRect rect(DevPixelsToAppUnits(x1),
> +              DevPixelsToAppUnits(y1),
> +              DevPixelsToAppUnits(x2 - x1),
> +              DevPixelsToAppUnits(y2 - y1));

I suggest you put this precision/clamping fix into its own separate patch.

	Thinker Li [:sinker] Assignee
	Comment 52 • 9 years ago

--
Attachment #8715610 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 54 • 9 years ago

--
Attachment #8716345 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 55 • 9 years ago

--
Attachment #8716346 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 56 • 9 years ago

--
Attachment #8716349 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 57 • 9 years ago

Comment on attachment 8716354 [details] [diff] [review]
Part 2: Invalidate the forest of new and removed layers, v13

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

Add GetSubtreeBounds() and combine loops for COMPUTE_CHANGE_COLLECT_OLD and COMPUTE_CHANGE_COLLECT_NEW.

	Thinker Li [:sinker] Assignee
	Comment 58 • 9 years ago

--
Attachment #8716354 [details] [diff] - Attachment is obsolete: true

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 59 • 9 years ago

Comment on attachment 8716350 [details] [diff] [review]
Part 1: Restrict the range of invalidation rect, v2

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

::: layout/base/nsPresContext.cpp
@@ +2459,5 @@
> +  // min value of int32_t.  The the type of return value of
> +  // DevPixelsTopAppUnits() is nscoord, int32_t or float.
> +  const int32_t minVal = AppUnitsToDevPixels(RectDouble::MaxIntRect().X());
> +  const int32_t maxVal = AppUnitsToDevPixels(RectDouble::MaxIntRect().XMost());
> +#define ADJUST_RANGE(x) std::max(std::min(x, maxVal), minVal)

Make this a static function int32_t Clamp(int32_t aValue, int32_t aMin, int32_t aMax) instead of a macro.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 60 • 9 years ago

Comment on attachment 8716365 [details] [diff] [review]
Part 2: Invalidate the forest of new and removed layers, v14

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +133,5 @@
> +   * context.  The bounds are added to the returned invalid region.
> +   *
> +   * The bounds of a layer with extend3d may not cover the whole
> +   * region that descendants are drawn on after being transformed.  It
> +   * collects the bounds recursively to make sure the result covering

covers

@@ +142,5 @@
> +  // participating the same 3D rendering context.
> +  static const int COMPUTE_CHANGE_COLLECT_OLD = 0x1;
> +  // Collect new bounds with new transforms of descendants
> +  // participating the same 3D rendering context.
> +  static const int COMPUTE_CHANGE_COLLECT_NEW = 0x2;

I'm not convinced that passing these flags to ComputeChangeInternal is the best way to implement this. Wouldn't it be simpler to have a GetOldSubtreeBounds function that computes the union of the old bounds of the old layers (recursively), and a GetNewSubtreeBounds function that computes the union of the new bounds of the new layers (recursively), and call those directly? Those functions can stop recursing when they find a layer that's the root of a 3D rendering context since in that case we can just return the (old or new) layer bounds?

Right now this patch has a mix of separate functions and flags being passed to existing functions, which doesn't seem necessary.

	Matt Woodrow (:mattwoodrow)
	Comment 63 • 9 years ago

Fixed review comments and simplified things a bit. Still passes the reftest.

Ryan, could you please check to confirm that this fixes the original test, as I can't reproduce it locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ada282e0066

	Matt Woodrow (:mattwoodrow)
	Comment 64 • 9 years ago

Comment on attachment 8718205 [details] [diff] [review]
Part 3: Compute the invalidation area for preserve-3d layers by accumulating the leaves

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +380,5 @@
>      result.OrWith(invalidOfLayer);
>  
>      return result;
>    }
> +  

Fixed whitespace locally :(

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 65 • 9 years ago

Comment on attachment 8718205 [details] [diff] [review]
Part 3: Compute the invalidation area for preserve-3d layers by accumulating the leaves

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

Yes, this looks better, thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 66 • 9 years ago

Looks great!

	Pulsebot
	Comment 67 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff5276e171f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a80ea3dbb8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5166d24e83a5

	Carsten Book [:Tomcat]
	Comment 68 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8ff5276e171f
https://hg.mozilla.org/mozilla-central/rev/1a80ea3dbb8b
https://hg.mozilla.org/mozilla-central/rev/5166d24e83a5

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 70 • 9 years ago

Awesome! Matt can you request uplift to aurora?

	Matt Woodrow (:mattwoodrow)
	Comment 71 • 9 years ago

Comment on attachment 8718205 [details] [diff] [review]
Part 3: Compute the invalidation area for preserve-3d layers by accumulating the leaves

Approval Request Comment
[Feature/regressing bug #]: Bug 1097464
[User impact if declined]: Incorrect invalidation for some dynamic 3d pages.
[Describe test coverage new/current, TreeHerder]: Manually tested, reftest added
[Risks and why]: Low risk, restricted to invalidation code.
[String/UUID change made/needed]: None

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 72 • 9 years ago

Comment on attachment 8718205 [details] [diff] [review]
Part 3: Compute the invalidation area for preserve-3d layers by accumulating the leaves

Fix for regression in 46, please uplift all 3 patches to aurora.

	Carsten Book [:Tomcat]
	Comment 73 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/58c9bc6085c4
https://hg.mozilla.org/releases/mozilla-aurora/rev/af67f21da7a2
https://hg.mozilla.org/releases/mozilla-aurora/rev/b4698646a681

	Carsten Book [:Tomcat]
	Comment 74 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/58c9bc6085c4
https://hg.mozilla.org/releases/mozilla-aurora/rev/af67f21da7a2
https://hg.mozilla.org/releases/mozilla-aurora/rev/b4698646a681

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 75 • 9 years ago

I had to back this out from aurora for reftest failures: https://treeherder.mozilla.org/logviewer.html#?job_id=1957314&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d1dec717c56

	Matt Woodrow (:mattwoodrow)
	Comment 76 • 9 years ago

That's the test that was added for this bug.

I suspect it means we're missing uplift for one of the other bugs related to preserve-3d.

There's quite a few regressions from that bug (and even more attached to some of the dep bugs).

Liz, do you have a good way of finding out which of these still haven't been uplifted?

	Thinker Li [:sinker] Assignee
	Comment 77 • 9 years ago

(In reply to Wes Kocher (:KWierso) from comment #75)
> I had to back this out from aurora for reftest failures:
> https://treeherder.mozilla.org/logviewer.html#?job_id=1957314&repo=mozilla-
> aurora

Is there any other platform having the same error?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 78 • 9 years ago

OSX, Windows and Android all failed the test. Doesn't seem to have failed on Linux, or maybe the test is skipped on Linux?

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 79 • 9 years ago

I don't have an easy way to check that, it would mean combing through all the dependencies. Looks like we need to do that though.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 80 • 9 years ago

Matt: I did try this query: https://bugzilla.mozilla.org/buglist.cgi?f1=blocked&o3=notequals&list_id=12868638&v3=fixed%2C%20verified&o1=anywordssubstr&resolution=---&o2=anyexact&query_format=advanced&f3=cf_status_firefox46&f2=cf_status_firefox47&v1=1224433%2C%201097464&v2=fixed%2C%20verified  And didn't see anything that blocks the main preserve-3d bug or this bug, that has been fixed in 47 but not fixed in 46. Can you and Thinker look again (check my logic on the query, or look again at the test failures...)

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 81 • 9 years ago

Looks like https://bugzilla.mozilla.org/show_bug.cgi?id=1245450 is not in 46 aurora.

	Matt Woodrow (:mattwoodrow)
	Comment 82 • 9 years ago

We're missing bug 1216832, which wasn't a regression but is needed for this to work.

	Carsten Book [:Tomcat]
	Comment 83 • 9 years ago

seems the other bugs have problems/conflicts. matt could you take a look, thanks!

	Matt Woodrow (:mattwoodrow)
	Comment 84 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/fb702f297604
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf3ad928b879
https://hg.mozilla.org/releases/mozilla-aurora/rev/e2f9838612ae

	Carsten Book [:Tomcat]
	Comment 85 • 8 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #84)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/fb702f297604
> https://hg.mozilla.org/releases/mozilla-aurora/rev/cf3ad928b879
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e2f9838612ae

setting flags

	Ryan VanderMeulen [:RyanVM]
	Comment 86 • 8 years ago

Verified fixed on both DevEdition and Nightly.