Closed Bug 1224433 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox43 --- unaffected
firefox44 --- unaffected
firefox45 + unaffected
firefox46 + verified
firefox47 --- verified

People

(Reporter: sinker, Assigned: sinker)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(5 files, 20 obsolete files)

(deleted), video/webm
Details
(deleted), application/x-zip-compressed
Details
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
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
Depends on: 1097464
Depends on: 1214212
No longer depends on: 1097464
I cannot reproduce this locally. Do you have any more details?
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.
(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?
Whiteboard: [gfx-noted]
It's a regression from bug 1097464, so probably.
Attached patch Invalidate the forest of new and removed layers (obsolete) (deleted) — Splinter Review
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.
Flags: needinfo?(ryanvm)
Attached video screencast with patch applied (deleted) —
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.
Flags: needinfo?(ryanvm)
--
Attachment #8687807 [details] [diff] - Attachment is obsolete: true
Attachment #8687807 - Attachment is obsolete: true
Attached patch debug.diff (obsolete) (deleted) — Splinter Review
Hi Ryan, need your help to get a log for the latest patch.  Thank you!
Flags: needinfo?(ryanvm)
Hi Ryan, please apply both patches.
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.
Flags: needinfo?(ryanvm)
Blocks: 1224976
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.
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.
Attached patch debug.diff (obsolete) (deleted) — Splinter Review
--
Attachment #8689408 [details] [diff] - Attachment is obsolete: true
Attachment #8689408 - Attachment is obsolete: true
--
Attachment #8689407 [details] [diff] - Attachment is obsolete: true
Attachment #8689407 - Attachment is obsolete: true
Ryan, according to comment 15, the latest patch fixes the problem at my side.  Please, check it!
Flags: needinfo?(ryanvm)
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?
Attachment #8690700 - Flags: feedback?(roc)
Comment on attachment 8690700 [details] [diff] [review]
Invalidate the forest of new and removed layers, v3

Looks good here!
Flags: needinfo?(ryanvm)
Attachment #8690700 - Flags: feedback+
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.
Attachment #8690700 - Flags: feedback+ → feedback-
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?
Attachment #8690700 - Flags: feedback?(roc)
(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().
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.
Attached patch debug.diff (obsolete) (deleted) — Splinter Review
--
Attachment #8690699 [details] [diff] - Attachment is obsolete: true
--
Attachment #8690700 [details] [diff] - Attachment is obsolete: true
Attachment #8690699 - Attachment is obsolete: true
Attachment #8690700 - Attachment is obsolete: true
Ryan, this patch seems not laggy.  Could you check it? Thank you!
Flags: needinfo?(ryanvm)
With this patch, the cube doesn't render at all for me (just an empty grey viewport with the text across the top).
Flags: needinfo?(ryanvm)
I run into the same problem after rebasing.
I just find that even the code in m-c it don't show the cube.
Filed bug 1230780 for the new issue with the cube not rendering at all.
Depends on: 1230780
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.
--
Attachment #8693161 [details] [diff] - Attachment is obsolete: true
Attachment #8693161 - Attachment is obsolete: true
Tracking as we don't want to release 45 with an obvious visual regression.
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.
Attachment #8703394 - Attachment is obsolete: true
Confirmed that this no longer reproduces on 45 since the backout landed.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d
--
Attachment #8703394 [details] [diff] - Attachment is obsolete: true
--
Attachment #8713611 [details] [diff] - Attachment is obsolete: true
Attachment #8713611 - Attachment is obsolete: true
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.
Attachment #8713656 - Flags: review?(roc)
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?
--
Attachment #8713656 [details] [diff] - Attachment is obsolete: true
Attachment #8713656 - Attachment is obsolete: true
Attachment #8713656 - Flags: review?(roc)
Comment on attachment 8713693 [details] [diff] [review]
Invalidate the forest of new and removed layers, v8

Add testcase
Attachment #8713693 - Flags: review?(roc)
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?
Attachment #8713693 - Flags: review?(roc)
(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).
--
Attachment #8713693 [details] [diff] - Attachment is obsolete: true
Attachment #8714154 - Flags: review?(roc)
Attachment #8713693 - Attachment is obsolete: true
--
Attachment #8714154 [details] [diff] - Attachment is obsolete: true
Attachment #8714156 - Flags: review?(roc)
Attachment #8714154 - Attachment is obsolete: true
Attachment #8714154 - Flags: review?(roc)
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?
Attachment #8714156 - Flags: review?(roc) → review-
(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.
--
Attachment #8714156 [details] [diff] - Attachment is obsolete: true
Attachment #8715610 - Flags: review?(roc)
Attachment #8714156 - Attachment is obsolete: true
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.
Attachment #8715610 - Flags: review?(roc)
--
Attachment #8715610 [details] [diff] - Attachment is obsolete: true
Attachment #8716345 - Flags: review?(roc)
Attachment #8715610 - Attachment is obsolete: true
Attached patch Part 1: Restrict the range of invalidation rect (obsolete) (deleted) — Splinter Review
Attachment #8716346 - Flags: review?(roc)
--
Attachment #8716345 [details] [diff] - Attachment is obsolete: true
Attachment #8716349 - Flags: review?(roc)
--
Attachment #8716346 [details] [diff] - Attachment is obsolete: true
Attachment #8716350 - Flags: review?(roc)
Attachment #8716345 - Attachment is obsolete: true
Attachment #8716345 - Flags: review?(roc)
Attachment #8716346 - Attachment is obsolete: true
Attachment #8716346 - Flags: review?(roc)
--
Attachment #8716349 [details] [diff] - Attachment is obsolete: true
Attachment #8716354 - Flags: review?(roc)
Attachment #8716349 - Attachment is obsolete: true
Attachment #8716349 - Flags: review?(roc)
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.
--
Attachment #8716354 [details] [diff] - Attachment is obsolete: true
Attachment #8716365 - Flags: review?(roc)
Attachment #8716354 - Attachment is obsolete: true
Attachment #8716354 - Flags: review?(roc)
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.
Attachment #8716350 - Flags: review?(roc)
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.
Attachment #8716365 - Flags: review?(roc)
Attached patch Part 1: Add reftest (deleted) — Splinter Review
Attachment #8716350 - Attachment is obsolete: true
Attachment #8716365 - Attachment is obsolete: true
Attachment #8718203 - Flags: review?(roc)
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
Attachment #8693160 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
Attachment #8718205 - Flags: review?(roc)
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 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!
Attachment #8718205 - Flags: review?(roc) → review+
Looks great!
Flags: needinfo?(ryanvm)
Awesome! Matt can you request uplift to aurora?
Flags: needinfo?(matt.woodrow)
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
Flags: needinfo?(matt.woodrow)
Attachment #8718205 - Flags: approval-mozilla-aurora?
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.
Attachment #8718205 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(lhenry)
(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?
Flags: needinfo?(tlee)
OSX, Windows and Android all failed the test. Doesn't seem to have failed on Linux, or maybe the test is skipped on Linux?
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.
Flags: needinfo?(lhenry)
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...)
Flags: needinfo?(matt.woodrow)
We're missing bug 1216832, which wasn't a regression but is needed for this to work.
Depends on: 1216832
Flags: needinfo?(matt.woodrow)
seems the other bugs have problems/conflicts. matt could you take a look, thanks!
Flags: needinfo?(matt.woodrow)
Verified fixed on both DevEdition and Nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: