Last Comment Bug 675410 - [10.7] Hardware acceleration on: The corners at the bottom have no fade out effect / anti-aliasing
: [10.7] Hardware acceleration on: The corners at the bottom have no fade out e...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal with 1 vote (vote)
: mozilla22
Assigned To: André Reinald
:
: Markus Stange [:mstange]
Mentors:
: 748782 (view as bug list)
Depends on: 676248
Blocks: lion-compatibility 676241
  Show dependency treegraph
 
Reported: 2011-07-30 00:17 PDT by Mehmet
Modified: 2013-03-27 18:59 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Firefox_vs_Safari.png (153.58 KB, image/png)
2011-07-30 00:17 PDT, Mehmet
no flags Details
wip (18.45 KB, patch)
2011-08-01 07:57 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
wip (20.68 KB, patch)
2011-08-02 08:05 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
v1 (14.84 KB, patch)
2012-10-25 14:57 PDT, Markus Stange [:mstange]
b56girard: review-
Details | Diff | Splinter Review
Updated Markus' patch and took into account Benoit's comments (16.22 KB, patch)
2013-03-08 09:27 PST, André Reinald
b56girard: review+
Details | Diff | Splinter Review
Patch DrawWindowOverlay and MaybeDrawRoundedBottomCorners the same way (17.57 KB, patch)
2013-03-15 07:48 PDT, André Reinald
no flags Details | Diff | Splinter Review
Cleaned previous patch - replacing it (17.57 KB, patch)
2013-03-15 12:00 PDT, André Reinald
no flags Details | Diff | Splinter Review
Bottom rounded corners are ok even on a retina display using the same openGL context as the rest of the window. (17.79 KB, patch)
2013-03-20 04:43 PDT, André Reinald
no flags Details | Diff | Splinter Review
Changes reflecting Markus' comments. (18.15 KB, patch)
2013-03-22 06:52 PDT, André Reinald
no flags Details | Diff | Splinter Review
cf. comment #21 (20.04 KB, patch)
2013-03-22 07:59 PDT, André Reinald
mstange: review+
b56girard: review+
Details | Diff | Splinter Review

Description Mehmet 2011-07-30 00:17:26 PDT
Created attachment 549554 [details]
Firefox_vs_Safari.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7) AppleWebKit/534.48.3 (KHTML, like Gecko) Version/5.1 Safari/534.48.3

Steps to reproduce:

Run Latest Trunk on 10.7 Lion and take a look at the corners at the bottom.


Actual results:

The corners at the bottom have no fade out effect.


Expected results:

The corners at the bottom should have a fade out effect.
Comment 1 Markus Stange [:mstange] 2011-07-30 06:03:39 PDT
Yes, OS X cuts us off without anti-aliasing because we're using OpenGL :(
It's possible to recreate a smooth anti-aliased rounded corner even with OpenGL, but it won't look perfect because the three pixels in the corner always stay completely transparent (while proper anti-aliasing would need two of them to be slightly opaque).
Comment 2 Markus Stange [:mstange] 2011-07-31 14:29:43 PDT
We can't say "I want square corners on my window", or at least I haven't found a documented way of doing that. OS X just cuts off the bottom corners of all windows with title bars, and for windows that use OpenGL it does so without anti-aliasing.

But now I've found one way of stopping OpenGL context bottom corner cutoff, using brute force:

extern "C" {
  typedef CFTypeRef CGSRegionObj;
  CGError CGSNewRegionWithRect(const CGRect *rect, CGSRegionObj *outRegion);
}

