Seth Fowler [:seth] [:s2h] Assignee
	Description • 9 years ago

We really want the advantages of downscale-during-decode without the negative aspects of decode-on-draw such as image pop-in. To do that, we have to be able to accurately predict the size of images without drawing them.

In this bug I'll implement size prediction for nsImageFrame, which will take care of this issue for <img> elements.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 1 • 9 years ago

Here's the patch.

It adds a new method, MaybeDecodeForPredictedSize(), to nsImageFrame. We call it
in situations where we should be ready to request a decode.
MaybeDecodeForPredictedSize() computes the expected size of the image in screen
coordinates. (Ideally we'd use layer coordinates, but it doesn't seem like we
can't compute those as early as we want to, and the two numbers are the same in
most situations.)

MaybeDecodeForPredictedSize() needed to do some of the same computation as
nsDisplayImage::GetDestRect(), so I moved most of that code into
nsImageFrame::PredictedDestRect().

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 2 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 3 • 9 years ago

Here's an updated version of the patch. I realized when working on bug 1151373
that we need to call GetOptimalImageSizeForDest() as well.

	Timothy Nikkel (:tnikkel)
	Comment 4 • 9 years ago

When an image's scale is being animated we round up the scale to the nearest power of two and paint at that scale. So this prediction will go wrong in that case. We should avoid asking for a useless size in this case as it could have significant negative performance implications. While we are computing the scale induced by ancestor with transforms can we also check if any of them are active in a way that ChooseScaleAndSetTransform would give a rounded up scale size?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #4)
> When an image's scale is being animated we round up the scale to the nearest
> power of two and paint at that scale. So this prediction will go wrong in
> that case. We should avoid asking for a useless size in this case as it
> could have significant negative performance implications. While we are
> computing the scale induced by ancestor with transforms can we also check if
> any of them are active in a way that ChooseScaleAndSetTransform would give a
> rounded up scale size?

Yeah, I was aware of that limitation, but I wasn't sure how to detect that case. Do you know how I can check for it?

	Timothy Nikkel (:tnikkel)
	Comment 6 • 9 years ago

GetTransformToAncestorScale uses GetTransformToAncestor to get the scale. You could make a version of GetTransformToAncestor that when it encounters a transform checks if it has HasAnimationsForCompositor or an actively animated transform like ChooseScaleAndSetTransform does.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 7 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #6)
> GetTransformToAncestorScale uses GetTransformToAncestor to get the scale.
> You could make a version of GetTransformToAncestor that when it encounters a
> transform checks if it has HasAnimationsForCompositor or an actively
> animated transform like ChooseScaleAndSetTransform does.

OK, thanks. I'll investigate that tomorrow and add a second patch that handles that case. (So you can still review the current patch as "part 1" if you want.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 8 • 9 years ago

Comment on attachment 8588492 [details] [diff] [review]
Predict the size of nsImageFrame images before drawing

Ack, 'git bz attach' didn't mark the old patch as obsolete.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 9 • 9 years ago

Updated commit message.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 10 • 9 years ago

Here's part 2. This part adds
nsLayoutUtils::GetTransformToAncestorScaleExcudingAnimated(). This new function
always returns the identity scale factors if some frame on the path to the
display root is subject to animation.

