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.

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

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

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?

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

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.

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

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.

Updated commit message.

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

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.

Here's the new version of part 2.

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?

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

This bitrotted a little so it needed a rebase.

This one also bitrotted.

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

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

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.

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

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

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.

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.

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

Here's the updated patch.

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.

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.

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

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.

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.

(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); } }

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

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

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.

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.

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.

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.

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.

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?

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

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

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

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

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

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.

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

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.

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

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

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.

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

Rebased part 1.

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.

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

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.

Fixed a rebase mistake.

OK, this looks ready to land at this point.

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

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