Closed Bug 1364221 Opened 7 years ago Closed 7 years ago

Allow frames to be prerendered as long as the area of the frame is less than the limit

Categories

(Core :: Graphics: Layers, enhancement, P1)

Product:
Core
Component:
Graphics: Layers
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Iteration:
55.7 - Jun 12
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-animation])

Attachments

(1 file)

Bug 1364221 - Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit.
59 bytes, text/x-review-board-request
Details 
We still need to check that the dimensions are also less than the absolute limit.

This is blocking the ability to transform images that are just wider than the viewport but not prohibitively tall.
Comment on attachment 8866972 [details]
Bug 1364221 - Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit.

https://reviewboard.mozilla.org/r/138572/#review141822

Does it still make sense to keep viewportRatioX and viewportRatioY separate?

I think Botond and/or Matt should double-check that this sizing policy is fine.

We could also try a little harder with the error message. At the moment it does not mention the absolute size. Maybe we should have two different error messages, depending on which check failed.
Attachment #8866972 - Flags: review?(mstange)
Attachment #8866972 - Flags: review?(matt.woodrow)
Attachment #8866972 - Flags: review?(botond)
Iteration: --- → 55.5 - May 15
Flags: qe-verify?
Priority: -- → P1
Comment on attachment 8866972 [details]
Bug 1364221 - Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit.

https://reviewboard.mozilla.org/r/138572/#review141878

::: dom/locales/en-US/chrome/layout/layout_errors.properties:18
(Diff revision 1)
>  
> -## LOCALIZATION NOTE(CompositorAnimationWarningContentTooLarge2):
> -## (%1$S, %2$S) is a pair of integer values of the frame size
> -## (%3$S, %4$S) is a pair of integer values of a limit based on the viewport size
> -## (%5$S, %6$S) is a pair of integer values of an absolute limit
> -CompositorAnimationWarningContentTooLarge2=Animation cannot be run on the compositor because the frame size (%1$S, %2$S) is too large relative to the viewport (larger than (%3$S, %4$S)) or larger than the maximum allowed value (%5$S, %6$S)
> +## LOCALIZATION NOTE(CompositorAnimationWarningContentTooLarge3):
> +## %1$S is the area of the frame size
> +## %2$S is the area of the viewport size
> +## %3$S is the area of an absolute limit
> +CompositorAnimationWarningContentTooLarge3=Animation cannot be run on the compositor because the area of the frame size (%1$S) is too large relative to the viewport (larger than %2$S) or larger than the maximum allowed value (%3$S)

It might be worth keeping the width/height values for the frame in question (as well as the computed area), to make it easier for authors to link their styles with the problem.

::: layout/painting/nsDisplayList.cpp:7354
(Diff revision 1)
>    uint32_t absoluteLimitY = gfxPrefs::AnimationPrerenderAbsoluteLimitY();
>    nsSize refSize = aBuilder->RootReferenceFrame()->GetSize();
>    // Only prerender if the transformed frame's size is <= a multiple of the
>    // reference frame size (~viewport), and less than an absolute limit.
>    // Both the ratio and the absolute limit are configurable.
> +  uint32_t relativeLimitArea = refSize.width * viewportRatioX *

These values could overflow, and end up passing the checks even though we're massive.

You should have a look at CheckedUint32 for this.

::: layout/painting/nsDisplayList.cpp:7358
(Diff revision 1)
>    // Both the ratio and the absolute limit are configurable.
> +  uint32_t relativeLimitArea = refSize.width * viewportRatioX *
> +                               refSize.height * viewportRatioY;
>    nsSize relativeLimit(nscoord(refSize.width * viewportRatioX),
>                         nscoord(refSize.height * viewportRatioY));
> +  uint32_t absoluteLimitArea = aFrame->PresContext()->DevPixelsToAppUnits(absoluteLimitX) *

Put the area values after the non-area versions, so you can avoid doing DevPixelsToAppUnits twice on the same value.