@interface NSSurface @end
@implementation NSSurface(DontCutOffCorners)
- (CGSRegionObj)_createRoundedBottomRegionForRect:(CGRect)rect
{
  // Create a normal rect region without rounded bottom corners.
  CGSRegionObj region;
  CGSNewRegionWithRect(&rect, &region);
  return region;
}
@end
Comment 3 Markus Stange [:mstange] 2011-08-01 07:55:20 PDT
Actually, scratch the last two comments. I was testing with a 3px corner radius. With a corner radius of 4.5px, the three pixels in each corner are completely transparent anyway, so the forced erasure doesn't hurt us.
Comment 4 Markus Stange [:mstange] 2011-08-01 07:57:17 PDT
Created attachment 549799 [details] [diff] [review]
wip
Comment 5 Markus Stange [:mstange] 2011-08-02 08:05:00 PDT
Created attachment 550077 [details] [diff] [review]
wip
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-25 09:24:05 PDT
*** Bug 748782 has been marked as a duplicate of this bug. ***
Comment 7 Justin Dolske [:Dolske] 2012-09-30 01:29:46 PDT
With the hidpi bugs that have just landed, this got a bit worse -- the corners are still "rounded" the same way, but the rounding appears to be done at low-resolution. Any possibility we can at least get this rounding at full res? Or is this just completely controlled by OSX?
Comment 8 Markus Stange [:mstange] 2012-10-25 14:57:57 PDT
Created attachment 675321 [details] [diff] [review]
v1
Comment 9 Markus Stange [:mstange] 2012-10-25 14:59:37 PDT
(In reply to Justin Dolske [:Dolske] from comment #7)
> With the hidpi bugs that have just landed, this got a bit worse -- the
> corners are still "rounded" the same way, but the rounding appears to be
> done at low-resolution. Any possibility we can at least get this rounding at
> full res? Or is this just completely controlled by OSX?

It's controlled by OSX, but the patch now deactivates that control and lets us do our own rounding. It looks really good in HiDPI mode :)

https://tbpl.mozilla.org/?tree=Try&rev=8b914e5bdff0
Comment 10 Josh Aas 2012-10-26 07:32:21 PDT
Comment on attachment 675321 [details] [diff] [review]
v1

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

You're much better off having Benoit Girard review this.

::: widget/cocoa/nsCocoaWindow.h
@@ +113,2 @@
>  - (void)setBottomCornerRounded:(BOOL)rounded;
> +- (BOOL)bottomCornerRounded;

What is this for? If this is a hidden API or something please document it.
Comment 11 Benoit Girard (:BenWa) 2012-10-26 09:29:04 PDT
Comment on attachment 675321 [details] [diff] [review]
v1

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

::: widget/cocoa/nsChildView.mm
@@ +1866,5 @@
> +                                         LOCAL_GL_CLAMP_TO_EDGE,
> +                                         TextureImage::UseNearestFilter);
> +
> +    // Creation of texture images can fail.
> +    if (!mCornerMaskImage)

I wonder if we rather crash in this case. It's unexpected and likely means we're out of memory. Simply returning here isn't a good way to handle it because we will try again the next frame and get into a bad performance loop.

I'd almost rather just see this show up in crash stats if it's a problem and if so properly handle the failure.

@@ +1869,5 @@
> +    // Creation of texture images can fail.
> +    if (!mCornerMaskImage)
> +      return;
> +
> +    nsIntRegion update(nsIntRect(0, 0, cornerRadius, cornerRadius));

This should be devPixelCornerRadius

@@ +1876,5 @@
> +      mCornerMaskImage = nullptr;
> +      return;
> +    }
> +
> +    NS_ABORT_IF_FALSE(asurf->GetType() == gfxASurface::SurfaceTypeQuartz,

There's no guarantee this will be a quartz surface. And its make refactoring that method fragile. It would be perfectly valid to return a gfxImageSurface. Maybe it's safer to create your own quartz surface and simply upload that to the texture yourself.
Comment 12 André Reinald 2013-03-08 09:27:42 PST
Created attachment 722844 [details] [diff] [review]
Updated Markus' patch and took into account Benoit's comments

The bottom round corners became square. I guess it's a result of changing cornerRadius to devPixelCornerRadius. Maybe you have a better suggestion. I'm probably not the best person to care of this bug as it seems to involve much more knowledge as I currently have on layers. But I'm willing to learn.
Comment 13 Benoit Girard (:BenWa) 2013-03-08 10:06:24 PST
Comment on attachment 722844 [details] [diff] [review]
Updated Markus' patch and took into account Benoit's comments

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

::: widget/cocoa/nsChildView.mm
@@ +1910,5 @@
>        mResizerImage = nullptr;
>        return;
>      }
>  
> +    // is the CairoSurface of a non QuartzSurface usable in the gfxQuartzSurface constructor ?