I think this is a good first cut for the animated case: just make sure that the
image is decoded at its intrinsic size. <img> elements which are animated
directly could be layerized, and in that case we'll paint them at the intrinsic
size anyway. Even for <img>'s which aren't layerized, I don't think that
predicting an "optimal" scale is a good idea here, because doing it accurately
seems tricky and brittle (i.e., changes in layer rasterization policy could
easily make the optimal scale non-optimal), and considering that during an
animation any such optimal scale could become out-of-date rapidly. So if we want
to get smarter than this, I think we should probably leave that for a followup.
(But let me know if I'm missing something!)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 11 • 9 years ago

So I got a chance to test part 2 on a B2G device today; I had previously only tested part 1. The approach in part 2 isn't going to cut it, because it's really important for the Gallery app in particular that we predict the drawn size accurately for images with animations.

I think what's actually important here is that we return the identity scale from GetTransformToAncestorScaleExcludingAnimated() if some ancestor has an animation or transition that *affects the scale*. I'm going to upload a new version of part 2 that works that way. There shouldn't be any problem with animations or transitions that only affect things like translation or opacity, and we really need to support them.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 12 • 9 years ago

Here's the new version of part 2.

	Timothy Nikkel (:tnikkel)
	Comment 13 • 9 years ago

Comment on attachment 8589390 [details] [diff] [review]
(Part 1) - Predict the size of nsImageFrame images before drawing

> void
> nsImageLoadingContent::IncrementVisibleCount()
> {
>   mVisibleCount++;
>   if (mVisibleCount == 1) {
>+    // Since we're becoming visible, request a decode.
>+    nsImageFrame* f = do_QueryFrame(GetOurPrimaryFrame());
>+    if (f) {
>+      f->MaybeDecodeForPredictedSize();
>+    }
>+
>     TrackImage(mCurrentRequest);
>     TrackImage(mPendingRequest);
>   }
> }

Would it be better to do this in nsImageLoadingContent::TrackImage?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 14 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #13)
> Would it be better to do this in nsImageLoadingContent::TrackImage?

Could be! The main thing is to ensure that we only do this when mVisibleCount == 1 (so we just became visible), to avoid unnecessary calls to MaybeDecodeForPredictedSize(), but it seems like we could just as easily make that check in TrackImage.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 15 • 9 years ago

This bitrotted a little so it needed a rebase.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 16 • 9 years ago

This one also bitrotted.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 17 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f316752e293

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 18 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 19 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=384ab0d3898c

	Timothy Nikkel (:tnikkel)
	Comment 20 • 9 years ago

Comment on attachment 8604983 [details] [diff] [review]
(Part 1) - Predict the size of nsImageFrame images before drawing

> nsImageLoadingContent::IncrementVisibleCount()
> {
>   mVisibleCount++;
>   if (mVisibleCount == 1) {
>+    // Since we're becoming visible, request a decode.
>+    nsImageFrame* f = do_QueryFrame(GetOurPrimaryFrame());
>+    if (f) {
>+      f->MaybeDecodeForPredictedSize();
>+    }
>+

I think I mentioned earlier, this would be better in TrackImage. TrackImage checks if the image has a frame too, so we won't end up decoding an image that never gets drawn.

We call MaybeDecodeForPredictedSize from nsImageFrame::Reflow. MaybeDecodeForPredictedSize checks if imageLoader->GetVisibleCount() == 0. But one important way that the visible count gets increment is from a reflow callback posted in nsImageFrame::Reflow, so it won't run until the full reflow pass is done. So we should probably do the MaybeDecodeForPredictedSize call in the reflow callback (if we are going to have a reflow callback).

>@@ -928,16 +1012,20 @@ nsImageFrame::Reflow(nsPresContext*     
>                            nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+2*(ICON_PADDING+ALT_BORDER_WIDTH)),
>                            nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+2*(ICON_PADDING+ALT_BORDER_WIDTH)));
>     // We include the altFeedbackSize in our visual overflow, but not in our
>     // scrollable overflow, since it doesn't really need to be scrolled to
>     // outside the image.
>     static_assert(eOverflowType_LENGTH == 2, "Unknown overflow types?");
>     nsRect& visualOverflow = aMetrics.VisualOverflow();
>     visualOverflow.UnionRect(visualOverflow, altFeedbackSize);
>+  } else {
>+    // We've just reflowed and we should have an accurate size, so we're ready
>+    // to request a decode.
>+    MaybeDecodeForPredictedSize();
>   }

Hmm, it would be good if we could also decode for predicted size if we are (currently) showing the altfeedback. If we have the image's intrinsic size then the image frame should be reflowed at that size correct? So that case should be fine. If we don't have the image's intrinsic size yet then either the size of the image frame is determined by it's style (so we know the final size) or it's determined by the instrinic size. In this last case is it possible that the size of the image frame won't be the instrinic size of the image? (So we should just ask for the intrinsize size decode.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 21 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #20)
> I think I mentioned earlier, this would be better in TrackImage. TrackImage
> checks if the image has a frame too, so we won't end up decoding an image
> that never gets drawn.

OK, I'll move it.

> We call MaybeDecodeForPredictedSize from nsImageFrame::Reflow.
> MaybeDecodeForPredictedSize checks if imageLoader->GetVisibleCount() == 0.
> But one important way that the visible count gets increment is from a reflow
> callback posted in nsImageFrame::Reflow, so it won't run until the full
> reflow pass is done. So we should probably do the
> MaybeDecodeForPredictedSize call in the reflow callback (if we are going to
> have a reflow callback).

Are you saying that we should replace the MaybeDecodeForPredictedSize() call in nsImageFrame::Reflow() with a call to MaybeDecodeForPredictedSize() in the reflow callback?

My intention was that calling MaybeDecodeForPredictedSize() in Reflow() covered the case where an existing visible image's size changes due to a style or DOM change, while the call in IncrementVisibleCount() covered the case where a previously non-visible image has become visible. I confess I'm not familiar with reflow callbacks at all (sounds like they run after the entire layout pass is done?), so it's not obvious to me whether the reflow callback would cover some additional cases or handle the existing ones better.

> >@@ -928,16 +1012,20 @@ nsImageFrame::Reflow(nsPresContext*     
> >                            nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+2*(ICON_PADDING+ALT_BORDER_WIDTH)),
> >                            nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+2*(ICON_PADDING+ALT_BORDER_WIDTH)));
> >     // We include the altFeedbackSize in our visual overflow, but not in our
> >     // scrollable overflow, since it doesn't really need to be scrolled to
> >     // outside the image.
> >     static_assert(eOverflowType_LENGTH == 2, "Unknown overflow types?");
> >     nsRect& visualOverflow = aMetrics.VisualOverflow();
> >     visualOverflow.UnionRect(visualOverflow, altFeedbackSize);
> >+  } else {
> >+    // We've just reflowed and we should have an accurate size, so we're ready
> >+    // to request a decode.
> >+    MaybeDecodeForPredictedSize();
> >   }
> 
> Hmm, it would be good if we could also decode for predicted size if we are
> (currently) showing the altfeedback. If we have the image's intrinsic size
> then the image frame should be reflowed at that size correct? So that case
> should be fine. If we don't have the image's intrinsic size yet then either
> the size of the image frame is determined by it's style (so we know the
> final size) or it's determined by the instrinic size. In this last case is
> it possible that the size of the image frame won't be the instrinic size of
> the image? (So we should just ask for the intrinsize size decode.)

