Set a performance warning when transform or opacity layers are too small

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox49 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Comment 1

3 years ago
This size restriction seems not to work for compositor animations.

Bas, you were involved in bug 678859, do you think the size restriction should be effective for compositor?  If not, this bug will be invalid.
Flags: needinfo?(bas)
Summary: Set a performance warning when transform layer is too small → Set a performance warning when transform or opacity layers are too small
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> This size restriction seems not to work for compositor animations.
> 
> Bas, you were involved in bug 678859, do you think the size restriction
> should be effective for compositor?  If not, this bug will be invalid.

That is probably better for performance and memory usage, yes.
Flags: needinfo?(bas)
(Assignee)

Comment 3

3 years ago
Thank you, Bas! Then I will fix it in this bug.
(Assignee)

Comment 4

3 years ago
GetVisibleRect() which is used in IsItemTooSmallForActiveLayer returns (0, 0) if the frame has 'preserve-3d' style.

https://dxr.mozilla.org/mozilla-central/rev/ea6298e1b4f7e22ce2311b2b6a918822f0adb112/layout/base/nsDisplayList.cpp#4211

On the other hand aFrame->GetVisualOverflowRectRelativeToSelf() in ShouldPrerenderTransformedContent() seems to return a correct value even if 'preserve-3d' is there.

https://dxr.mozilla.org/mozilla-central/rev/ea6298e1b4f7e22ce2311b2b6a918822f0adb112/layout/base/nsDisplayList.cpp#5710

Matt, should we also use GetVisualOverflowRectRelativeToSelf in IsItemTooSmallForActiveLayer?
Flags: needinfo?(matt.woodrow)
Don't we always force an active layer for preserve-3d anyway?

We also disable OMTA for preserve-3d, so I don't think this matters.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 6

3 years ago
Created attachment 8739831 [details]
MozReview Request: Bug 1258904 - Part 1: Don't create layers for compositor animations if the layer size is less than 16x16. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/45389/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45389/
Attachment #8739831 - Flags: review?(bas)
Attachment #8739832 - Flags: review?(bbirtles)
Attachment #8739833 - Flags: review?(bbirtles)
Attachment #8739834 - Flags: review?(bbirtles)
(Assignee)

Comment 7

3 years ago
Created attachment 8739832 [details]
MozReview Request: Bug 1258904 - Part 3: Factor out ToLocalizedStringForKey. r?birtles

This function will be used for the warning of small content.

Review commit: https://reviewboard.mozilla.org/r/45391/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45391/
(Assignee)

Comment 8

3 years ago
Created attachment 8739833 [details]
MozReview Request: Bug 1258904 - Part 4: Set performance warning for small content. r?birtles

Review commit: https://reviewboard.mozilla.org/r/45393/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45393/
(Assignee)

Comment 9

3 years ago
Created attachment 8739834 [details]
MozReview Request: Bug 1258904 - Part 5: Remove redundant tests for animation performance warnings. r?birtles

There were two places run gAnimationsTests in
test_animation_performance_warning.html.

Review commit: https://reviewboard.mozilla.org/r/45395/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45395/
(Assignee)

Comment 10

3 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Don't we always force an active layer for preserve-3d anyway?
> 
> We also disable OMTA for preserve-3d, so I don't think this matters.

Thank you, Matt.  I have another question about GetVisibleRect().
In a test case in part 3 patch, an element whose width and height are 8px with transform animation is created.  In that case GetVisibleRect().ToOutsidePixels(AppUnitsPerDevPixel()) sometimes returns 9px.  I don't investigate what is the trigger of the wrong size yet.  As far as I can tell,  only 'width' seems to be wrong.  Is this a known issue or should I file it as a new bug?
Flags: needinfo?(matt.woodrow)
Check what the top/left values are in app units?

If it's 8px, but the top/left isn't at a pixel edge (multiple of AppUnitsPerDevPixel()), then rounding out to pixels might make it 9.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8739831 [details]
MozReview Request: Bug 1258904 - Part 1: Don't create layers for compositor animations if the layer size is less than 16x16. r?mattwoodrow

I feel Matt is better qualified to review this even though I feel this should be fine.
Attachment #8739831 - Flags: review?(matt.woodrow)
Attachment #8739831 - Flags: review?(bas)
(Assignee)