::: layout/painting/nsDisplayList.cpp:7369
(Diff revision 1)
>    gfxSize scale = nsLayoutUtils::GetTransformToAncestorScale(aFrame);
> -  nsSize frameSize = nsSize(overflow.Size().width * scale.width,
> +  uint32_t frameArea = overflow.Size().width * scale.width *
> +                       overflow.Size().height * scale.height;
> +  nsSize frameSize(overflow.Size().width * scale.width,
> -                            overflow.Size().height * scale.height);
> +                   overflow.Size().height * scale.height);
> -  if (frameSize <= maxSize) {
> +  if (frameArea <= maxArea && frameSize <= absoluteLimit) {

The error message suggests that we only care about the areas, but this is still checking if we exceed the max dimension too.

I don't have a strong preference as to which behaviour we go with, as long as it's consistent.
Attachment #8866972 - Flags: review?(matt.woodrow)
Comment on attachment 8866972 [details]
Bug 1364221 - Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit.

https://reviewboard.mozilla.org/r/138572/#review142146

Thanks, this generally looks good. Just a couple of comments, and I agree with Matt's comments well.

::: layout/painting/nsDisplayList.cpp:7358
(Diff revision 1)
>    // Both the ratio and the absolute limit are configurable.
> +  uint32_t relativeLimitArea = refSize.width * viewportRatioX *
> +                               refSize.height * viewportRatioY;
>    nsSize relativeLimit(nscoord(refSize.width * viewportRatioX),
>                         nscoord(refSize.height * viewportRatioY));
> +  uint32_t absoluteLimitArea = aFrame->PresContext()->DevPixelsToAppUnits(absoluteLimitX) *

There's no point in checking against the absolute area limit if we're also checking against the absolute limit for the individual dimensions. (If the frame's dimensions are individually smaller than the absolute limit, then by definition the area will be smaller as well.)

::: layout/painting/nsDisplayList.cpp:7384
(Diff revision 1)
>      AnimationPerformanceWarning(
>        AnimationPerformanceWarning::Type::ContentTooLarge,
>        {
> -        nsPresContext::AppUnitsToIntCSSPixels(frameSize.width),
> -        nsPresContext::AppUnitsToIntCSSPixels(frameSize.height),
> -        nsPresContext::AppUnitsToIntCSSPixels(relativeLimit.width),
> +        nsPresContext::AppUnitsToIntCSSPixels(frameArea),
> +        nsPresContext::AppUnitsToIntCSSPixels(relativeLimitArea),
> +        nsPresContext::AppUnitsToIntCSSPixels(absoluteLimitArea),

As Matt mentioned, please keep the values reported in the error consistent with what we're checking for.

That is, since the check includes the individual components of the area, the error message should as well.
Attachment #8866972 - Flags: review?(botond)
Iteration: 55.5 - May 15 → 55.6 - May 29
Flags: qe-verify? → qe-verify-
Comment on attachment 8866972 [details]
Bug 1364221 - Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit.

https://reviewboard.mozilla.org/r/138572/#review143282

::: layout/painting/nsDisplayList.cpp:7384
(Diff revision 2)
>    // reference frame size (~viewport), and less than an absolute limit.
>    // Both the ratio and the absolute limit are configurable.
>    nsSize relativeLimit(nscoord(refSize.width * viewportRatioX),
>                         nscoord(refSize.height * viewportRatioY));
> +  CheckedUint32 checkedRelativeLimitArea =
> +    CheckedUint32(relativeLimit.width * relativeLimit.height);

This actually does the computation before changing the type to CheckedUint32 so won't work (and I *think* it'll fail static analysis builds.

CheckedUint32(relativeLimit.width) * relativeLimit.height;

should do what you want.

::: layout/painting/nsDisplayList.cpp:7396
(Diff revision 2)
>    nsSize maxSize = Min(relativeLimit, absoluteLimit);
>    gfxSize scale = nsLayoutUtils::GetTransformToAncestorScale(aFrame);
> -  nsSize frameSize = nsSize(overflow.Size().width * scale.width,
> +  nsSize frameSize(overflow.Size().width * scale.width,
> -                            overflow.Size().height * scale.height);
> +                   overflow.Size().height * scale.height);
> -  if (frameSize <= maxSize) {
> +  CheckedUint32 checkedFrameArea =
> +    CheckedUint32(frameSize.width * frameSize.height);

Same CheckedUint32 issue here.

::: layout/painting/nsDisplayList.cpp:7399
(Diff revision 2)
> -                            overflow.Size().height * scale.height);
> +                   overflow.Size().height * scale.height);
> -  if (frameSize <= maxSize) {
> +  CheckedUint32 checkedFrameArea =
> +    CheckedUint32(frameSize.width * frameSize.height);
> +  uint32_t frameArea = checkedFrameArea.isValid() ? checkedFrameArea.value() :
> +                                                    UINT32_MAX;
> +  if (frameArea <= relativeLimitArea && frameSize <= absoluteLimit) {

frameSize <= maxSize here?

::: layout/painting/nsDisplayList.cpp:7407
(Diff revision 2)
>    } else if (gfxPrefs::PartiallyPrerenderAnimatedContent()) {
>      *aDirtyRect = nsLayoutUtils::ComputePartialPrerenderArea(*aDirtyRect, overflow, maxSize);
>      return PartialPrerender;
>    }
>  
> +  if (frameArea <= relativeLimitArea) {

if (frameArea > relativeLimitArea) right? This is the error message for when our area exceeded the limit.
Attachment #8866972 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8866972 [details]
Bug 1364221 - Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit.

https://reviewboard.mozilla.org/r/138572/#review143500

::: layout/painting/nsDisplayList.cpp:7399
(Diff revision 2)
> -                            overflow.Size().height * scale.height);
> +                   overflow.Size().height * scale.height);
> -  if (frameSize <= maxSize) {
> +  CheckedUint32 checkedFrameArea =
> +    CheckedUint32(frameSize.width * frameSize.height);
> +  uint32_t frameArea = checkedFrameArea.isValid() ? checkedFrameArea.value() :
> +                                                    UINT32_MAX;
> +  if (frameArea <= relativeLimitArea && frameSize <= absoluteLimit) {

I think we should be doing `if (frameArea <= relativeLimitArea && frameSize <= absoluteLimit) {`. Checking frameSize <= maxSize (maxSize is defined as the minimum of the relativeLimit and absoluteLimit) would be redundant since it is already covered by the frameArea <= relativeLimitArea (when relativeLimit is lower than absoluteLimit).
Flags: needinfo?(matt.woodrow)
From IRC:
<botond|wfh> jaws: agreed, i think the check now makes sense as written
Flags: needinfo?(matt.woodrow)
What about if these were our input values:

relativeLimit(100,100)
absoluteLimit(200,200)
frameSize(150, 50)

That gives us these computed values:

frameArea(7500)
relativeLimitArea(10000)
maxSize(100,100)

frameArea is indeed smaller than relativeLimitArea, frameSize is smaller than absoluteLimit, but frameSize is *not* smaller than relativeLimit (or maxSize).
(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> What about if these were our input values:
> 
> relativeLimit(100,100)
> absoluteLimit(200,200)
> frameSize(150, 50)
> 
> That gives us these computed values:
> 
> frameArea(7500)
> relativeLimitArea(10000)
> maxSize(100,100)
> 
> frameArea is indeed smaller than relativeLimitArea, frameSize is smaller
> than absoluteLimit, but frameSize is *not* smaller than relativeLimit (or
> maxSize).

I thought that was the whole point of the patch, to allow fully pre-rendering in such a case.
This is a good point. I guess the decision is whether we always want to do the dimensions compare against absoluteLimit, or the larger of the two.
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> This is a good point. I guess the decision is whether we always want to do
> the dimensions compare against absoluteLimit, or the larger of the two.

Based on comment 0, I assumed the intention was the former.
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> This is a good point. I guess the decision is whether we always want to do
> the dimensions compare against absoluteLimit, or the larger of the two.

Well, maxSize is actually defined as the Min(relativeLimit, absoluteLimit). We could calculate the *actual* maxSize of the two if we are worried that relativeLimit could be larger than absoluteLimit.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8c14563b37d
Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit. r=mattwoodrow
Why are you changing the string ID for CompositorAnimationWarningContentTooLarge2? The string is the same, you shouldn't change the ID just to make the string ID more understandable.
Comment on attachment 8866972 [details]
Bug 1364221 - Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit.

Hey Matt, can you review this again? I made some changes to no longer use CheckedUint32 since I found it overflows very often when using appunits. Instead I'm just storing the result in a 64-bit int which won't overflow as long as the two operands are 32-bit.

Also, I had to remove the usage of nsPresContext::AppUnitsToIntCSSPixels when dealing with the 64-bit integers because the function will cast to 32-bit integer and we get back overflowed numbers as a result, so I'm doing the division in place and then converting the result to an int (the compiler wasn't happy with a cast to uint32_t) because AnimationPerformanceWarning uses 32-bit integers.
Flags: needinfo?(jaws)
Attachment #8866972 - Flags: review+ → review?(matt.woodrow)
Comment on attachment 8866972 [details]
Bug 1364221 - Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit.

https://reviewboard.mozilla.org/r/138572/#review146988

::: layout/painting/nsDisplayList.cpp:7405
(Diff revision 5)
> +          int(frameArea / uint64_t(nsPresContext::AppUnitsPerCSSPixel())),
> +          int(maxLimitArea / uint64_t(nsPresContext::AppUnitsPerCSSPixel())),

I think you actually need to divide by nsPresContext::AppUnitsPerCSSPixel()^2 because you're converting an area.

r+ with that change. You probably want to pull out the value of nsPresContext::AppUnitsPerCSSPixel() into a variable so that the lines can be shorter.
Attachment #8866972 - Flags: review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb1542b364f2
Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit. r=mattwoodrow,mstange
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c93d5b222a0f
Allow frames to be prerendered as long as the area of the frame is less than the area of the relative limit and the dimensions are less than the absolute limit. r=mattwoodrow,mstange
https://hg.mozilla.org/mozilla-central/rev/c93d5b222a0f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I focused on the changed string ID and not on the new string the first time this landed. The string currently is: "Animation cannot be run on the compositor because the area of the frame size (%1$S) is too large relative to the viewport (larger than %2$S)".

"the area of the frame size": is "size" wanted here or a remnant of copy and paste from CompositorAnimationWarningContentTooLarge2?
Flags: needinfo?(jaws)
(In reply to Francesco Lodolo [:flod] from comment #29)
> I focused on the changed string ID and not on the new string the first time
> this landed. The string currently is: "Animation cannot be run on the
> compositor because the area of the frame size (%1$S) is too large relative
> to the viewport (larger than %2$S)".
> 
> "the area of the frame size": is "size" wanted here or a remnant of copy and
> paste from CompositorAnimationWarningContentTooLarge2?

"size" was intended to be there, but maybe my grammar is not correct and we should remove it.
Flags: needinfo?(jaws)
Depends on: 1370528
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: