Skip to content
This repository was archived by the owner on Jul 10, 2023. It is now read-only.

cocoa: Add a new platform-specific API that allows library consumers to specify that only the corners of a window can be transparent. #94

Merged

Conversation

pcwalton
Copy link

@pcwalton pcwalton commented May 27, 2016

By doing this, we significantly improve performance by allowing the
window server to perform occlusion culling under most of the window.

This patch relies on the private CGSRegion and the private
-[NSCGSWindow setOpaqueRegion:] APIs.

Requires servo/core-graphics-rs#50 and servo/cocoa-rs#129.

r? @metajack


This change is Reviewable

@metajack
Copy link

Mostly fine, but seems a bit more hacky than it should be. Is there a way to clean this up?

Previously, pcwalton (Patrick Walton) wrote…

cocoa: Add a new platform-specific API that allows library consumers to specify that only the corners of a window can be transparent.

By doing this, we significantly improve performance by allowing the

window server to perform occlusion culling under most of the window.

This patch relies on the private CGSRegion and the private

-[NSCGSWindow setOpaqueRegion:] APIs.

Requires servo/core-graphics-rs#50 and servo/cocoa-rs#129.

r? @metajack


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/api/cocoa/mod.rs, line 417 [r1] (raw file):

    }

    fn create_window(attrs: &WindowAttributes, transparent_corner_radius: Option<u32>)

It seems slightly weird that this is passed separately.


src/api/cocoa/mod.rs, line 1143 [r1] (raw file):

}

fn update_opaque_region_of_window_if_necessary(window: id, transparent_corner_radius: Option<u32>) {

This needs a comment to explain what it does.


Comments from Reviewable

@paulrouget
Copy link

If you don't mind, I'd like to review this too. There's something I'd like to validate first. I'll look at this on Monday.

@paulrouget
Copy link

Ok - looks fine to me.

seems a bit more hacky than it should be. Is there a way to clean this up?

I guess we could land this as a temporary solution, but the best thing to do is probably to clip in glutin directly instead or doing that in webrender via CSS border-radius in browserhtml.

@pcwalton
Copy link
Author

@metajack @paulrouget I'm confused as to what you mean by clipping in Glutin directly or doing it in WR via CSS border radius. The problem is that the Mac OS X window server doesn't know anything about what we do in our OpenGL context: it just composites buffers together. To allow it to perform occlusion culling, we need to tell it something extra, namely the region of the window that is guaranteed to be opaque. It can't work that out on its own just from the contents of the window. This is a bit of an ugly layering violation, but it's how the system works.

@pcwalton
Copy link
Author

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/api/cocoa/mod.rs, line 417 [r1] (raw file):

Previously, metajack (Jack Moffitt) wrote…

It seems slightly weird that this is passed separately.

It's because the transparent corner radius is part of the platform-specific attributes, which aren't part of the `WindowAttributes`. Would you prefer that I passed the entire set of platform-specific attributes instead of pulling out the transparent corner radius?

Comments from Reviewable

@pcwalton pcwalton force-pushed the mac-transparent-border-radius-servo branch from 6417fec to 210f7c5 Compare May 31, 2016 21:32
@pcwalton
Copy link
Author

Updated per comments

@metajack
Copy link

@bors-servo r+

Previously, pcwalton (Patrick Walton) wrote…

Updated per comments


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@bors-servo
Copy link

📌 Commit 210f7c5 has been approved by metajack

@bors-servo
Copy link

⌛ Testing commit 210f7c5 with merge 0fc9769...

bors-servo pushed a commit that referenced this pull request May 31, 2016
…etajack

cocoa: Add a new platform-specific API that allows library consumers to specify that only the corners of a window can be transparent.

By doing this, we significantly improve performance by allowing the
window server to perform occlusion culling under most of the window.

This patch relies on the private `CGSRegion` and the private
`-[NSCGSWindow setOpaqueRegion:]` APIs.

Requires servo/core-graphics-rs#50 and servo/cocoa-rs#129.

r? @metajack

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/glutin/94)
<!-- Reviewable:end -->
@bors-servo
Copy link

💔 Test failed - travis

@paulrouget
Copy link

Before, we didn't care about the window border radius, it was supported out of the box.
Today, browserhtml needs to use border-radius:3px.
Tomorrow, with this PR, servo will also need to use with_transparent_corner_radius.

Ideally, we would not have to care about the border radius in both browserhtml and Servo.

I understand there are good reasons why we ended up in this situation, but I'm wondering if there would be an easier setup.

I don't see any reason why anybody would want windows with no border radius, so maybe, if it's possible (I have honestly no idea), couldn't we just always clip the radius within glutin, with builtin values for the size of the radius, and use these builtin values to set the always-opaque regions?

Hope that makes more sense.

Also - I'm ok with the current approach. I don't want to block this. It's important.

@metajack
Copy link

metajack commented Jun 1, 2016