Comment 13

3 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Check what the top/left values are in app units?
> 
> If it's 8px, but the top/left isn't at a pixel edge (multiple of
> AppUnitsPerDevPixel()), then rounding out to pixels might make it 9.

The width in app units was 481 in case when we get '9px'.  left was 1260, top was 20880 at that time. AppUnitsPerDevPixel is 60.
(Assignee)

Comment 14

3 years ago
A try result, there were two problems.  One is new test cases for this bug failed on Android.  The other is the new test cases broke all subsequent test cases on Linux non-E10S.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae0e27e02b91a15e5d3da90225d8633300d1147c

The failure reason on Android is related device DPI.  We should set layout.css.devPixelsPerPx to 1 in test.  I confirmed that setting the preference fixes the failure on Android locally.

The failure reason of the second one is not yet know.
Comment on attachment 8739831 [details]
MozReview Request: Bug 1258904 - Part 1: Don't create layers for compositor animations if the layer size is less than 16x16. r?mattwoodrow

https://reviewboard.mozilla.org/r/45389/#review42143
Attachment #8739831 - Flags: review?(matt.woodrow) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
 
> The width in app units was 481 in case when we get '9px'.  left was 1260,
> top was 20880 at that time. AppUnitsPerDevPixel is 60.

So we're a single app unit bigger than 8px.

Sounds like a rounding bug, float precision maybe?

You'll have to debug where it comes from.
(Assignee)

Comment 17

3 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
>  
> > The width in app units was 481 in case when we get '9px'.  left was 1260,
> > top was 20880 at that time. AppUnitsPerDevPixel is 60.
> 
> So we're a single app unit bigger than 8px.
> 
> Sounds like a rounding bug, float precision maybe?

It might not be a float precision problem.  aProperties.mTransformList has non-zero value, I see 0.00928551611 now there, this small value causes the error. 

https://hg.mozilla.org/mozilla-central/file/tip/layout/base/nsDisplayList.cpp#l5533
(Assignee)

Comment 18

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> >  
> > > The width in app units was 481 in case when we get '9px'.  left was 1260,
> > > top was 20880 at that time. AppUnitsPerDevPixel is 60.
> > 
> > So we're a single app unit bigger than 8px.
> > 
> > Sounds like a rounding bug, float precision maybe?
> 
> It might not be a float precision problem.  aProperties.mTransformList has
> non-zero value, I see 0.00928551611 now there, this small value causes the
> error. 

To be more precise aProperties.mTransformList->..->Item(1) has the non-zero value. All of other values are zeo.  I am now tracking where the non-zero comes from.
(Assignee)

Comment 19

3 years ago
Ah, I did not know that the value is the transform specified in test case itself.  That means the error comes from the difference NSToIntFloor and NSToIntCeil in nsRect::ScaleToOutsidePixels.

https://dxr.mozilla.org/mozilla-central/rev/e847cfcb315f511f4928b03fd47dcf57aad05e1e/gfx/src/nsRect.h#243
Comment on attachment 8739832 [details]
MozReview Request: Bug 1258904 - Part 3: Factor out ToLocalizedStringForKey. r?birtles

https://reviewboard.mozilla.org/r/45391/#review42255

r=birtles with comments addressed

::: dom/animation/AnimationPerformanceWarning.cpp:13
(Diff revision 1)
> +nsresult
> +AnimationPerformanceWarning::ToLocalizedStringForKey(
> +  const char* aKey, nsXPIDLString& aLocalizedString) const

Maybe we should call this ToLocalizedStringWithIntParams?

(I was thinking that we could actually templatize this and make the number of parameters a template argument. Then we wouldn't need kMaxParamsForLocalization, I think. However, it would introduce greater code bloat, I guess because we'd end up generating two versions of this function. We'd also have to pass mParams by const ref but I think that's ok? It's up to you if you think that's worthwhile.)

