1224433 - A dirty bar at right side of the screen does not go until doing window resize

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Thinker Li [:sinker]]

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

Comment 1

[Thinker Li [:sinker]]

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

Comment 2

[u279076]

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

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 4

[u279076]

(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?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers

Comment 7

[Thinker Li [:sinker]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Attachment: screencast with patch applied

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.

Comment 9

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v2

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

Comment 10

[Thinker Li [:sinker]]

Attachment: debug.diff

Comment 11

[Thinker Li [:sinker]]

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

Comment 12

[Thinker Li [:sinker]]

Hi Ryan, please apply both patches.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Attachment: v2 patch log w/ frame layer dumping enabled

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.

Comment 14

[Thinker Li [:sinker]]

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.

Comment 15

[Thinker Li [:sinker]]

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.

Comment 16

[Thinker Li [:sinker]]

Attachment: debug.diff

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

Comment 17

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v3

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

Comment 18

[Thinker Li [:sinker]]

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

Comment 19

[Thinker Li [:sinker]]

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?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Looks good here!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 23

[Thinker Li [:sinker]]

(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().

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 25

[Thinker Li [:sinker]]

Attachment: debug.diff

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

Comment 26

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v4

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

Comment 27

[Thinker Li [:sinker]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Thinker Li [:sinker]]

I run into the same problem after rebasing.

Comment 30

[Thinker Li [:sinker]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 33

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v5

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

Comment 34

[Sylvestre Ledru [:Sylvestre]]

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

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

Comment 37

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v6

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

Comment 38

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v7

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

Comment 39

[Thinker Li [:sinker]]

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.

Comment 40

[Thinker Li [:sinker]]

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?

Comment 41

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v8

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

Comment 42

[Thinker Li [:sinker]]

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

Add testcase

Comment 43

[Thinker Li [:sinker]]

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

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 45

[Thinker Li [:sinker]]

(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).

Comment 46

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v9

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

Comment 47

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v9

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

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 49

[Thinker Li [:sinker]]

(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.

Comment 50

[Thinker Li [:sinker]]

Attachment: Invalidate the forest of new and removed layers, v10

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

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 52

[Thinker Li [:sinker]]

Attachment: Part 2: Invalidate the forest of new and removed layers, v11

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

Comment 53

[Thinker Li [:sinker]]

Attachment: Part 1: Restrict the range of invalidation rect

Comment 54

[Thinker Li [:sinker]]

Attachment: Part 2: Invalidate the forest of new and removed layers, v12

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

Comment 55

[Thinker Li [:sinker]]

Attachment: Part 1: Restrict the range of invalidation rect, v2

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

Comment 56

[Thinker Li [:sinker]]

Attachment: Part 2: Invalidate the forest of new and removed layers, v13

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

Comment 57

[Thinker Li [:sinker]]

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.

Comment 58

[Thinker Li [:sinker]]

Attachment: Part 2: Invalidate the forest of new and removed layers, v14

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

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 61

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add reftest

Comment 62

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Clamp the invalidation rect to values that fit within nscoord

Comment 63

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Compute the invalidation area for preserve-3d layers by accumulating the leaves

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

Comment 64

[Matt Woodrow (:mattwoodrow)]

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 :(

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Looks great!

Comment 67

[Pulsebot]

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

Comment 68

[Carsten Book [:Tomcat]]

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

Comment 70

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Awesome! Matt can you request uplift to aurora?

Comment 71

[Matt Woodrow (:mattwoodrow)]

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

Comment 72

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 73

[Carsten Book [:Tomcat]]

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

Comment 74

[Carsten Book [:Tomcat]]

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

Comment 75

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

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

Comment 76

[Matt Woodrow (:mattwoodrow)]

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?

Comment 77

[Thinker Li [:sinker]]

(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?

Comment 78

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

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

Comment 79

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 80

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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...)

Comment 81

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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

Comment 82

[Matt Woodrow (:mattwoodrow)]

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

Comment 83

[Carsten Book [:Tomcat]]

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

Comment 84

[Matt Woodrow (:mattwoodrow)]

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

Comment 85

[Carsten Book [:Tomcat]]

(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

Comment 86

[Ryan VanderMeulen [:RyanVM]]

Verified fixed on both DevEdition and Nightly.