This is a good point, but unfortunately we can't benefit from this, because right now we won't actually do *any* full decode until the size is available, and we don't have a mechanism for saving sizes that were requested. (We just have an |mWantFullDecode| boolean.) I think we should look into this, though; I'll file a followup about supporting RequestDecodeForSize() before the intrinsic size is available, and then we can take advantage of it in this case.

	Timothy Nikkel (:tnikkel)
	Comment 22 • 9 years ago

Comment on attachment 8604985 [details] [diff] [review]
(Part 2) - Treat nsImageFrames subject to scale animation as having an identity scale when predicting size

Can you reuse the same machinery that ChooseScaleAndSetTransform in FrameLayerBuilder uses to actually compute the scale that things will be drawn at? ComputeSuitableScaleForAnimation might even allow us to choose the right scale when the scale is being animated, but that can be a followup bug.

But also importantly using ActiveLayerTracker::IsStyleAnimated will catch cases where the scale is being animated by javascript.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 23 • 9 years ago

Some relevant IRC discussion:

<seth>	tn: so re: bug 1151359 comment 22
<seth>	tn: the problem with using ChooseScaleAndSetTransform or ActiveLayerTracker::IsStyleAnimated is that we don't have e.g. an nsDisplayListBuilder
<seth>	tn: also, IsStyleAnimated checks HasCurrentAnimationsForProperties, but i think we want to be more conservative and not try to predict anything if the frame is subject to a CSS animation/transition at all, even if it's not playing currently
<seth>	tn: it really would be nice to not duplicate similar code in a bunch of places, though
<@tn>	seth, yeah we can't use ChooseScaleAndSetTransform directly,  but the builder argument is allowed to be null for IsStyleAnimated
<@tn>	seth, i think we definitely want to detect javascript driven animations
<@tn>	which is the LayerActivity bit of IsStyleAnimated
<seth>	tn: do we need to check that all the way up the frame tree?
<@tn>	yeah, i guess so
<seth>	tn: ok. so maybe what i could do is add an argument to ActiveLayerTracker::IsStyleAnimated to let callers specify whether we want to check current animations or all animations
<seth>	tn: and then walk up the tree and check IsStyleAnimated(..., eCSSProperty_transform)
<seth>	tn: so that'd replace HasAnimatedScale and ContainsAnimatedScale in the patch
<seth>	tn: sound good?
<seth>	tn: oh no, wait
<seth>	tn: that doesn't work!
<seth>	tn: the problem being that it's absolutely critical that we support translations
<seth>	tn: it looks like generally LayerActivityTracker doesn't support this. we could add an additional counter for transforms that include a non-identity scale, though
<seth>	tn: so i think what we'd end up with is: move HasAnimatedScale into ActiveLayerTracker, add an additional counter to LayerActivityTracker for scale changes, and check that counter in HasAnimatedScale() in addition to the other checks

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 24 • 9 years ago

In this new version of part 1, we call MaybeDecodeForPredictedSize() from
TrackImage() instead of from IncrementVisibleCount().

As per our discussions on IRC, the situation remains the same when we reflow
already-visible frames: we call MaybeDecodeForPredictedSize() in
nsImageFrame::Reflow().

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 25 • 9 years ago

This version of part 2, which has grown substantially since the last version,
adds ActiveLayerTracker::IsScaleSubjectToAnimation(), which detects both
potential animation of scale due to CSS animations and transitions, and
animation of scale due to JavaScript. The latter is achieved by adding a counter
just for scale changes to LayerActivity.

We actually detect the scale changes in nsStyleDisplay::CalcDifference(), where
we call SameScaleFactors() when a difference in transforms is detected to
determine if the scale factors have changed. If they have, we dispatch a new
change hint, nsChangeHint_UpdateTransformScale, which ultimately causes
ActiveLayerTracker::NotifyTransformScaleRestyle() to be called, which records
the scale change on LayerActivity.

I did want to respond as well to the point about using the same algorithm as
ChooseScaleAndSetTransform (presumably you mean for the calculations in
MaybeDecodeForPredictedSize). It's not possible to call that code directly,
because we're doing this stuff before display list construction even begins. I
don't think it's great to have similar computations in multiple places, though,
so in the long term it'd be ideal to share as much code as possible between
these calculations. I think that requires some refactoring that's out of the
scope of this bug, though.

The existing MaybeDecodeForPredictedSize algorithm is based on code that until
recently we used in nsDisplayList.cpp for some APZ-related calculations (IIRC),
pointed out to me by Botond. That code seems to have moved since I originally
posted the patch and I'm not having an easy time finding it, so unfortunately I
can't point to it. But in my local tests, at least, this code is 100% accurate
when animation isn't involved - I haven't encountered any situations where it
computed a size different than the one we ended up drawing at.

	Timothy Nikkel (:tnikkel)
	Comment 26 • 9 years ago

Comment on attachment 8606151 [details] [diff] [review]
(Part 1) - Predict the size of nsImageFrame images before drawing

I think it would probably be a good idea to also pass FLAG_ASYNC_NOTIFY to RequestDecodeForSize since we call this during reflow and potentially other things.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 27 • 9 years ago