::: dom/animation/AnimationPerformanceWarning.cpp:17
(Diff revision 1)
>  
> +nsresult
> +AnimationPerformanceWarning::ToLocalizedStringForKey(
> +  const char* aKey, nsXPIDLString& aLocalizedString) const
> +{
> +  // We can pass an array of parameters whose length is greater than 7 to

Is '7' still significant here?

::: dom/animation/AnimationPerformanceWarning.cpp:43
(Diff revision 1)
>        MOZ_ASSERT(mParams->Length() <= kMaxParamsForLocalization,
>                   "Parameter's length should be less than "
>                   "kMaxParamsForLocalization");

I think we should move this assertion to ToLocalizedStringForKey?
Attachment #8739832 - Flags: review?(bbirtles) → review+
Comment on attachment 8739833 [details]
MozReview Request: Bug 1258904 - Part 4: Set performance warning for small content. r?birtles

https://reviewboard.mozilla.org/r/45393/#review42257

::: dom/animation/AnimationPerformanceWarning.cpp:43
(Diff revision 1)
> +      MOZ_ASSERT(mParams->Length() <= kMaxParamsForLocalization,
> +                 "Parameter's length should be less than "
> +                 "kMaxParamsForLocalization");

As with the previous patch, I think we can move this to ToLocalizedStringForKey (unless we templatize that method, in which case we might not need this at all).

::: dom/animation/test/chrome/test_animation_performance_warning.html:504
(Diff revision 1)
> +                    // We need to set transform here to try creating an
> +                    // individual layer for this opacity element.
> +                    'transform: translateX(100px);' },

Why is this needed? I don't quite follow.

