Vibrancy was backed out in bug 1367385 due to Talos regressions. However, the browser UI was disabled for the specific tests that were regressed, with the exception of tp5o_scroll. Bug 1377281 is taking care of that though, and once that lands we should be able to land vibrancy without regressing Talos.

I think we should worry about the compact light/dark themes in a separate bug.

Comment on attachment 8882387 [details] Bug 1377284 - Enable titlebar vibrancy on macOS. https://reviewboard.mozilla.org/r/153496/#review158740 I'm not really familiar with the specifics of tp5o_scroll. Please explain why it's okay to regress the thing tested by tp5o_scroll with the UI enabled. r- until this is cleared up.

I finally got this to reproduce on my MacMini after I got it upgraded to Yosemite (which first introduced vibrancy support). Profiling the WindowServer, it's pretty clear this is where the time is being spent. Over a ~2min profile of tp5o_scroll, the WindowServer spends about 18.8% of its time running gl_composite_inactive_backdrops. It spends no time in there in the "good" case (without vibrancy). So my take away here is that this regression is because OS X's WindowServer is doing more work to composite the vibrancy effect, and that the tp5o_scroll test, which drives the compositor pretty hard I guess, picks it up. canuckistani, is this enough to make a call here on what to do?

dolske brought up a good point - it's not clear why the WindowServer needs to recomposite the whole window when we're just scrolling the content area. I ran the test on the MacMini again with paint flashing. One thing I noticed was that the caret in the location bar was flashing during the test. I wonder if its rect is close enough to the tab strip to cause us to re-composite it for each flash. I'll run the test on the MacMini again with the location bar not focused.

clearing my needinfo

So I talked to mstange about this a little today, and here's what I found out. There are two stages of composition: 1. Gecko composites all of the different layers, including content area, chrome area, etc, to produce the final GL Context (or maybe texture - I can't remember which term I should use here) which is the full window. 2. After Gecko composites, macOS WindowServer then takes what the Gecko compositor produces and does its own round of composition with the background stuff behind the windows to produce the final pixels that get sent to the screen. Vibrancy causes the second step to become more expensive. Also, it doesn't matter that nothing in the titlebar area is repainting - we're sending the whole window area to the WindowServer, and it's interpreting this as "Oh, the whole window needs to be recomposited", and then does it. So any regression here should probably be expressed in milliseconds per composite. It might be possible to tell CoreAnimation / the WindowServer that the chrome and content are two separate entities and that when we scroll in tp5o_scroll, that only the content part has changed and that the chrome area has not. This would be a huge engineering effort, and would not be possible in time for 57, according to mstange. The next step is to compute the cost per composite so that somebody can make a call here. I'm keeping my needinfo set. Other browsers: Chrome gets around this by having only the content area be a GL Context, and skips the vibrancy effect entirely. Safari uses a different form of vibrancy that only cares about the content that's been scrolled inside of it.

So I looked at a profile where we captured the amount of time it took for the compositor to do a composite (which, according to mstange, will also effectively capture the time it takes for macOS to do its composition step once its pipeline gets filled up). No Vibrancy Vibrancy Delta Average 1.96 2.32 0.36 Median 1.98 2.10 0.12 So with vibrancy, it looks like we take a small hit on every composite. On average, a fraction of a millisecond - 0.36ms to be precise. Is there any more information required here to make a decision?

ni? dolske, for that last, since he was talking to me earlier about ensuring we had enough data here to make an informed decision.

[Copying from Slack, after I realized the discussion should really be here...] Yeah, I think we're at the point now where we can start to make a decision on the tradeoffs. I feel like the two main choices are: 1) ship it. it's a small impact in absolute terms (~0.3ms per frame), so in normal usage you're probably not going to see any difference 2) don't ship. they typical argument is that you've got a 16.6ms budget to have the browser run at 60fps, and every bit you take away from than means you're that much more likely to run out of time and drop frames. This is consuming 1.8% of that budget, (apparently as a constant cost, due to how we interact with the OS compositor?) Two other bits might be useful to know: * Does the GFX team have an opinion on if this is worth it? i.e. how does this compare to other things we've run across, and the status-quo / future direction in terms of gfx performance? * If we decide to not ship this now, it taking a "major engineering effort" makes it sound like we effectively will never be able to ship this. (Unless that just just meant as "too big for 57 now, but entirely feasible to improve for a release soon after?) [Jeff suggested having it controlled by the toolkit.cosmeticAnimations.enabled pref] Having it be controlled by the perf pref seems like a good idea; it's technically not an animation, but if someone's on a struggling system that would be an obvious thing to flip to eke out a bit more performance. (Although, frankly, for that small a cost it seems unlikely to make a noticeable difference) Also, AIUI (and I could be wrong?) the 16.6ms budget concern is less of an issue now with OMTC / OOP compositing? Pre-E10S this was hammered on because that 16.6ms was shared by _everything_ the browser was doing, and Every Millisecond Was Precious™. [mconley mentioned that we have a COMPOSITE_TIME probe] Looking at that probe for just Yosemite-and-newer (where this would be used?) for Release... https://mzl.la/2uNW5yk 97.69% of frames are composited in <= 16ms, so AIUI that gives a sense of potential impact. (Although 0.87% are > 18ms already, so it depends how you want to look at it.)