(In reply to Timothy Nikkel (:tn) from comment #26)
> Comment on attachment 8606151 [details] [diff] [review]
> (Part 1) - Predict the size of nsImageFrame images before drawing
> 
> I think it would probably be a good idea to also pass FLAG_ASYNC_NOTIFY to
> RequestDecodeForSize since we call this during reflow and potentially other
> things.

Thanks for the review!

I think that change sounds like a good idea. I'll update the patch.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 28 • 9 years ago

Here's the updated patch.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 29 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f2c0562c2e4

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 30 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 31 • 9 years ago

So the test failures above are all triggered by the fact that it's kinda tricky to call nsDisplayTransform::GetResultingTransformMatrix without passing in a frame. I'll fix that in the next revision of the patch. As far as I can tell, this is intended to work, but I suspect that code path isn't exercised right now.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 32 • 9 years ago

Comment on attachment 8606154 [details] [diff] [review]
(Part 2) - Treat nsImageFrames subject to scale animation as having an identity scale when predicting size

Brian, can you take a look at this as well? In particular it'd be great if you could evaluate all the stuff related to animation in this patch.

	Brian Birtles (:birtles)
	Comment 33 • 9 years ago

Comment on attachment 8606154 [details] [diff] [review]
(Part 2) - Treat nsImageFrames subject to scale animation as having an identity scale when predicting size

>diff --git a/layout/base/ActiveLayerTracker.cpp b/layout/base/ActiveLayerTracker.cpp
>--- a/layout/base/ActiveLayerTracker.cpp
>+++ b/layout/base/ActiveLayerTracker.cpp
>@@ -313,16 +322,86 @@ ActiveLayerTracker::IsOffsetOrMarginStyl
>   // We should also check for running CSS animations of these properties once
>   // bug 1009693 is fixed. Until that happens, layerization isn't useful for
>   // animations of these properties because we'll invalidate the layer contents
>   // on every change anyway.
>   // See bug 1151346 for a patch that adds a check for CSS animations.
>   return false;
> }
> 
>+// A helper function for IsScaleSubjectToAnimation which returns true if the
>+// given AnimationCollection contains an effect that animates scale.
>+static bool
>+ContainsAnimatedScale(AnimationCollection* aCollection, nsIFrame* aFrame)
>+{
>+  if (!aCollection) {
>+    return false;
>+  }
>+
>+  for (dom::Animation* anim : aCollection->mAnimations) {
>+    if (!anim->GetEffect() || anim->GetEffect()->IsFinishedTransition()) {
>+      continue;
>+    }

I'm a bit unsure which scale animations you're interested in. In comment 23 you said you are interested in more than just current animations. Currently, however, this code will return true for CSS transitions that have finished as well as CSS animations that have finished (even if they *don't* fill forwards). In future, it will also include any animations created by the animation API even after they are cancelled. So I'm pretty sure we don't want that.

Just to recap:

* "current" = playing, paused, waiting to start (i.e. in the delay phase) -- pretty much everything except finished/cancelled animations
* "in effect" = playing, paused, filling (backwards or forwards) -- pretty much everything except waiting to start (and not backwards filling), finished (and not forwards filling), cancelled animations
* "relevant" = "current" OR "in effect" = playing, paused, waiting to start, filling (i.e. NOT finished transitions or finished animations that don't fill forwards)

So "current" might be enough unless you are concerned about animations that are filling forwards. In that case, you probably want "relevant". So you probably want to change ContainsAnimatedScale to skip if !anim->IsRelevant() (or HasCurrentEffect() if "current" is enough).

Note that both IsRelevant() and HasCurrentEffect() won't return true if there's no effect or if the effect is a finished transition so you could drop the following check:

  if (!anim->GetEffect() || anim->GetEffect()->IsFinishedTransition()) {
    continue;
  }

And just have:

  if (!anim->IsRelevant()) {
    continue;
  }

>diff --git a/layout/style/StyleAnimationValue.cpp b/layout/style/StyleAnimationValue.cpp
>--- a/layout/style/StyleAnimationValue.cpp
>+++ b/layout/style/StyleAnimationValue.cpp
>@@ -3521,16 +3522,53 @@ StyleAnimationValue::ExtractComputedValu
>       return true;
>     }
>     case eStyleAnimType_None:
>       NS_NOTREACHED("shouldn't use on non-animatable properties");
>   }
>   return false;
> }
> 
>+gfxSize
>+StyleAnimationValue::GetScale(nsIFrame* aForFrame) const
>+{
>+  if (!aForFrame) {
>+    NS_WARNING("No frame.");
>+    return gfxSize();
>+  }
>+  if (GetUnit() != StyleAnimationValue::eUnit_Transform) {
>+    NS_WARNING("Expected a transform.");
>+    return gfxSize();
>+  }

I know you're just moving code here but I notice that we have existing accessors here that end in ...Value() and test the type as follows:

  NS_ASSERTION(IsCSSValueSharedListValue(mUnit), "unit mismatch");

Are we actually expecting this to be called on a StyleAnimationValue that might not be a transform value? Or without a frame? If not, what do you think about making this something like:

gfxSize
StyleAnimationValue::GetScaleValue(nsIFrame* aForFrame) const
{
  MOZ_ASSERT(IsCSSValueSharedListValue(mUnit), "unit mismatch");
  MOZ_ASSERT(aForFrame, "No frame");
  ...

?

In fact, the call to GetCSSValueSharedListValue() later in this method includes the first assertion so we could drop it here.

r=me for the animation parts with those comments addressed

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 34 • 9 years ago

Thanks for the review!

(In reply to Brian Birtles (:birtles) from comment #33)
> * "current" = playing, paused, waiting to start (i.e. in the delay phase) --
> pretty much everything except finished/cancelled animations

Oh dear - I thought "current" meant only animations that were currently playing! You're right, "current" is definitely what I want. I'll update the patch, and make the other changes you recommended as well.

> Are we actually expecting this to be called on a StyleAnimationValue that
> might not be a transform value? Or without a frame?

I'll double check but I'm pretty sure that's right - we should never hit those error cases. I'll make those changes too.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 35 • 9 years ago

I ended up adding a huge hunk of code to nsStyleTransformMatrix in my latest local version of the patch. Before updating the patch here, I want to run a try job to make sure that that huge hunk of code works right. =) If so, expect an updating version of the patch that does a much better job of estimating the scale on transform property values, either later tonight or tomorrow. And I'll include the changes Brian recommended as well.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 36 • 9 years ago

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

	Brian Birtles (:birtles)
	Comment 37 • 9 years ago

(In reply to Seth Fowler [:seth] from comment #34)
> (In reply to Brian Birtles (:birtles) from comment #33)
> > * "current" = playing, paused, waiting to start (i.e. in the delay phase) --
> > pretty much everything except finished/cancelled animations
> 
> Oh dear - I thought "current" meant only animations that were currently
> playing! You're right, "current" is definitely what I want. I'll update the
> patch, and make the other changes you recommended as well.

Just remember, if you want to cover animations that have finished but are still filling, like the following, you want "relevant":

div {
  animation: scaleUp 1s forwards; /* scales up and STAYS scaled up */
}
@keyframes {
  to { transform: scale(2); }
}

	Brian Birtles (:birtles)
	Comment 38 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #37)
> (In reply to Seth Fowler [:seth] from comment #34)
> > (In reply to Brian Birtles (:birtles) from comment #33)
> > > * "current" = playing, paused, waiting to start (i.e. in the delay phase) --
> > > pretty much everything except finished/cancelled animations
> > 
> > Oh dear - I thought "current" meant only animations that were currently
> > playing! You're right, "current" is definitely what I want. I'll update the
> > patch, and make the other changes you recommended as well.
> 
> Just remember, if you want to cover animations that have finished but are
> still filling, like the following, you want "relevant":
> 
> div {
>   animation: scaleUp 1s forwards; /* scales up and STAYS scaled up */
> }
> @keyframes {
>   to { transform: scale(2); }
> }

Oops, forgot the keyframes name:

@keyframes scaleUp { ... }

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 39 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #37)
> Just remember, if you want to cover animations that have finished but are
> still filling, like the following, you want "relevant":

Thanks for clarifying the meaning of "relevant" - it's clearer to me now what it means for an animation to be still "filling". That's helpful.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 40 • 9 years ago

OK, detecting scale changes due to the |transform| property really robustly
involved more code than I initially expected. =) I'm adding a new part 2 and
part 3; the original part 2 will become part 4.

This patch adds nsStyleTransformMatrix::EstimateTransforms(), which works like
nsStyleTransformMatrix::ReadTransforms() except that it produces an estimated
value when calculating the actual value would require knowing the particular
frame or style context to which the transform is being applied. For most common
cases, it produces the exact same output as ReadTransforms(). Percent units are
calculated against an arbitrary (but always square) reference box passed in by
the caller. Relative length units like em's are evaluated against the document's
initial font. Calc() expressions are simplified away totally.

The work of simplifying the transform value is performed recursively by the
TransformSimplifier class. There is a bit more explanation in the comments in
the code; basically, the goal of TransformSimplifier is to rewrite the transform
CSS value into something which does not need a style context or frame to be
evaluated. In the common case, nothing needs to be rewritten at all;
TransformSimplifier tries to minimize the amount of rewriting it does to reduce
time spent allocating memory and copying.

It's probably worth addressing the question: why not simplify modify
ReadTransforms() to let us evaluate transform properties without a style context
or frame? The answer is that I tried to do that before I decided to go this
route, and it ends up being a bit of a mess, and the mess spills over into
nsRuleNode. We'd need to do significant refactoring to make it possible to do
that cleanly, and I'd like to avoid doing that in this bug.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 41 • 9 years ago

This part uses the new code added in part 2 to detect transform property changes
that affect scale.

Much of this code was present in the old part 2, which has already been
reviewed; mainly I want to draw your attention to the code in nsStyleStruct.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 42 • 9 years ago

Part 4 is the old part 2, minus some code that moved into part 3. Nothing has
changed here except that I've addressed Brian's review comments, so this doesn't
need to be re-reviewed.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 43 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 44 • 9 years ago

Comment on attachment 8610956 [details] [diff] [review]
(Part 2) - Make it possible to estimate the matrix the transform property generates, even without a frame

Switching the r? on the new style-related stuff to David.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 45 • 9 years ago

Comment on attachment 8610959 [details] [diff] [review]
(Part 3) - Add style system support for detecting transform property changes that affect scale

I'll go ahead and switch this one as well. Everything in this patch has already been r+'d by Timothy and Brian except for the changes in nsStyleStruct, which have been rewritten in this version of the patch. That means that the remaining code to review here is all style code.

	David Baron :dbaron:
	Comment 46 • 9 years ago

I'm curious how accurate you need this prediction to be.  e.g., does it need to take into account scaling that's the result of 3-D transforms?  What goes wrong if it's a little off, or a lot off?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 47 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #46)
> I'm curious how accurate you need this prediction to be.

If it's not accurate, it's a performance issue: we produce a scaled version of the image that we won't end up using. But it'd never affect correctness.

There's one situation in particular where, at least for now, we're deliberating making it inaccurate: rather than trying to be smart about animations that involve scaling, we just decode to the intrinsic size of the image in the animated case. If the image ends up layerized due to the animation, that's exactly what we want, and if not, at least we can scale the intrinsically sized version of the image down to whatever we actually need.

> e.g., does it need
> to take into account scaling that's the result of 3-D transforms?

The scale we're computing should be the same as the size of the layer that we apply the 3D transform to, so it should work fine. For the specific case of JavaScript manually animating the |transform| property, if 3D transforms are involved, for now we just assume that the scale is changing.

> What goes wrong if it's a little off, or a lot off?

The worst case scenario, as mentioned above, is that we decode a version of the image at the wrong size and have to decode again at some point. Purely a perf issue.

	David Baron :dbaron:
	Comment 48 • 9 years ago

Could you explain why you want TransformSimplifier rather than making MatrixForTransformFunction allow those parameters to be null in the general case by performing some estimation when they're null?  (It seems like that would be substantially less code, I think.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 49 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #48)
> Could you explain why you want TransformSimplifier rather than making
> MatrixForTransformFunction allow those parameters to be null in the general
> case by performing some estimation when they're null?  (It seems like that
> would be substantially less code, I think.)

I tried that first, before I went with this approach, and it proved to be more complicated in practice. Once you've modified everything in nsStyleTransformMatrix, you're just getting started - you have to modify a lot of code in nsRuleNode as well. And you end up weakening a lot of checks and invariants in nsRuleNode.

I do think that in the long term, it'd be preferable to refactor the code in nsRuleNode and nsStyleTransformMatrix to support this use-case better. But I'd prefer to wait for a followup to do that, because it will touch a lot of shared code that could cause regressions, and I don't want backouts over those kinds of issues to stop the code we're adding in this bug from working. Especially as B2G starts to rely on downscale-during-decode, backing out these basic patches could cause substantial memory usage regressions. So it's nice to compartmentalize things, at least at first - if there are bugs in TransformSimplifier, it'll only affect this particular functionality.

	David Baron :dbaron:
	Comment 50 • 9 years ago

Comment on attachment 8610959 [details] [diff] [review]
(Part 3) - Add style system support for detecting transform property changes that affect scale

Why does this new hint need to be so precise (i.e., not happen in cases
where the estimated scale isn't changing)?  In other words, why do we
need all this code to avoid executing some other code (which could
presumably bail out reasonably quickly) in cases where the transform
changes in ways that do not affect scale?  (We're already producing an
UpdateTransformLayer hint; presumably where we handle that it's easy
enough to get the matrix in the normal way.)  Or, to put it another way,
why do you want this new hint rather than a test within the code that
handles the existing UpdateTransformLayer hint -- which also has access
to the data to get the correct scale?

Probably needs more nsChangeHint.h changes:  I think you need to add
the new hint to nsChangeHint_Hints_NotHandledForDescendants and
NS_HintsNotHandledForDescendantsIn.

Why do you not need to produce this hint when we're changing from
no-transform to having a transform, or vice-versa?

Sorry for the questions-over-time here; it's taken me a bit of time to
understand what you're trying to do (and I still don't think I get all
of it).

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 51 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #50)
> Comment on attachment 8610959 [details] [diff] [review]
> (Part 3) - Add style system support for detecting transform property changes
> that affect scale
> 
> Why does this new hint need to be so precise (i.e., not happen in cases
> where the estimated scale isn't changing)?  In other words, why do we
> need all this code to avoid executing some other code (which could
> presumably bail out reasonably quickly) in cases where the transform
> changes in ways that do not affect scale?

It's necessary to do this check not because we're trying to avoid executing some expensive code, but because what we're trying to do here is to determine whether or not JavaScript code is animating the scale of the transform manually. Currently there is no way for ActiveLayerTracker to distinguish the case of JavaScript animating the scale from the case of JavaScript animating other components of the transformation. In practice we need the ability to distinguish animation of the scale from animation of, say, translation or perspective, or we will end up taking the fallback path (and just decoding images at full size) way too often. We encounter this issue in practice even in the B2G Gallery app.

> Or, to put it another way,
> why do you want this new hint rather than a test within the code that
> handles the existing UpdateTransformLayer hint -- which also has access
> to the data to get the correct scale?

We don't have access to the _previous_ value of the |transform| property there.

> Probably needs more nsChangeHint.h changes:  I think you need to add
> the new hint to nsChangeHint_Hints_NotHandledForDescendants and
> NS_HintsNotHandledForDescendantsIn.

Well, I think it's fine not to propagate the hint to descendants, because we need to walk up the tree to make decisions in ActiveLayerTracker anyway. I may be missing something, though.

> Why do you not need to produce this hint when we're changing from
> no-transform to having a transform, or vice-versa?

I suppose we could, and treat no-transform as having the identity scale.

> Sorry for the questions-over-time here; it's taken me a bit of time to
> understand what you're trying to do (and I still don't think I get all
> of it).

Just to clarify: the point of the stuff you're reviewing is primarily to detect animation by JavaScript of the |transform| property's scale, and distinguish it from other kinds of manual animation of the |transform| property, so we can take that information into account in the heuristics in ActiveLayerTracker.

As I understand it, we will end up needing this for more than just this image size prediction code; I've been told that there's interest in this information for things like layerization heuristics and layer resolution decisions, as well.

	David Baron :dbaron:
	Comment 52 • 9 years ago

Quick response to this point (I might respond to others in a bit):

(In reply to Seth Fowler [:seth] - PTO until 6/12 from comment #51)
> > Probably needs more nsChangeHint.h changes:  I think you need to add
> > the new hint to nsChangeHint_Hints_NotHandledForDescendants and
> > NS_HintsNotHandledForDescendantsIn.
> 
> Well, I think it's fine not to propagate the hint to descendants, because we
> need to walk up the tree to make decisions in ActiveLayerTracker anyway. I
> may be missing something, though.

Those functions aren't controlling whether the hints are handled for descendants -- they're describing to the style code that calls them how the hints are handled (by RestyleManager::ProcessRestyledFrames and whatever it calls to handle them).  In other words, RestyleManager::ProcessRestyledFrames handles an nsChangeHint_NeedDirtyReflow hint in a way that implicitly implies that hint for all descendants.  The same is true for nsChangeHint_RepaintFrame.  Thus, the style system doesn't need to re-report the hint for a descendant that also has it.  But your hint handling code (reasonably) doesn't do that here, so you need to update those functions so that the restyling code that calls those functions sends you hints on both ancestor and descendant when needed.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 53 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #52)
> Those functions aren't controlling whether the hints are handled for
> descendants -- they're describing to the style code that calls them how the
> hints are handled (by RestyleManager::ProcessRestyledFrames and whatever it
> calls to handle them).  In other words,
> RestyleManager::ProcessRestyledFrames handles an
> nsChangeHint_NeedDirtyReflow hint in a way that implicitly implies that hint
> for all descendants.  The same is true for nsChangeHint_RepaintFrame.  Thus,
> the style system doesn't need to re-report the hint for a descendant that
> also has it.  But your hint handling code (reasonably) doesn't do that here,
> so you need to update those functions so that the restyling code that calls
> those functions sends you hints on both ancestor and descendant when needed.

Hmm.. I suppose that it may be better to rereport on the descendants, in the case where the transform's scale has changed for both an ancestor and its descendant, because not doing so will affect the heuristic's behavior. (We'll ignore updates to the transform's scale on a descendant frame if an ancestor frame also had an update, and I can imagine there could be edge cases where that'd change whether we considered there to be active JavaScript animation of the transform scale.)

	David Baron :dbaron:
	Comment 54 • 9 years ago

Re comment 53 - you already are re-reporting on the descendants.  You're
just telling the reresolution code that it can perform optimizations
that aren't valid, and all you need to do for that is adjust the mask
and the function that I pointed out in comment 50.

Re comment 51:
> It's necessary to do this check not because we're trying to avoid
> executing some expensive code, but because what we're trying to do
> here is to determine whether or not JavaScript code is animating the
> scale of the transform manually. Currently there is no way for
> ActiveLayerTracker to distinguish the case of JavaScript animating the
> scale from the case of JavaScript animating other components of the
> transformation. In practice we need the ability to distinguish
> animation of the scale from animation of, say, translation or
> perspective, or we will end up taking the fallback path (and just
> decoding images at full size) way too often. We encounter this issue
> in practice even in the B2G Gallery app.

Isn't there a place you can store the last scale that you used, or
a place it is already stored?

Re comment 51:
> As I understand it, we will end up needing this for more than just
> this image size prediction code; I've been told that there's interest
> in this information for things like layerization heuristics and layer
> resolution decisions, as well.

I'm not convinced that this approximation (which is also a lot of code
to make that approximation) will actually be good enough for all those
uses, whereas storing the last scale used for whatever it is we're
comparing against seems very simple and pretty much guaranteed to be
accurate.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 55 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC-4 (mostly busy, back June 29) from comment #54)
> Re comment 53 - you already are re-reporting on the descendants.  You're
> just telling the reresolution code that it can perform optimizations
> that aren't valid, and all you need to do for that is adjust the mask
> and the function that I pointed out in comment 50.

The more we talk about this the less I understand it. I'm not sure if we're getting tripped up on semantics here (possibly what I mean by "re-reporting" is different from what you mean) or if there's something subtle about the behavior of this mask that I don't understand. I'd like to clear this up; we should discuss this in person if possible.

> Isn't there a place you can store the last scale that you used, or
> a place it is already stored?

It's not already stored anywhere. We could potentially store the previous scale in a frame property on every frame that has a |transform| property, and then when the |transform| property changes, compute the new scale for every frame that the |transform| property applies to and compare to the old value. This approach requires computing the transform matrix for every frame the |transform| property applies to - in other words, it's O(n) instead of O(1), since for each frame we have to do roughly the same work that we'd have to do to compare the |transform| property values directly.

> I'm not convinced that this approximation (which is also a lot of code
> to make that approximation) will actually be good enough for all those
> uses, whereas storing the last scale used for whatever it is we're
> comparing against seems very simple and pretty much guaranteed to be
> accurate.

I expect the approximation here to be extremely reliable, because all we care about in determining whether the |transform| property's scale has _changed_ or not, and I'm pretty convinced the approach I've taken here is reliable for that. There are some gotchas involving things like |em| units, but it should be perfectly accurate in almost all real world situations.

That said, overall I really just want to get this thing done and shipped. It's certainly true that storing the previous scale in a frame property and comparing it will work, so if you are convinced that's a better approach, I don't mind rewriting the patch to do it. I was specifically trying to avoid that when writing this patch, but possibly it's just not worth the effort to avoid. (And I admit, if the amount of effort that dealing with calc() and the like requires was clearer to me when I started writing the patch, I probably wouldn't have gone in this direction.)

	David Baron :dbaron:
	Comment 56 • 9 years ago

(In reply to Seth Fowler [:seth] from comment #55)
> (In reply to David Baron [:dbaron] ⏰UTC-4 (mostly busy, back June 29) from
> comment #54)
> > Re comment 53 - you already are re-reporting on the descendants.  You're
> > just telling the reresolution code that it can perform optimizations
> > that aren't valid, and all you need to do for that is adjust the mask
> > and the function that I pointed out in comment 50.
> 
> The more we talk about this the less I understand it. I'm not sure if we're
> getting tripped up on semantics here (possibly what I mean by "re-reporting"
> is different from what you mean) or if there's something subtle about the
> behavior of this mask that I don't understand. I'd like to clear this up; we
> should discuss this in person if possible.

OK, after reading what you wrote in response to the following comment I think I was misunderstanding what's going on.  I guess the code in ActiveLayerTracker::NotifyTransformScaleRestyle(aFrame) has to do something that applies to all descendants of aFrame for this to work -- although it's not obvious from looking at it.  Maybe that should be explained somewhere?

If that's the case, then my comment about needing to change the mask and the function is moot.

> > Isn't there a place you can store the last scale that you used, or
> > a place it is already stored?
> 
> It's not already stored anywhere. We could potentially store the previous
> scale in a frame property on every frame that has a |transform| property,

I see why you'd need this, and so far it sounds preferable to what you have.

> and then when the |transform| property changes, compute the new scale for
> every frame that the |transform| property applies to and compare to the old
> value.

I don't understand what you mean here.  What do you mean by "every frame that the |transform| property applies to"?  Do you mean the frame with 'transform' and all of its descendants?  If so, why is that needed?  If not, why is this a problem?

> This approach requires computing the transform matrix for every frame
> the |transform| property applies to - in other words, it's O(n) instead of
> O(1), since for each frame we have to do roughly the same work that we'd
> have to do to compare the |transform| property values directly.

Again, I'm not sure what you mean by every frame here -- we call CalcDifference, and process the change hints, for every frame on which the style has changed.  Are you talking about all descendants?

	David Baron :dbaron:
	Comment 57 • 9 years ago

Comment on attachment 8610956 [details] [diff] [review]
(Part 2) - Make it possible to estimate the matrix the transform property generates, even without a frame

I believe we agreed in an in-person discussion a while back that doing this wasn't nearly as valuable as seth thought since it wasn't turning any once-per-frame work into something less, so seth would go back and write a simpler version.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 58 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC+2 (busy until July 20) from comment #57)
> Comment on attachment 8610956 [details] [diff] [review]
> (Part 2) - Make it possible to estimate the matrix the transform property
> generates, even without a frame
> 
> I believe we agreed in an in-person discussion a while back that doing this
> wasn't nearly as valuable as seth thought since it wasn't turning any
> once-per-frame work into something less, so seth would go back and write a
> simpler version.

This is correct.

I've now popped things off my stack enough that I can work on this again. New versions of the style-related patches upcoming.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 59 • 9 years ago

Rebased part 1.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 60 • 9 years ago

Reimplemented part 2 by just storing the previous CSS transform scale and
comparing. Much less code.

Markus, I suspect at this point you should review this rather than David, as
everything that remains is in ActiveLayerTracker. All I'm trying to accomplish
in this part is to detect scale changes resulting from JavaScript changing the
CSS transform property; I use this information in part 3 as one of the
heuristics for deciding what size we should decode images at.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 61 • 9 years ago

Rebased part 4 and renumbered it to part 3 (as it was originally, before part 2 got split up).

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 62 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 63 • 9 years ago

Thanks for the review!

The previous version didn't handle the case where the transform property value
was removed. This version explicitly handles that case.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 64 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f8e2224776d

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 65 • 9 years ago

Fixed a rebase mistake.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 66 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ed1a1910ef1

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 67 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=977e0a22bfeb

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 68 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d733edebece

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 69 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=789b44783fcc

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 70 • 9 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 71 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01f7d06d47f3

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 72 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=888300c4decd

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 73 • 9 years ago

OK, this looks ready to land at this point.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 74 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/146f1bea4147
https://hg.mozilla.org/integration/mozilla-inbound/rev/f97e9a70a822
https://hg.mozilla.org/integration/mozilla-inbound/rev/03be986cf1aa

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

https://hg.mozilla.org/mozilla-central/rev/146f1bea4147
https://hg.mozilla.org/mozilla-central/rev/f97e9a70a822
https://hg.mozilla.org/mozilla-central/rev/03be986cf1aa