No it's not :(
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-quartz-surface.c#3604

I think the right thing to do here would be to allocate a Quartz Surface, draw to it and then upload that instead of trying to draw using Begin/EndUpdate which doesn't guarantee to provide a quartz surface (it could for example draw to the texture using skiagl).

Feel free to have this work like before since it is a pre-existing problem and this patch doesn't make it worse.
Comment 14 André Reinald 2013-03-15 07:48:04 PDT
Created attachment 725401 [details] [diff] [review]
Patch DrawWindowOverlay and MaybeDrawRoundedBottomCorners the same way

I tried another way of building the gfxQuartzSurface in which corners mask and resizer picture were drawn, that was using lower level cairo-quartz APIs. But it didn't work out for incompatible (and even not assignable) types of images: I could not create mCornerMaskImage and mResizerImage that way.

I also realized in the process that those surfaces likely had to share the LayerManager passed as parameter, so they all had the same GL context, which is the point for unblocking c. So the only way was indeed to use the gl()->CreateTextureImage method, even though its drawback is it doesn't guarantee we have the right type of supporting surface underneath.

Sorry, I found no better for now.

Also, the bottom corners are not rounded in Mac OS 10.8. I still have to investigate this. But at least my patch should allow work on 676241 to begin.
Comment 15 André Reinald 2013-03-15 12:00:15 PDT
Created attachment 725523 [details] [diff] [review]
Cleaned previous patch - replacing it
Comment 16 André Reinald 2013-03-15 12:02:49 PDT
Comment on attachment 725523 [details] [diff] [review]
Cleaned previous patch - replacing it

Forgot to mark previous attachment (725401) as obsolete. Sorry.
Comment 17 André Reinald 2013-03-20 04:43:11 PDT
Created attachment 727145 [details] [diff] [review]
Bottom rounded corners are ok even on a retina display using the same openGL context as the rest of the window.
Comment 18 Benoit Girard (:BenWa) 2013-03-20 06:49:36 PDT
I wont have time to review until early next week. Feel free to move the review if you don't want to wait.
Comment 19 Markus Stange [:mstange] 2013-03-20 08:07:54 PDT
Comment on attachment 727145 [details] [diff] [review]
Bottom rounded corners are ok even on a retina display using the same openGL context as the rest of the window.

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm
>@@ -47,16 +47,17 @@
> #endif
> 
> #include "gfxContext.h"
> #include "gfxQuartzSurface.h"
> #include "nsRegion.h"
> #include "Layers.h"
> #include "LayerManagerOGL.h"
> #include "GLTextureImage.h"
>+#include "GLContext.h"

Is this still necessary?

>+@implementation NSSurface(DontCutOffCorners)
>+- (CGSRegionObj)_createRoundedBottomRegionForRect:(CGRect)rect
>+{
>+  // Create a normal rect region without rounded bottom corners.
>+  CGSRegionObj region;
>+  CGSNewRegionWithRect(&rect, &region);
>+  return region;
>+  }

wrong indent before }

>+    // is the CairoSurface of a non QuartzSurface usable in the gfxQuartzSurface constructor ?

I don't think so. I think these are the options we have here:
 1. Don't bother - if the surface is not a Quartz surface, print a warning and return.
 2. Use gfxQuartzNativeDrawing to get a Quartz surface.
 3. Don't use a CGContext at all and convert the drawing to use gfxContext APIs.

Option 3 would probably be the cleanest, option 1 the quickest.

>diff --git a/widget/cocoa/nsCocoaWindow.mm b/widget/cocoa/nsCocoaWindow.mm
>--- a/widget/cocoa/nsCocoaWindow.mm
>+++ b/widget/cocoa/nsCocoaWindow.mm
>@@ -2808,17 +2808,17 @@ static const NSString* kStateShowsToolba
>     [super setBackgroundColor:mColor];
>     mBackgroundColor = [[NSColor whiteColor] retain];
> 
>     mUnifiedToolbarHeight = 0.0f;
> 
>     // setBottomCornerRounded: is a private API call, so we check to make sure
>     // we respond to it just in case.
>     if ([self respondsToSelector:@selector(setBottomCornerRounded:)])
>-      [self setBottomCornerRounded:NO];
>+      [self setBottomCornerRounded:nsCocoaFeatures::OnMountainLionOrLater()];