(I would welcome a GFX sanity-check as to if my assumptions/understanding above is valid or not.)

(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #8) > Safari uses a different form of vibrancy that only cares about the content > that's been scrolled inside of it. How feasible is it for us to do the same? I expect it's either not feasible at all or at least too much work for 57, but I thought I'd ask.

(In reply to Dão Gottwald [::dao] from comment #13) > How feasible is it for us to do the same? I expect it's either not feasible > at all or at least too much work for 57, but I thought I'd ask. From my read of [1], the in-window blending effect requires us to be using Core Animation layers, which I don't think we do - and I suspect would be non-trivial to implement. That's my take on the situation though - ni'ing mstange to confirm. [1]: See https://developer.apple.com/documentation/appkit/nsvisualeffectview for details

It would be non-trivial to implement, but more feasible than the project to have an OpenGL context for the content that does not overlap with the vibrancy effect. However, I'm not sure it's even wanted from the UX perspective. I've done some work on it before in bug 1050804, which was then wontfixed.

(In reply to Justin Dolske [:Dolske] from comment #11) > Two other bits might be useful to know: > > * Does the GFX team have an opinion on if this is worth it? i.e. how does > this compare to other things we've run across, and the status-quo / future > direction in terms of gfx performance? Speaking as a GFX team adjacent person: I think it's worth taking this hit. We're currently far from being bound on composition time on Mac; all our time is usually either spent on the content side (display list construction, layer building, rasterization) or on texture upload. The only time where the additional overhead from vibrancy will make a difference is if it changes the GPU workload from "just under one frame" to "just over one frame", e.g. from 16.5ms to 16.7ms, which should be extremely rare. Most pages are very quick to composite and don't involve much GPU work at the moment. It's harder to say how this will interact with WebRender. WebRender will drastically increase the load we place on the GPU during compositing, so the vibrancy overhead might be more noticeable. But I think it's too early to tell whether this will actually make a difference in practice. > * If we decide to not ship this now, it taking a "major engineering effort" > makes it sound like we effectively will never be able to ship this. I agree. > (Unless > that just just meant as "too big for 57 now, but entirely feasible to > improve for a release soon after?) Unlikely, I think. > [Jeff suggested having it controlled by the > toolkit.cosmeticAnimations.enabled pref] > > Having it be controlled by the perf pref seems like a good idea; it's > technically not an animation, but if someone's on a struggling system that > would be an obvious thing to flip to eke out a bit more performance. > (Although, frankly, for that small a cost it seems unlikely to make a > noticeable difference) An extra pref seems like overkill to me. There already exists a system-wide pref to disable vibrancy ("Display" -> "Reduce Transparency" in the accessibility preferences) which concerned users can use instead. > (Although 0.87% > are > 18ms already, so it depends how you want to look at it.) I wouldn't be too concerned by these numbers because they don't measure GPU workload, they just measure how long we're waiting for the system to finish presenting the frame. If this takes longer than 16ms, that's usually because the system is waiting for vsync because there's already a frame in the queue, and it doesn't want to queue up too much.

Sounds to me like the relevant parties have weighed in. I think we should just go ahead and do this now.

Actually, redirecting needinfo? to dao - hopefully the above information satisfies performance concerns, and you're willing to take another look at nhnt11's patch.

(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #18) > Actually, redirecting needinfo? to dao - hopefully the above information > satisfies performance concerns pdol or jgriffiths should make this call.

Redirecting to canuckistani, since he's already got the context of this bug. Hey canuckistani - it sounds like we've got buy-in from mstange to try to land the patch. I think we're just hoping for an explicit sign-off on doing this before landing.

(In reply to Markus Stange [:mstange] from comment #16) > > [Jeff suggested having it controlled by the > > toolkit.cosmeticAnimations.enabled pref] > > > > Having it be controlled by the perf pref seems like a good idea; it's > > technically not an animation, but if someone's on a struggling system that > > would be an obvious thing to flip to eke out a bit more performance. > > (Although, frankly, for that small a cost it seems unlikely to make a > > noticeable difference) > > An extra pref seems like overkill to me. There already exists a system-wide > pref to disable vibrancy ("Display" -> "Reduce Transparency" in the > accessibility preferences) which concerned users can use instead. Ah, I didn't know about that. Given the extremely minimal cost of this, and availability of an OS pref, that seem fine to me. Thanks for the info, I'm still comfortable that enabling vibrancy is the right decision here.

Comment on attachment 8882387 [details] Bug 1377284 - Enable titlebar vibrancy on macOS. https://reviewboard.mozilla.org/r/153496/#review168102 (In reply to Markus Stange [:mstange] from comment #16) > It's harder to say how this will interact with WebRender. WebRender will > drastically increase the load we place on the GPU during compositing, so the > vibrancy overhead might be more noticeable. But I think it's too early to > tell whether this will actually make a difference in practice. This seems somewhat concerning. Last I heard WebRender won't make it into 57, so guess we don't need to worry too much about this right now, but I'd still like jgriffiths to make this call. r=me code-wise

(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #20) > Redirecting to canuckistani, since he's already got the context of this bug. > > Hey canuckistani - it sounds like we've got buy-in from mstange to try to > land the patch. I think we're just hoping for an explicit sign-off on doing > this before landing. Yup, let's do it.

Looks like we're good to go here! Perhaps let's try to land this after merge?

Yaaaaaas! *finger hovers over land button*

(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #24) > Looks like we're good to go here! Perhaps let's try to land this after merge? No need to wait. This is guarded by the MOZ_PHOTON_THEME ifdef.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s df05a45cd52c -d 58c1de79d494: rebasing 410839:df05a45cd52c "Bug 1377284 - Enable titlebar vibrancy on macOS. r=dao" (tip) merging browser/themes/osx/browser.css warning: conflicts while merging browser/themes/osx/browser.css! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3ba4ec951216 Enable titlebar vibrancy on macOS. r=dao

(In reply to Dão Gottwald [::dao] from comment #26) > No need to wait. This is guarded by the MOZ_PHOTON_THEME ifdef. Absolutely right - my mistake. :)

(In reply to Markus Stange [:mstange] from comment #16) > It's harder to say how this will interact with WebRender. WebRender will > drastically increase the load we place on the GPU during compositing, so the > vibrancy overhead might be more noticeable. But I think it's too early to > tell whether this will actually make a difference in practice. > I'm concerned about this performance hit, especially as we move more load to the GPU with WebRender. Taking a 0.2ms hit compared to the other browsers on OS X puts us at a systematic disadvantage. I think we should do more investigation into about how big this performance hit is on a wider variety of systems. It's also worth checking what the impact of this change is on battery life during something like video playback. Finally, It would be good to know if this is using up a noticeable amount of memory bandwidth that will slow down the entire browser. Who's the best person to do this investigation?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #32) > I think we should do more > investigation into about how big this performance hit is on a wider variety > of systems. It's also worth checking what the impact of this change is on > battery life during something like video playback. Finally, It would be good > to know if this is using up a noticeable amount of memory bandwidth that > will slow down the entire browser. We'd need a wider variety of macOS hardware then, I guess. Perhaps SoftVision is our best choice - they probably have the most varied collection of supported hardware. This didn't cause any AWSY flags to be thrown, so that's at least a point in favour for memory, unless you're talking about something that AWSY would not detect. For power consumption, I wonder if a before-and-after run of ./mach power with a video running on various hardware would be sufficient? > Who's the best person to do this investigation? Hey igoldan, how much Apple desktop hardware do you have access to?

(In reply to Mike Conley (:mconley) - (PTO starting Aug 11 - Back Aug 17) from comment #33) > This didn't cause any AWSY flags to be thrown, so that's at least a point in > favour for memory, unless you're talking about something that AWSY would not > detect. As I understand it, the concern is about WebRender once it's mature, which it isn't yet. So we probably don't have meaningful perfherder results if perfherder even uses this configuration in the first place.

Mike, we do have a wide collection of Mac devices, with all OSX versions you need.

Ovidiu, you asked me on IRC for help to verify this. This bug adds a translucent/blurred effect to the background of the tab-strip on supporting macOS versions (starting with 10.10 - Yosemite). Let me know if this helps.

Thanks Nihanth, I verified on Mac OS X 10.10 and 10.12 with FF Nightly 57.0a1(2017-09-12) and I can confirm the fix, I have a translucent effect on the background of the tab-strip, these results are for the default theme. For dark and light themes I saw that there's an open bug 1386721, what are the plans for this? Is this bug going to be fixed in 57 FF version or 58 FF version?

For light and dark themes, the vibrancy to tabstrip will be tracked in bug 1386721. Based on previous comment mark this bug verified as fixed for the Default theme.