Closed Bug 1335159 Opened 3 years ago Closed 3 years ago

Plane-splitting has caused breakage on Spotify's Flash-using UI

Categories

(Core :: Graphics, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: wisniewskit, Assigned: miko)

References

Details

(Keywords: regression, testcase, Whiteboard: [webcompat])

Attachments

(2 files)

Attached file testcase.html
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).
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.
Blocks: 1323797
No longer blocks: 1286716
Keywords: regression, testcase
(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)
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)
(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.
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).
Aha! Sneaky. --> restoring the classification of this as a regression from bug 1286716 then.
Blocks: 1286716
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 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+
Duplicate of this bug: 1334830
Flags: in-testsuite+
Version: Trunk → 53 Branch
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d6c96a5afbcf
Also invert layer local transform r=mattwoodrow
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d6c96a5afbcf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora approval on this patch when you get a chance.
Flags: needinfo?(mikokm)
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.
(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 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+
You need to log in before you can comment on or make changes to this bug.