::: dom/animation/test/chrome/test_animation_performance_warning.html:511
(Diff revision 1)
> +                    'transform: translateX(100px);' },
> +    expected: [
> +      {
> +        property: 'opacity',
> +        runningOnCompositor: false,
> +        warning: /Async animation disabled because frame size \(8, 8\) is smaller than \(16, 16\)/

I thought 'warning' could be a string too? That would save escaping the braces?
Attachment #8739833 - Flags: review?(bbirtles) → review+
Comment on attachment 8739834 [details]
MozReview Request: Bug 1258904 - Part 5: Remove redundant tests for animation performance warnings. r?birtles

https://reviewboard.mozilla.org/r/45395/#review42259
Attachment #8739834 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 23

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> A try result, there were two problems.  One is new test cases for this bug
> failed on Android.  The other is the new test cases broke all subsequent
> test cases on Linux non-E10S.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ae0e27e02b91a15e5d3da90225d8633300d1147c
> 
> The failure reason on Android is related device DPI.  We should set
> layout.css.devPixelsPerPx to 1 in test.  I confirmed that setting the
> preference fixes the failure on Android locally.
> 
> The failure reason of the second one is not yet know.

I've finally found the reason why tests which are subsequent to small element test cases fails.

With part 1 patch, once a small element is created, NS_FRAME_NO_COMPONENT_ALPHA is set against container frame in FrameLayerBuilder::BuildContainerLayerFor.  As a result, elements created in the subsequent tests are not sent to the compositor at all.

According to bug 741682#c9 and bug 741682#c7, NS_FRAME_NO_COMPONENT_ALPHA remains while the container frame is alive.  And it's hard to tell when it should be cleared (I think).  So now I am inclined to introduce a new flag named isTooSmallForActiveLayer into nsDisplayItem, check it and set mDisableFlattening if it's true.
(Assignee)

Comment 24

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
>  So now I am inclined to introduce a new flag
> named isTooSmallForActiveLayer into nsDisplayItem, check it and set
> mDisableFlattening if it's true.

I am going to upload whole patches including this part as part 2 from now on, I am really hoping MozReview does not break anything.
(Assignee)

Comment 25

2 years ago
Created attachment 8742628 [details]
MozReview Request: Bug 1258904 - Part 2: nsDisplay(Opacity|Transform)::GetLayerState should return LAYER_ACTIVE_FORCE if they have async animations to avoid flattening the layers.

Review commit: https://reviewboard.mozilla.org/r/47383/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47383/
Attachment #8739831 - Attachment description: MozReview Request: Bug 1258904 - Part 1: Don't create layers for compositor animations if the layer size is less than 16x16. r?Bas → MozReview Request: Bug 1258904 - Part 1: Don't create layers for compositor animations if the layer size is less than 16x16. r?mattwoodrow
Attachment #8739832 - Attachment description: MozReview Request: Bug 1258904 - Part 2: Factor out ToLocalizedStringForKey. r?birtles → MozReview Request: Bug 1258904 - Part 3: Factor out ToLocalizedStringForKey. r?birtles
Attachment #8739833 - Attachment description: MozReview Request: Bug 1258904 - Part 3: Set performance warning for small content. r?birtles → MozReview Request: Bug 1258904 - Part 4: Set performance warning for small content. r?birtles
Attachment #8739834 - Attachment description: MozReview Request: Bug 1258904 - Part 4: Remove redundant tests for animation performance warnings. r?birtles → MozReview Request: Bug 1258904 - Part 5: Remove redundant tests for animation performance warnings. r?birtles
Attachment #8742628 - Flags: review?(matt.woodrow)
(Assignee)

Comment 26

2 years ago
Comment on attachment 8739831 [details]
MozReview Request: Bug 1258904 - Part 1: Don't create layers for compositor animations if the layer size is less than 16x16. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45389/diff/1-2/
(Assignee)

Comment 27

2 years ago
Comment on attachment 8739832 [details]
MozReview Request: Bug 1258904 - Part 3: Factor out ToLocalizedStringForKey. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45391/diff/1-2/
(Assignee)

Comment 28

2 years ago
Comment on attachment 8739833 [details]
MozReview Request: Bug 1258904 - Part 4: Set performance warning for small content. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45393/diff/1-2/
(Assignee)

Comment 29

2 years ago
Comment on attachment 8739834 [details]
MozReview Request: Bug 1258904 - Part 5: Remove redundant tests for animation performance warnings. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45395/diff/1-2/
(Assignee)

Comment 30

2 years ago
https://reviewboard.mozilla.org/r/45391/#review42255

> Maybe we should call this ToLocalizedStringWithIntParams?
> 
> (I was thinking that we could actually templatize this and make the number of parameters a template argument. Then we wouldn't need kMaxParamsForLocalization, I think. However, it would introduce greater code bloat, I guess because we'd end up generating two versions of this function. We'd also have to pass mParams by const ref but I think that's ok? It's up to you if you think that's worthwhile.)

Thanks!  I made the function template function.
(Assignee)

Comment 31

2 years ago
https://reviewboard.mozilla.org/r/45393/#review42257

> I thought 'warning' could be a string too? That would save escaping the braces?

Unfortunetely it can't because 'string' is used to convert the string to localized string there.
I don't understand part 2.

mDisableFlattening is part of a BasicCompositor specific optimization, where we 'flatten' active layers together to prevent us ending up with subpixel-AA over a transparent background in the final layer tree.

We don't want to block this flattening because we have a small display item somewhere in the display list.

mIsTooSmallForActiveLayer also appears to be the reverse of what the name suggests, since it's set to true when we're *not* too small.
(Assignee)

Comment 33

2 years ago
Comment on attachment 8742628 [details]
MozReview Request: Bug 1258904 - Part 2: nsDisplay(Opacity|Transform)::GetLayerState should return LAYER_ACTIVE_FORCE if they have async animations to avoid flattening the layers.

(In reply to Matt Woodrow (:mattwoodrow) from comment #32)
> I don't understand part 2.
> 
> mDisableFlattening is part of a BasicCompositor specific optimization, where
> we 'flatten' active layers together to prevent us ending up with subpixel-AA
> over a transparent background in the final layer tree.
> 
> We don't want to block this flattening because we have a small display item
> somewhere in the display list.

Thank you, Matt.  I will consider other approach here.

> mIsTooSmallForActiveLayer also appears to be the reverse of what the name
> suggests, since it's set to true when we're *not* too small.

This was totally wrong. The test case I checked must be passed accidentally.
Attachment #8742628 - Flags: review?(matt.woodrow)
What are you trying to solve? I can try suggest something.
(Assignee)

Comment 35

2 years ago
What I am trying to fix is an issue in comment 23 which I've been seeing on non-E10S.  The issue is that layers for small display items are flatten in the failure case.  

I am now looking at mPropagateComponentAlphaFlattening, it is set to false if the display item is transfrom  at https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/layout/base/FrameLayerBuilder.cpp#4125 .  But it seems to me that aHasComponentAlphaChildren is set to true in ContainerState::Finish() if there is at least one layer whose mPropagateComponentAlphaFlattening is true despite other layer's mPropagateComponentAlphaFlattening state.  I might be missing something though.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> What I am trying to fix is an issue in comment 23 which I've been seeing on
> non-E10S.  The issue is that layers for small display items are flatten in
> the failure case.  
> 
> I am now looking at mPropagateComponentAlphaFlattening, it is set to false
> if the display item is transfrom  at
> https://dxr.mozilla.org/mozilla-central/rev/
> 67ac40fb8f680ea5e03805552187ba1b5e8392a1/layout/base/FrameLayerBuilder.
> cpp#4125 .  But it seems to me that aHasComponentAlphaChildren is set to
> true in ContainerState::Finish() if there is at least one layer whose
> mPropagateComponentAlphaFlattening is true despite other layer's
> mPropagateComponentAlphaFlattening state.  I might be missing something
> though.

You can return LAYER_ACTIVE_FORCE from GetLayerState if you feel that the layer being active is more important than getting subpixel-AA for the text (which I feel like it is for this case).
(Assignee)

Comment 37

2 years ago
Comment on attachment 8739831 [details]
MozReview Request: Bug 1258904 - Part 1: Don't create layers for compositor animations if the layer size is less than 16x16. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45389/diff/2-3/
Attachment #8742628 - Attachment description: MozReview Request: Bug 1258904 - Part 2: Don't flatten if the visible rect of the display item is too small to layerize. r?mattwoodrow. → MozReview Request: Bug 1258904 - Part 2: Don't flatten layers for small content size. r?mattwoodrow
Attachment #8742628 - Flags: review?(matt.woodrow)
(Assignee)

Comment 38

2 years ago
Comment on attachment 8742628 [details]
MozReview Request: Bug 1258904 - Part 2: nsDisplay(Opacity|Transform)::GetLayerState should return LAYER_ACTIVE_FORCE if they have async animations to avoid flattening the layers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47383/diff/1-2/
(Assignee)

Comment 39

2 years ago
Comment on attachment 8739832 [details]
MozReview Request: Bug 1258904 - Part 3: Factor out ToLocalizedStringForKey. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45391/diff/2-3/
(Assignee)

Comment 40

2 years ago
Comment on attachment 8739833 [details]
MozReview Request: Bug 1258904 - Part 4: Set performance warning for small content. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45393/diff/2-3/
(Assignee)

Comment 41

2 years ago
Comment on attachment 8739834 [details]
MozReview Request: Bug 1258904 - Part 5: Remove redundant tests for animation performance warnings. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45395/diff/2-3/
(Assignee)

Comment 42

2 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #36)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> > What I am trying to fix is an issue in comment 23 which I've been seeing on
> > non-E10S.  The issue is that layers for small display items are flatten in
> > the failure case.  
> > 
> > I am now looking at mPropagateComponentAlphaFlattening, it is set to false
> > if the display item is transfrom  at
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 67ac40fb8f680ea5e03805552187ba1b5e8392a1/layout/base/FrameLayerBuilder.
> > cpp#4125 .  But it seems to me that aHasComponentAlphaChildren is set to
> > true in ContainerState::Finish() if there is at least one layer whose
> > mPropagateComponentAlphaFlattening is true despite other layer's
> > mPropagateComponentAlphaFlattening state.  I might be missing something
> > though.
> 
> You can return LAYER_ACTIVE_FORCE from GetLayerState if you feel that the
> layer being active is more important than getting subpixel-AA for the text
> (which I feel like it is for this case).

Thank you, Matt!  Firstly I did not notice that CanUseAsyncAnimations has to return false in case of LAYER_ACTIVE_FORCE.  Now returning LAYER_ACTIVE_FORCE works fine.
Sorry, I meant to return LAYER_ACTIVE_FORCE from the MayBeAnimated() test in GetLayerState, that should be all you need.

Full explanation:

When we get a layer that contains text without an opaque background behind it, we can't easily render the text using subpixel-AA (since subpixel-AA needs to know the partial pixel colours behind the text to blend correctly).

For accelerated layers we render the layer twice (once on a black background, once on a white). The compositor can then use the two textures to recover the alpha channel and get the correct text. We call this 'component alpha' (since we're effectively retaining an alpha value for each r/g/b channel instead of only one for the pixel).

For BasicCompositor/BasicLayerManager this recovery process is too slow, so we don't bother. If we detect a layer that would require component alpha, we restart Layer building, but we convert LAYER_ACTIVE -> LAYER_INACTIVE (but not LAYER_ACTIVE_FORCE), and also disable active geometry roots. Hopefully this results in the component alpha layer no longer existing, but if it's still there then we just have to render the text without subpixel-AA.

In your broken test case, small (inactive?) transforms are the content that causes the layer to have component alpha, and thus trigger flattening. There are however, many other way to trigger flattening, background-attachment:fixed on the page background is the most common.

I think the right solution is to make sure that flattening doesn't prevent async animations from working (by 'forcing' an unflattenable layer using LAYER_ACTIVE_FORCE), rather than trying to stop the flattening from triggering (since there's too many ways it can happen, and it serves a useful purpose).

As for why this only happens with non-e10s, with e10s we decided that async scrolling was more valuable than subpixel-AA, so flattening never happens when APZ is enabled.
(Assignee)

Updated

2 years ago
Attachment #8742628 - Flags: review?(matt.woodrow)
(Assignee)

Comment 44

2 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #43)
> Sorry, I meant to return LAYER_ACTIVE_FORCE from the MayBeAnimated() test in
> GetLayerState, that should be all you need.

Thanks for the detailed explanation!  If I understand correctly the explanation, we need to return LAYER_ACTIVE_FORCE from NeedsActiveLayer() test in nsDisplayOpacity::GetLayerState() as well.  Right?
Flags: needinfo?(matt.woodrow)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #43)
> > Sorry, I meant to return LAYER_ACTIVE_FORCE from the MayBeAnimated() test in
> > GetLayerState, that should be all you need.
> 
> Thanks for the detailed explanation!  If I understand correctly the
> explanation, we need to return LAYER_ACTIVE_FORCE from NeedsActiveLayer()
> test in nsDisplayOpacity::GetLayerState() as well.  Right?

Yes indeed!
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 46

2 years ago
Comment on attachment 8739831 [details]
MozReview Request: Bug 1258904 - Part 1: Don't create layers for compositor animations if the layer size is less than 16x16. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45389/diff/3-4/
Attachment #8742628 - Attachment description: MozReview Request: Bug 1258904 - Part 2: Don't flatten layers for small content size. r?mattwoodrow → MozReview Request: Bug 1258904 - Part 2: nsDisplay(Opacity|Transform)::GetLayerState should return LAYER_ACTIVE_FORCE if they have async animations to avoid flattening the layers.
Attachment #8742628 - Flags: review?(matt.woodrow)
(Assignee)

Comment 47

2 years ago
Comment on attachment 8742628 [details]
MozReview Request: Bug 1258904 - Part 2: nsDisplay(Opacity|Transform)::GetLayerState should return LAYER_ACTIVE_FORCE if they have async animations to avoid flattening the layers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47383/diff/2-3/
(Assignee)

Comment 48

2 years ago
Comment on attachment 8739832 [details]
MozReview Request: Bug 1258904 - Part 3: Factor out ToLocalizedStringForKey. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45391/diff/3-4/
(Assignee)

Comment 49

2 years ago
Comment on attachment 8739833 [details]
MozReview Request: Bug 1258904 - Part 4: Set performance warning for small content. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45393/diff/3-4/
(Assignee)

Comment 50

2 years ago
Comment on attachment 8739834 [details]
MozReview Request: Bug 1258904 - Part 5: Remove redundant tests for animation performance warnings. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45395/diff/3-4/
(Assignee)

Comment 51

2 years ago
Thank you, Matt. It seems to work fine now.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b02fd42f2b32
Comment on attachment 8742628 [details]
MozReview Request: Bug 1258904 - Part 2: nsDisplay(Opacity|Transform)::GetLayerState should return LAYER_ACTIVE_FORCE if they have async animations to avoid flattening the layers.

https://reviewboard.mozilla.org/r/47383/#review47933
Attachment #8742628 - Flags: review?(matt.woodrow) → review+

Updated

2 years ago
Blocks: 1279670

Updated

2 years ago
Assignee: nobody → hiikezoe

Updated

2 years ago
Blocks: 1280060

Updated

2 years ago
Blocks: 1262669
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1262669

Updated

2 years ago
Blocks: 1288059
Blocks: 1294391
Depends on: 1301305
You need to log in before you can comment on or make changes to this bug.