Closed
Bug 1335159
Opened 8 years ago
Closed 8 years ago
Plane-splitting has caused breakage on Spotify's Flash-using UI
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox53 | --- | fixed |
firefox54 | --- | fixed |
People
(Reporter: wisniewskit, Assigned: mikokm)
References
Details
(Keywords: regression, testcase, Whiteboard: [webcompat])
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
text/x-review-board-request
|
mattwoodrow
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
For those of us still getting the Spotify Flash UI, the plane-splitting patches in bug 1286716 seem to have regressed it so that content below the fold vanishes, though it is still interactive to some extent.
I've attached a reduced test-case to showcase the glitch; it reproduces for me in Linux, OSX 10.11.6, and a new Windows 10 Edge VM, either with HWA on or off.
Based on the test-case I would suspect that this could be causing issues on other sites as well.
(Note that mozregression was what implicated the plane-splitting bug; see the "see also" link for more context if desired).
Updated•8 years ago
|
Flags: needinfo?(mikokm)
Comment 1•8 years ago
|
||
Hmm, I think the testcase might be a slightly different (or a slightly more specific) form of the Spotify issue -- that, or the original spotify regression range was mistaken.
Because, mozregression is giving me this regression range for the attached testcase (on Linux):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7884f9ed756bad6e9d73b8a5adb82d0773daae08&tochange=aeda115ca5c37723ec703a2574204952001e40db
And in that range, bug 1323797 looks like the culprit.
Keywords: regression,
testcase
Comment 2•8 years ago
|
||
(Chatted briefly with twisniewski about comment 1 - I think he's going to spin off another bug for the original Spotify issue which regressed from bug 1286716. Let's keep this bug about the attached reduced testcase, which represents its own regression, perhaps distinct from the Spotify issue.)
Flags: needinfo?(twisniewski)
Assignee | ||
Comment 3•8 years ago
|
||
Thank you so much Thomas for creating a test case.
I think this bug is caused by invalid texture coordinate calculations in https://dxr.mozilla.org/mozilla-central/source/gfx/layers/Compositor.cpp#227. I will look into this right away.
Assignee: nobody → mikokm
Status: NEW → ASSIGNED
Flags: needinfo?(mikokm)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #1)
> Hmm, I think the testcase might be a slightly different (or a slightly more
> specific) form of the Spotify issue -- that, or the original spotify
> regression range was mistaken.
>
> Because, mozregression is giving me this regression range for the attached
> testcase (on Linux):
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=7884f9ed756bad6e9d73b8a5adb82d0773daae08&tochange=aeda
> 115ca5c37723ec703a2574204952001e40db
> And in that range, bug 1323797 looks like the culprit.
The regression range might be different depending on if GPU acceleration is enabled or not. Before bug 1323797 the software layers implementation did not support non-rectangular layers.
Reporter | ||
Comment 5•8 years ago
|
||
Indeed I think you're right, miko. I just ran mozregression on OSX 10.11 on my testcase. With HWA on, I got my initial range with the plane-splitting (1286716) as the culprit, but with HWA off, I got the same result dholbert did: BasicCompositor triangle layers (1323797).
Comment 6•8 years ago
|
||
Aha! Sneaky. --> restoring the classification of this as a regression from bug 1286716 then.
Blocks: 1286716
Updated•8 years ago
|
Flags: needinfo?(twisniewski)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
The cause of the bug was incomplete transformation of layer geometry, which resulted in layers getting culled when they shouldn't have. This patch adds the missing inverse transform and a reftest.
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8832285 [details]
Bug 1335159 - Also invert layer local transform
https://reviewboard.mozilla.org/r/108618/#review109760
Attachment #8832285 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 10•8 years ago
|
||
Keywords: checkin-needed
Updated•8 years ago
|
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
Flags: in-testsuite+
Version: Trunk → 53 Branch
Comment 12•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d6c96a5afbcf
Also invert layer local transform r=mattwoodrow
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 14•8 years ago
|
||
Please request Aurora approval on this patch when you get a chance.
Flags: needinfo?(mikokm)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8832285 [details]
Bug 1335159 - Also invert layer local transform
Approval Request Comment
[Feature/Bug causing the regression]: Plane splitting implementation for OpenGL (bug 1286716, bug 689498)
[User impact if declined]: Broken Spotify web UI, and other websites that use nested layers and scrolling.
[Is this code covered by automated tests?]: Yes, there is a reftest covering the code and verifying the fix.
[Has the fix been verified in Nightly?]: Yes, I have verified that the fix works in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: Optional - there is an automated reftest. Manual testing is possible by opening testcase.html attachment.
[List of other uplifts needed for the feature/fix]: Bug 1333934 fixes another regression, and touches the same code. To avoid potential issues, it would be good to uplift both patches.
[Is the change risky?]: Medium.
[Why is the change risky/not risky?]: The change is a medium risk because it affects all the CSS 3D transforms. Fortunately, the code changes are only three lines in total, pass the previous reftest suite, and have new reftests to verify the expected behavior.
[String changes made/needed]: None.
Flags: needinfo?(mikokm)
Attachment #8832285 -
Flags: approval-mozilla-aurora?
Sounds like we should get the patch from bug 1333934 too before we try to uplift this.
Good that there's lots of test coverage, but it does sound like it might be a broad change that could cause regressions.
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> Sounds like we should get the patch from bug 1333934 too before we try to
> uplift this.
> Good that there's lots of test coverage, but it does sound like it might be
> a broad change that could cause regressions.
I agree that it is a little risky to modify a code path used by most CSS transforms. However because of that, I also think that there could be more regressions if these patches are not uplifted. The test case in this bug is very simple, which makes it all the more likely that other sites besides Spotify might be broken.
This feature is behind a pref flag (layers.geometry.(d3d11|basic|opengl).enabled), which makes it easy to disable this feature if something goes wrong.
Comment 18•8 years ago
|
||
Comment on attachment 8832285 [details]
Bug 1335159 - Also invert layer local transform
Fix a broken Spotify web UI. Aurora53+. Bug 1333934 should be uplift, too.
Attachment #8832285 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•