Closed Bug 805831 Opened 8 years ago Closed 8 years ago

Add additional short lived gradient cache for border corners

Categories

(Core :: Graphics, defect)

x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Snappy])

Attachments

(1 file, 1 obsolete file)

Some workloads are showing very large times spent generating the gradients in order to render border corners. Considering a lot of designs will have several similar borders, and a lot of borders will have multiple corners using the same gradients, or a gradient exactly reversed, we could benefit considerably from a cache when using Azure content. This patch adds an Azure-specific drawing path for common border corners and caches gradients here.

This is only guaranteed to give correct (and in some cases more correct) rendering with the Direct2D Azure and Quartz Cairo backends. For other cairo backends it could create un-antialiased border corner transitions since that at least used to be pixman's behaviour for hard stops.

On my profiles this makes up to a 30% difference in painting for certain workloads.
Attachment #675585 - Flags: review?(jmuizelaar)
Fixed some whitespace.
Attachment #675585 - Attachment is obsolete: true
Attachment #675585 - Flags: review?(jmuizelaar)
Attachment #675588 - Flags: review?(joe)
Comment on attachment 675588 [details] [diff] [review]
Add an additional cache for border corner gradients v2

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

::: gfx/2d/Types.h
@@ +121,5 @@
>  
> +  uint32_t ToABGR() const
> +  {
> +    return uint32_t(r * 255.0f) | uint32_t(g * 255.0f) << 8 |
> +      uint32_t(b * 255.0f) << 16 | uint32_t(a * 255.0f);

What am I missing, since this seems like it's going to overwrite the r bits with a.

Also, indent correctly.

::: layout/base/nsCSSRenderingBorders.cpp
@@ +53,5 @@
> +    : mColor1(aColor1.ToABGR()), mColor2(aColor2.ToABGR())
> +    , mBackendType(aBackendType)
> +  { }
> +
> +  BorderGradientCacheKey(const BorderGradientCacheKey* aOther)

Did you mean this to be BorderGradientCacheKey& aOther? As is, this isn't a copy constructor (and it seems like this class would be fine with the default copy constructor).

@@ +1273,5 @@
> +  const twoFloats gradientCoeff[4] = { { -1, +1 },
> +                                       { -1, -1 },
> +                                       { +1, -1 },
> +                                       { +1, +1 } };
> +  

whitespace (which I can't help but notice you removed from the place you copied this from :) )

@@ +1535,5 @@
> +
> +  const twoFloats centerAdjusts[4] = { { 0, +0.5 },
> +                                        { -0.5, 0 },
> +                                        { 0, -0.5 },
> +                                        { +0.5, 0 } };

indentation is wrong here
Attachment #675588 - Flags: review?(joe) → review+
(In reply to Joe Drew (:JOEDREW! \o/) from comment #2)
> Comment on attachment 675588 [details] [diff] [review]
> ::: layout/base/nsCSSRenderingBorders.cpp
> @@ +53,5 @@
> > +    : mColor1(aColor1.ToABGR()), mColor2(aColor2.ToABGR())
> > +    , mBackendType(aBackendType)
> > +  { }
> > +
> > +  BorderGradientCacheKey(const BorderGradientCacheKey* aOther)
> 
> Did you mean this to be BorderGradientCacheKey& aOther? As is, this isn't a
> copy constructor (and it seems like this class would be fine with the
> default copy constructor).

Nope, this is what the templates need.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/651c802473d5 - apparently it wasn't what something needed, since it doesn't build.
https://hg.mozilla.org/mozilla-central/rev/ea35621ef12a
https://hg.mozilla.org/mozilla-central/rev/08c38354ae7c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Whiteboard: [Snappy]
Blocks: 750871
You need to log in before you can comment on or make changes to this bug.