This is not strictly necessary, because starting with Lion the window ignores calls to setBottomCornerRounded and returns YES from bottomCornerRounded anyway, but I can see how that can be confusing. If you want to make this change we should use OnLionOrLater().
Comment 20 André Reinald 2013-03-22 06:52:18 PDT
Created attachment 728199 [details] [diff] [review]
Changes reflecting Markus' comments.

See comment 19.
Comment 21 Markus Stange [:mstange] 2013-03-22 07:01:20 PDT
Comment on attachment 728199 [details] [diff] [review]
Changes reflecting Markus' comments.

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm
>@@ -43,20 +43,22 @@
> #include "nsMenuUtilsX.h"
> #include "nsMenuBarX.h"
> #ifdef __LP64__
> #include "ComplexTextInputPanel.h"
> #endif
> 
> #include "gfxContext.h"
> #include "gfxQuartzSurface.h"
>+#include "gfxQuartzNativeDrawing.h"

You're not using this.

>+    if (asurf->GetType() != gfxASurface::SurfaceTypeQuartz) {
>+      NS_WARN_IF_FALSE(FALSE, "mResizerImage's surface is not Quartz");
>+      mCornerMaskImage = nullptr;

This needs to be mResizerImage.

So now, if the surfaces are not Quartz surfaces, we attempt to recreate them on every paint and spam the console with warnings. That's not beautiful it's OK I guess.
Comment 22 André Reinald 2013-03-22 07:59:06 PDT
Created attachment 728222 [details] [diff] [review]
cf. comment #21
Comment 23 Benoit Girard (:BenWa) 2013-03-25 11:15:21 PDT
(In reply to Markus Stange from comment #21)
> >+    if (asurf->GetType() != gfxASurface::SurfaceTypeQuartz) {
> >+      NS_WARN_IF_FALSE(FALSE, "mResizerImage's surface is not Quartz");
> >+      mCornerMaskImage = nullptr;
> 
> This needs to be mResizerImage.
> 
> So now, if the surfaces are not Quartz surfaces, we attempt to recreate them
> on every paint and spam the console with warnings. That's not beautiful it's
> OK I guess.

IMO stuff like that should just crash. If it happens I'd like crash-stats to tell us about it loudly rather then having the performance go to zero users that may never report the problem.
Comment 24 André Reinald 2013-03-26 09:36:15 PDT
(In reply to Benoit Girard (:BenWa) from comment #23)
> IMO stuff like that should just crash. If it happens I'd like crash-stats to
> tell us about it loudly rather then having the performance go to zero users
> that may never report the problem.

For now it seems to work fine for all of us. But what if the patch crashes on all users when they upgrade to (say) 10.9? That would force us to patch in emergency. IMO it's preferable to have FF work in a sub-optimal mode rather than not work at all. Still waiting to ask for commit.
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-03-27 10:59:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7917cae45600
Comment 26 André Reinald 2013-03-27 13:05:51 PDT
Sorry it's not yet clear to me how I should mark this patch for landing in mozilla-central. I just marked it resolved + fixed for now.
Comment 27 Steven Michaud [:smichaud] (Retired) 2013-03-27 14:14:02 PDT
Actually it's normal now for patches destined for mozilla-central to land on mozilla-inbound first.  Others are responsible for watching the tree on mozilla-inbound, and backing out patches that cause trouble.  Generally the developer has to do this on mozilla-central.

Mozilla-inbound is merged with mozilla-central about once a day.  Whoever does this will mark all relevant bugs "fixed".

("Trouble" includes breaking the tree, of course.  But it can also include persistent test failures or significant performance regressions.)
Comment 28 Ryan VanderMeulen [:RyanVM] 2013-03-27 18:59:03 PDT
https://hg.mozilla.org/mozilla-central/rev/7917cae45600

Note You need to log in before you can comment on or make changes to this bug.