Closed Bug 1239564 Opened 8 years ago Closed 8 years ago

CSS Animation of transform: scale on element with border-radius broken with HWA disabled

Categories

(Core :: Layout, defect)

43 Branch
Unspecified
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 + wontfix
firefox46 + fixed
firefox47 --- fixed

People

(Reporter: reesewill, Assigned: chenpighead)

References

Details

(Keywords: regression, testcase)

Attachments

(8 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36

Steps to reproduce:

Bug can be reproduced by using a css animation to resize an element with transform scale. The element must have a border-radius property set. I have only seen this occur on Windows 7, Firefox 43.

https://jsfiddle.net/4wdoxfaf/


Actual results:

Instead of the element growing or shrinking, it translates diagonally, as though the transform origin is shifting.


Expected results:

The element should have stayed in its position, the actual size shrinking or growing.
OS: Unspecified → Windows 7
Priority: -- → P3
Component: Untriaged → Layout
Priority: P3 → --
Product: Firefox → Core
(In reply to reesewill from comment #0)
> Bug can be reproduced by using a css animation to resize an element with
> transform scale. The element must have a border-radius property set. I have
> only seen this occur on Windows 7, Firefox 43.
> 
> https://jsfiddle.net/4wdoxfaf/

I couldn't reproduce the described problem. This testcase doesn't have border radius.
Flags: needinfo?(reesewill)
I've updated the test case. Apologies about the first one:

https://jsfiddle.net/4wdoxfaf/1/

One more detail... at least one of the 2 machines I've seen this on was 32-bit. I can't be sure about the other one.

This is where I originally noticed the bug. You'll notice the problem on the third and sixth example spinner: 

http://cssload.net/en/spinners
Flags: needinfo?(reesewill)
I couldn't reproduce. I tried on Mac and Windows. Nightly and release. The Windows machine was using 32bit Firefox, but it's running 64bit Windows.
I'll make a video of the big next Monday. After I first saw the bug on my coworkers Windows notebook. I recreated it on microsofts Windows 7 i.e. 9 official virtual machine for Mac Vm virtual box. 

https://dev.windows.com/en-us/microsoft-edge/tools/vms/mac/

I'll also get the complete specs of my coworkers notebook on Monday.
It's reproducible with Nightly and only HWA disabled.
Attached file 1239564.html
Keywords: testcase
It's a regression:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e747377d86eb4297f5be44aab1d911978dce1c5c&tochange=f590a95a2c64dd420d25d07f081870f2cec26b4d

My guess goes to bug 1149923.
Blocks: 1149923
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jeremychen)
Keywords: regression
Summary: Windows Firefox 43: CSS Animation of transform: scale on element with border-radius broken → CSS Animation of transform: scale on element with border-radius broken with HWA disabled
Tracking for 45 and 46, recent regression.
It can be reproduced on my Ubuntu machine (ff43-release no matter HWA is disabled or not). But it can't be reproduced on my mac (ff43-release)...hmmm. I'll keep digging and find out if bug 1149923 is the root cause or not.
Flags: needinfo?(jeremychen)
Yes, it was just a guess, maybe a different bug in the changelog is the underlying regression.
Quick update. In bug 1149923, only CompositorOGL::DrawQuad [1] is considered, BasicCompositor::DrawQuad [2]is not. When HWA is enabled, image w/ mask layer would be rendered by [1]; however, when HWA is disabled, image w/ mask layer would be rendered by [2]. I'm working on a patch to take HWA disabled into consideration as well. Sorry about the regression.


[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp?from=compositorogl.cpp#1065
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#401
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
After digging deeper, I think my patch in bug 1149923 is not the root cause of this regression. This bug seems more likely a former regression which is revealed after my patch landed. I'll upload a patch and ask for feedback to prove my assumption.
In [1], newTransform calls postTranslate() to translate to RenderTarget's origin; however, maskTransform calls preTranslate() in [2]. I think this is the root cause of this bug.

Hi Matt, I'm wondering if maskTransform should call postTranslate() to agree with that of newTransform?

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#391
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#402
Attachment #8713541 - Flags: feedback?(matt.woodrow)
Comment on attachment 8713541 [details] [diff] [review]
Part1: Post translate maskSurface to renderTarget coord. (v1)

Review of attachment 8713541 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!
Attachment #8713541 - Flags: feedback?(matt.woodrow) → feedback+
Comment on attachment 8713541 [details] [diff] [review]
Part1: Post translate maskSurface to renderTarget coord. (v1)

Matt, thank you for the feedback. :)
Attachment #8713541 - Flags: review?(roc)
Comment on attachment 8713541 [details] [diff] [review]
Part1: Post translate maskSurface to renderTarget coord. (v1)

Review of attachment 8713541 [details] [diff] [review]:
-----------------------------------------------------------------

Needs a test!
Attachment #8713541 - Flags: review?(roc) → review+
Attached file simpler test case
Testing steps should be as follows:
1. turn HWA pref off
2. launch browser w/ HWA off
3. open test page and reference page to validate the result

I'm still figuring out how to write this test. Note that HWA preference is a  "read once" preference, so We'll need to push preference "before" launching browser. However, it seems that reftest and mochitest could not do this. =(
Jeremy, do you have an eta when this patch will land? thanks
Flags: needinfo?(jeremychen)
(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> Jeremy, do you have an eta when this patch will land? thanks

Here's the progress updated on 2/7. I can do re-launching browser w/ specific prefs in marionette test, but the screenshot api seems not work as I expected.

I'll continue to finish the test on 2/17, after I return from Chinese New Year. I think I can make it by 2/20.
Flags: needinfo?(jeremychen)
> Here's the progress updated on 2/7. I can do re-launching browser w/
> specific prefs in marionette test, but the screenshot api seems not work as
> I expected.
> 
> I'll continue to finish the test on 2/17, after I return from Chinese New
> Year. I think I can make it by 2/20.

Looks like every result of first paint would be correct, and then the composed reseult got messed very soon (if the patch of this bug is not applied). The screenshot taken from marionette test seems always present that correct one, so test can't present what I expected, a broken one. Keep digging in the rendering pipeline...
Ok, it seems more clear to me after tracing the code path back and forth. As Comment 21 said, it take two requirements to make the test:

1. Be able to launch browser w/ HWA on/off (may relaunch browser if needed)
2. Make a screenshot to get a reference image for comparison

If we could fulfill both, then test could be made. (verify whether HWA-ON result is consistent with HWA-Off could be one choice) However, I got few problems in the 2nd requirement.

Problem 1: Can't trigger MakeSnapShot() to get compositor result.
When calling marionette.screenshot() with chrome context in [1], some flags may cause MakeSnapShotIfRequired() in [2] early returned, so we can't make a snapshot from parent side. In this case, the returned screenshot may be composed in client side alone (by moz2D api I guess), which can't present the real composed result on the screen since the code path would never go through compositor parent (neither BasicCompositor nor CompositorOGL).

After I did some hack to force marionette.screenshot() successfully go through compositor parent, I jumped into another problem.

Problem 2: Can't make a screenshot for failed test case.
After successfully calling MakeSnapShot(), in compositor parent side, we always get a new clean mRenderTarget with mOrigin (0,0), which leads to a offset=(0,0) in [3]. However, this bug can only reproduced w/ non-zero offset (see [4]). This could be proved by switching to test page from any other tab, you'll see a correct rendered result at a glance.

In conclusion, even we force marionette.screenshot() to go MaksSnapShot() code path and get the compositor result, the screenshot would never show a failed result.


Maybe I can try Bug 1173117 to use OS level snapshot, but it seems only work in windows? Or, I'm not sure if we should make this test as a followup bug?

Hi roc, is there anything else I could try?


[1] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/testing/marionette/driver.js#2586
[2] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/gfx/layers/client/ClientLayerManager.cpp#494
[3] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/gfx/layers/basic/BasicCompositor.cpp#366
[4] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/gfx/layers/basic/BasicCompositor.cpp#411
Flags: needinfo?(roc)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #26)
> 1. Be able to launch browser w/ HWA on/off (may relaunch browser if needed)

You don't need to force this. You can assume we'll run some tests with HWA off. As long as your test passes with HWA on and off, we'll be fine.

We should be able to create a reftest for this similar to other animation reftests.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #26)
> > 1. Be able to launch browser w/ HWA on/off (may relaunch browser if needed)
> 
> You don't need to force this. You can assume we'll run some tests with HWA
> off. As long as your test passes with HWA on and off, we'll be fine.
> 
> We should be able to create a reftest for this similar to other animation
> reftests.

You're right, I should rephrase what I said in Comment 26. The first requirement may be optional if we already have tests for both HWA on and off.

However, I'm wondering if we could not fulfill the second requirement, how can we make a snapshot for broken compositor result?

Here is the exact what happens in BasicCompositor while reproducing this bug:
1. Start first time composition in BasicCompositor with a clean mRenderTarget. Since this is the first time composition, mRenderTarget should always be the whole viewport with mOrigin = (0,0).
2. Refresh driver triggers next tick due to whatever reason, this time only partial region, where the mask layer positioned, would be updated.
3. Start second time composition, this time only update where mask layer is positioned. So, the same mRenderTarget with non-zero mOrigin is used for BasicCompositor to do partial updating. Then, the non-zero mOrigin leads to a non-zero offset translate in mask layer painting, and then causes the broken result.

This means we can't get the first time composition go through the broken code path. 

In Comment 26, I stated that the testing screenshot API is either doing composition in content side by Moz2D, or doing so in parent side by sending MakeSnapShot() to compositor. Doing composition in content side of course will not go through the broken code path since it never go through parent's compositor. Sending MakeSnapShot() to compositor means asking compositor to do a one time composition, which will always like the first time composition (like the step 1 mentioned above).

Neither ways can go through the broken code path. =(
Flags: needinfo?(roc)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #28)
> However, I'm wondering if we could not fulfill the second requirement, how
> can we make a snapshot for broken compositor result?
> 
> Here is the exact what happens in BasicCompositor while reproducing this bug:
> 1. Start first time composition in BasicCompositor with a clean
> mRenderTarget. Since this is the first time composition, mRenderTarget
> should always be the whole viewport with mOrigin = (0,0).
> 2. Refresh driver triggers next tick due to whatever reason, this time only
> partial region, where the mask layer positioned, would be updated.
> 3. Start second time composition, this time only update where mask layer is
> positioned. So, the same mRenderTarget with non-zero mOrigin is used for
> BasicCompositor to do partial updating. Then, the non-zero mOrigin leads to
> a non-zero offset translate in mask layer painting, and then causes the
> broken result.
> 
> This means we can't get the first time composition go through the broken
> code path. 

Reftest snapshots use the compositor, and they support partial updates. So you could write a reftest that uses reftest-wait and then triggers an incremental update in the MozReftestInvalidate event. That should work.
Flags: needinfo?(roc)
Attachment #8713541 - Attachment is obsolete: true
Attachment #8721955 - Attachment is obsolete: true
Attachment #8721957 - Attachment description: Part2: Add reftest for mask layer composition. (v2) → Part2: Add reftest for mask layer composition. (v1)
Comment on attachment 8721957 [details] [diff] [review]
Part2: Add reftest for mask layer composition. (v1)

Since we already have reftest-no-accel on try, this test should work. Use fuzzy due to few distorted pixels caused by border-radius.

roc, thank you for pointing me to the right direction.
Attachment #8721957 - Flags: review?(roc)
try looks positove: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5de14450423b

except following known issues:
Linux x64 debug tc-M-e10s[Tier-2] 10, bc4 => Bug 1227730
Windows 7 debug R-e10s => Bug 1145903

Both are not related to this bug. So I'm setting the checkin-needed flag.
Keywords: checkin-needed
failed to apply:

renamed 1239564 -> Bug-1239564---Post-translate-maskSurface-to-fender.patch
applying Bug-1239564---Post-translate-maskSurface-to-fender.patch
patching file gfx/layers/basic/BasicCompositor.cpp
Hunk #1 FAILED at 428
1 out of 1 hunks FAILED -- saving rejects to file gfx/layers/basic/BasicCompositor.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1239564---Post-translate-maskSurface-to-fender.patch
Flags: needinfo?(jeremychen)
Keywords: checkin-needed
never mind my bad :)
Flags: needinfo?(jeremychen)
https://hg.mozilla.org/mozilla-central/rev/7dd1ed930b50
https://hg.mozilla.org/mozilla-central/rev/2c8df59280f9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Too late for 45, Jeremy, do you want to uplift this to 46?
(In reply to Sylvestre Ledru [:sylvestre] from comment #44)
> Too late for 45, Jeremy, do you want to uplift this to 46?

Sounds good to me. I'm setting the approval request.
Flags: needinfo?(jeremychen)
Comment on attachment 8721956 [details] [diff] [review]
Part1: Post translate maskSurface to renderTarget. (v2, carry r+:roc)

Approval Request Comment
[Feature/regressing bug #]:layout rendering w/o hardware acceleration
[User impact if declined]:
Users may see an ill-composed result on screen when setting css attributes like transform and/or animation.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]:low risk
[String/UUID change made/needed]:none
Attachment #8721956 - Flags: approval-mozilla-beta?
Comment on attachment 8721957 [details] [diff] [review]
Part2: Add reftest for mask layer composition. (v1)

Approval Request Comment
[Feature/regressing bug #]:layout reftest
[User impact if declined]:
Ill-composed result may show on screen w/o HWA if we can't test them in advance.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]:low risk
[String/UUID change made/needed]:none
Attachment #8721957 - Flags: approval-mozilla-beta?
Comment on attachment 8721956 [details] [diff] [review]
Part1: Post translate maskSurface to renderTarget. (v2, carry r+:roc)

Fix for users with out HWA. Includes new reftest, ok to uplift to beta.
Attachment #8721956 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8721957 [details] [diff] [review]
Part2: Add reftest for mask layer composition. (v1)

More tests sounds good to me.
Attachment #8721957 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You don't need to set checkin-needed for uplifts :)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #50)
> You don't need to set checkin-needed for uplifts :)

Just got Daily Release Tracking Alert email, and thought maybe there's something that I should have done. Thank you for telling me. :)
QA Whiteboard: [good first verify]
[bugday-20160323]

Able to get expected results for:

Status: RESOLVED,FIXED -> VERIFIED

Component: 
Name 	        Firefox
Version 	46.0b2
Build ID 	20160314144540
Update Channel 	beta
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS              Windows 7 SP1 x86_64

Actual Results: 
Element at its place while growing and shrinking.
border: 2px, --working
border-radius: 234px, --working

Expected Results: 
CSS animation is working and responding to border-radius in HWA disabled mode.
Is there a reason why, in the reftest added in part 2, the ref does not use translateZ at all, but the test does? This is seemingly causing it to fail under some content backends (i.e. Skia), whereas either removing the translateZ or ensuring it is present in both allows it to pass.
Flags: needinfo?(jeremychen)
(In reply to Lee Salzman [:lsalzman] from comment #54)
> Is there a reason why, in the reftest added in part 2, the ref does not use
> translateZ at all, but the test does? This is seemingly causing it to fail
> under some content backends (i.e. Skia), whereas either removing the
> translateZ or ensuring it is present in both allows it to pass.

We can't remove translateZ since using translateZ plus will-change could ensure masklayers not been merged in content side and force the code flow go through compositor side. After I did some testing, ensuring translateZ been present in both test and ref should cover this bug as well. I'll upload a patch later.
Flags: needinfo?(jeremychen)
Let transform property in reference file consistent with that in
test file.

Hi Matt, I make this change due to Comment 54.
Could you help review this? Thank you.
Attachment #8734217 - Flags: review?(matt.woodrow)
Attachment #8734217 - Flags: review?(matt.woodrow) → review+
Hi lsalzman, could you provide a link for the try-fail you mentioned in Comment 54?
Flags: needinfo?(lsalzman)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #58)
> Hi lsalzman, could you provide a link for the try-fail you mentioned in
> Comment 54?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fbd63502b0d
Flags: needinfo?(lsalzman)
Comment on attachment 8734217 [details] [diff] [review]
Bug 1239564 - fix reftest.

This patch fix the test-fail under some content backends (i.e. Skia). Please see Comment 54 & Comment 59. Since this test has been landed in beta, set uplift requests for both aurora and beta.

Approval Request Comment
[Feature/regressing bug #]: layout reftest
[User impact if declined]:
Ill-composed result may show on screen w/o HWA if we can't test them in advance.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8734217 - Flags: approval-mozilla-beta?
Attachment #8734217 - Flags: approval-mozilla-aurora?
Comment on attachment 8734217 [details] [diff] [review]
Bug 1239564 - fix reftest.

Test fix, NPOTB, ok to uplift
Attachment #8734217 - Flags: approval-mozilla-beta?
Attachment #8734217 - Flags: approval-mozilla-beta+
Attachment #8734217 - Flags: approval-mozilla-aurora?
Attachment #8734217 - Flags: approval-mozilla-aurora+
Tested on Windows 10 x64, on the build from 2016-01-13, it was reproducible.
Retested in on 47.0b3 and the bug was fixed.
[testday-20160506]
You need to log in before you can comment on or make changes to this bug.