Windows windows have no border radius, at least in Windows 10. I suspect that Android and other mobile OSes probably also have square corners.

I don't think this invalidates your point as glutin could either do or not do this depending on platform.

@pcwalton
Copy link
Author

pcwalton commented Jun 1, 2016

@paulrouget Oh, I see what you're saying. We could get closer to that ideal by having WebRender implicitly add border radii to the display list on the Mac platform so you wouldn't have to do it in browser.html, but that would just move the complexity to Servo without much gain. It'd be more code to do it in Servo vs. one line in the CSS of browser.html (though I guess we could do it with some sort of UA stylesheet or something like that).

Still, that isn't the ideal, as we'd have to have that code somewhere in Servo. Unfortunately I just don't know of a well-supported, Apple-approved way to do what we want to do, at least without using Core Animation (which, incidentally, zwarich recently advised us against). The OS is fundamentally designed so that either (a) each app draws the rounded corners; or (b) Core Animation draws the entire UI of the app. Most apps don't have to worry about drawing anything explicitly when following route (a) because the native NSThemeFrame takes care of drawing the borders. As you may recall, I tried to go down the NSThemeFrame route at first myself. But the problem is that NSThemeFrame fundamentally assumes that the rendering of the app will happen on the CPU if not using Core Animation and only knows how to draw rounded corners on CPU using Core Graphics. (Also, it assumes that windows are opaque and paints the background on CPU.) The Apple-supported way to do all the drawing yourself is to use NSNextStepFrame (i.e. NSBorderlessWindowMask), which unfortunately doesn't contain any logic to do the rounded corners. So we're basically backed into a corner (no pun intended) here as far as I see: our least bad option seems to be to use NSNextStepFrame and essentially replicate the NSThemeFrame logic to draw the rounded corners ourselves, except on GPU instead of CPU because that's how we get our performance wins in Servo.

By the way, the reason we don't want to use Core Animation is that Core Animation is essentially designed to take over all of the GPU-based UI painting for your app. This works fine if you're using the traditional tiled rendering model, like Safari does, but we have to have very fine-grained control over custom shaders and so forth, which is something that CA doesn't support. So it isn't a good option for us. We could technically draw everything into one big in-memory texture and ship that over to CA, which would then blit the texture to the screen with the proper corner mask (which is in fact what would happen if we used NSFullSizeContentViewWindowMask), but this isn't really using the system the way it was intended to be used, and it incurs an extra window-sized texture blit for no good reason.

@pcwalton pcwalton force-pushed the mac-transparent-border-radius-servo branch from 210f7c5 to e78b723 Compare June 2, 2016 03:10
@pcwalton
Copy link
Author

pcwalton commented Jun 2, 2016

@bors-servo: r=metajack

@bors-servo
Copy link

📌 Commit e78b723 has been approved by metajack

bors-servo pushed a commit that referenced this pull request Jun 2, 2016
…etajack

cocoa: Add a new platform-specific API that allows library consumers to specify that only the corners of a window can be transparent.

By doing this, we significantly improve performance by allowing the
window server to perform occlusion culling under most of the window.

This patch relies on the private `CGSRegion` and the private
`-[NSCGSWindow setOpaqueRegion:]` APIs.

Requires servo/core-graphics-rs#50 and servo/cocoa-rs#129.

r? @metajack

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/glutin/94)
<!-- Reviewable:end -->
@bors-servo
Copy link

⌛ Testing commit e78b723 with merge c53ecc6...

@bors-servo
Copy link

💔 Test failed - travis

@pcwalton pcwalton force-pushed the mac-transparent-border-radius-servo branch from e78b723 to 49573e7 Compare June 2, 2016 22:29
@pcwalton
Copy link
Author

pcwalton commented Jun 2, 2016

@bors-servo: r=metajack

@bors-servo
Copy link

📌 Commit 49573e7 has been approved by metajack

@bors-servo
Copy link

⌛ Testing commit 49573e7 with merge b181d27...

bors-servo pushed a commit that referenced this pull request Jun 2, 2016
…etajack

cocoa: Add a new platform-specific API that allows library consumers to specify that only the corners of a window can be transparent.

By doing this, we significantly improve performance by allowing the
window server to perform occlusion culling under most of the window.

This patch relies on the private `CGSRegion` and the private
`-[NSCGSWindow setOpaqueRegion:]` APIs.

Requires servo/core-graphics-rs#50 and servo/cocoa-rs#129.

r? @metajack

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/glutin/94)
<!-- Reviewable:end -->
specify that only the corners of a window can be transparent.

By doing this, we significantly improve performance by allowing the
window server to perform occlusion culling under most of the window.

This patch relies on the private `CGSRegion` and the private
`-[NSCGSWindow setOpaqueRegion:]` APIs.

Requires servo/core-graphics-rs#50 and servo/cocoa-rs#129.
@bors-servo
Copy link

☀️ Test successful - travis

@bors-servo bors-servo merged commit 49573e7 into servo:servo Jun 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants