Closed
Bug 1258904
Opened 8 years ago
Closed 8 years ago
Set a performance warning when transform or opacity layers are too small
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
We need to set it here. https://dxr.mozilla.org/mozilla-central/rev/ea6298e1b4f7e22ce2311b2b6a918822f0adb112/layout/base/nsDisplayList.cpp#5890
Assignee | ||
Comment 1•8 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
Comment 2•8 years ago
|
||
(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•8 years ago
|
||
Thank you, Bas! Then I will fix it in this bug.
Assignee | ||
Comment 4•8 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)
Comment 5•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45393/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45393/
Assignee | ||
Comment 9•8 years ago
|
||
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•8 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)
Comment 11•8 years ago
|
||
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 12•8 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 I feel Matt is better qualified to review this even though I feel this should be fine.
Attachment #8739831 -
Flags: review?(matt.woodrow)
Updated•8 years ago
|
Attachment #8739831 -
Flags: review?(bas)
Assignee | ||
Comment 13•8 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•8 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 15•8 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 https://reviewboard.mozilla.org/r/45389/#review42143
Attachment #8739831 -
Flags: review?(matt.woodrow) → review+
Comment 16•8 years ago
|
||
(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•8 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•8 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•8 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 20•8 years ago
|
||
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 21•8 years ago
|
||
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 22•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 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•8 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.
Comment 32•8 years ago
|
||
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•8 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)
Comment 34•8 years ago
|
||
What are you trying to solve? I can try suggest something.
Assignee | ||
Comment 35•8 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.
Comment 36•8 years ago
|
||
(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•8 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•8 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•8 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•8 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•8 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•8 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.
Comment 43•8 years ago
|
||
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•8 years ago
|
Attachment #8742628 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 44•8 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)
Comment 45•8 years ago
|
||
(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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
Thank you, Matt. It seems to work fine now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b02fd42f2b32
Comment 52•8 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. https://reviewboard.mozilla.org/r/47383/#review47933
Attachment #8742628 -
Flags: review?(matt.woodrow) → review+
Comment 53•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c119fc3431b https://hg.mozilla.org/integration/mozilla-inbound/rev/05cb31c61bf2 https://hg.mozilla.org/integration/mozilla-inbound/rev/156b9e7e21c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f185cfc034 https://hg.mozilla.org/integration/mozilla-inbound/rev/246fdb220ef3
Comment 54•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2c119fc3431b https://hg.mozilla.org/mozilla-central/rev/05cb31c61bf2 https://hg.mozilla.org/mozilla-central/rev/156b9e7e21c7 https://hg.mozilla.org/mozilla-central/rev/f4f185cfc034 https://hg.mozilla.org/mozilla-central/rev/246fdb220ef3
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•8 years ago
|
Assignee: nobody → hiikezoe
You need to log in
before you can comment on or make changes to this bug.
Description
•