Closed Bug 907926 Opened 6 years ago Closed 6 years ago

Enable Azure content for windows BasicLayers

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 1 open bug)

Details

Attachments

(11 files, 1 obsolete file)

4.52 KB, patch
nrc
: review+
Details | Diff | Splinter Review
7.24 KB, patch
nrc
: review+
Details | Diff | Splinter Review
2.15 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.47 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.71 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
10.20 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.95 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
947 bytes, patch
nrc
: review+
Details | Diff | Splinter Review
1.31 KB, patch
jrmuizel
: review-
Details | Diff | Splinter Review
3.38 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.06 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
No description provided.
Comment on attachment 793696 [details] [diff] [review]
Make ContentClientBasic support Azure content

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

::: gfx/layers/client/ContentClient.cpp
@@ +86,5 @@
>  TemporaryRef<DrawTarget>
>  ContentClientBasic::CreateDTBuffer(ContentType aType,
>                                     const nsIntRect& aRect,
>                                     uint32_t aFlags,
>                                     RefPtr<DrawTarget>* aWhiteDT)

Do we support component alpha for basic layers? (I think not, right). If so, then you need to create aWhiteDT, if not you should assert aFlags does not have the component alpha flag.

@@ +94,5 @@
> +
> +  return gfxPlatform::GetPlatform()->CreateOffscreenDrawTarget(
> +    IntSize(aRect.width, aRect.height),
> +    ImageFormatToSurfaceFormat(format));
> +

nit: remove empty line

::: gfx/layers/client/ContentClient.h
@@ +140,5 @@
>                                                         const nsIntRect& aRect,
>                                                         uint32_t aFlags,
>                                                         RefPtr<gfx::DrawTarget>* aWhiteDT);
> +  virtual bool SupportsAzureContent() const
> +  { return true; }

nit: all on one line or split across four, not two.
Attachment #793696 - Flags: review?(ncameron) → review+
Comment on attachment 793697 [details] [diff] [review]
Supprt having different content/canvas backends in gfxPlatform

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

::: gfx/layers/Layers.h
@@ +385,5 @@
>    virtual already_AddRefed<gfxASurface>
>      CreateOptimalMaskSurface(const gfxIntSize &aSize);
>  
>    /**
> +   * Creates a DrawTarget for use with canvas which is optimized for 

nit: trailing whitespace

::: gfx/thebes/gfxPlatform.h
@@ +237,1 @@
>        CreateOffscreenDrawTarget(const mozilla::gfx::IntSize& aSize, mozilla::gfx::SurfaceFormat aFormat);

should this be CreateOffscreenContentDrawTarget if the other is ...CanvasDrawTarget?

::: image/src/ClippedImage.cpp
@@ +232,5 @@
>      // Create a surface to draw into.
>      mozilla::RefPtr<mozilla::gfx::DrawTarget> target;
>      target = gfxPlatform::GetPlatform()->
> +      CreateOffscreenCanvasDrawTarget(gfx::IntSize(mClip.width, mClip.height),
> +                                      gfx::FORMAT_B8G8R8A8);

Why use the canvas draw target here? Isn't it content?
Attachment #793697 - Flags: review?(ncameron) → review+
Comment on attachment 793698 [details] [diff] [review]
Azure support for windows/nsWindow

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

::: widget/windows/nsWindowGfx.cpp
@@ +358,5 @@
>              return false;
>            }
>  
> +          nsRefPtr<gfxContext> thebesContext;
> +          if (gfxPlatform::GetPlatform()->SupportsAzureContentForType(mozilla::gfx::BACKEND_CAIRO)) {

Why don't we use gfxPlatform's choice of backend here?
Comment on attachment 793698 [details] [diff] [review]
Azure support for windows/nsWindow

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

::: widget/windows/nsWindowGfx.cpp
@@ +358,5 @@
>              return false;
>            }
>  
> +          nsRefPtr<gfxContext> thebesContext;
> +          if (gfxPlatform::GetPlatform()->SupportsAzureContentForType(mozilla::gfx::BACKEND_CAIRO)) {

I think Matt is correct, since our current code is always drawing to a cairo surface to represent the window. We can't really create a D2D DT for example around a window.
Attachment #793698 - Flags: review?(bas) → review+
(In reply to Nick Cameron [:nrc] from comment #5)
> 
> ::: gfx/thebes/gfxPlatform.h
> @@ +237,1 @@
> >        CreateOffscreenDrawTarget(const mozilla::gfx::IntSize& aSize, mozilla::gfx::SurfaceFormat aFormat);
> 
> should this be CreateOffscreenContentDrawTarget if the other is
> ...CanvasDrawTarget?

Probably, I'll change it.

> 
> ::: image/src/ClippedImage.cpp
> @@ +232,5 @@
> >      // Create a surface to draw into.
> >      mozilla::RefPtr<mozilla::gfx::DrawTarget> target;
> >      target = gfxPlatform::GetPlatform()->
> > +      CreateOffscreenCanvasDrawTarget(gfx::IntSize(mClip.width, mClip.height),
> > +                                      gfx::FORMAT_B8G8R8A8);
> 
> Why use the canvas draw target here? Isn't it content?

It probably should be content in the long run, yes. Some of our platforms don't have a content backend enabled currently though, so that would fail.

I stuck with canvas to preserve the existing behaviour for now.
Assignee: nobody → matt.woodrow
Depends on: 908811
Depends on: 908822
Depends on: 908828
Depends on: 908974
Depends on: 909009
Depends on: 909000
Reopening this since it got disabled.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
The size returned from gfxWindowsSurface::GetSize() is the width/height of the clipped bounds. This isn't really all that useful unless you also take the x/y values into account.

This uses the client area instead to determine the size of the draw target, which I think is more correct.
Attachment #797014 - Flags: review?(bas)
Attachment #797013 - Flags: review? → review?(bas)
These patches seem to fix all the correctness issues that I can find.

Still probably need to look at the performance onces.
Attachment #797015 - Flags: review?(bas)
Oops, forgot about Azure's different handling of OP_SOURCE which completed broke rotated buffers.
Attachment #797015 - Attachment is obsolete: true
Attachment #797015 - Flags: review?(bas)
Attachment #797609 - Flags: review?(bas)
I think this is correct. We don't need to push a group to emulate an unbounded mask if the mask already covers the entire clipped area.

Couple of other things I noticed that we could do as follow-ups:

We're using IsOperatorBoundByMask to determine if we need to push a group. Instead, shouldn't we only be pushing a group when the boundness *differs* from what cairo implements, i.e. only OP_SOURCE?

We usually push a group when we have a fill operation and an opacity to apply. I think it would be cheaper to instead just clip and use paint, at least some of the time.

We could also apply this optimization to FillRect etc, but they currently call into DrawPattern and there's no easy way to determine if the current path covers the clipped area. Just need to refactor things a bit so we have all the required information available when we decide if we need to push group.

The current patch catches the most common use of OP_SOURCE at least, where we push a group to double buffer window drawing, and then copy it to the screen.
Attachment #797611 - Flags: review?(bas)
I'm on PTO next week, so feel free to take over landing these Bas.
Attachment #797611 - Flags: review?(bas) → review+
Comment on attachment 797014 [details] [diff] [review]
Use the ClientArea when initializing the draw target

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

I'm not sure I understand why these aren't the same :) But the change should be strictly an improvement.

::: widget/windows/nsWindowGfx.cpp
@@ +365,3 @@
>              RefPtr<mozilla::gfx::DrawTarget> dt =
>                gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(targetSurface,
> +                                                                     mozilla::gfx::IntSize(paintRect.right - paintRect.left,

I think we have the client rect stored already as a member on the widget, you might want to use that.
Attachment #797014 - Flags: review?(bas) → review+
Attachment #797013 - Flags: review?(bas) → review+
Comment on attachment 797609 [details] [diff] [review]
Don't ignore the composition operator in the Azure paths through ThebesLayerBuffer v2

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

I wonder if SOURCE really is/should be unbounded on Azure. I think it's bound by the clip in the D2D backend but I might be wrong.

::: gfx/layers/ThebesLayerBuffer.cpp
@@ +196,5 @@
>  
> +  if (aOperator == OP_SOURCE) {
> +    // OP_SOURCE is unbounded in Azure, and we really don't want that behaviour here.
> +    // We also can't do a ClearRect+FillRect since we need the drawing to happen
> +    // as an atomic operation (to prevent flickering).

Wait, how would a buffer ever get -presented- while we're in the middle of this function? That seems like a bigger problem. Maybe this is true on WinXP I guess?

Also, can we only do this for transparent surfaces. Most our surfaces are opaque and we can safely use OVER? Or are we already taking care of that in the surrounding code?
(In reply to Bas Schouten (:bas.schouten) from comment #20)
> Comment on attachment 797609 [details] [diff] [review]
> Don't ignore the composition operator in the Azure paths through
> ThebesLayerBuffer v2
> 
> Review of attachment 797609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder if SOURCE really is/should be unbounded on Azure. I think it's
> bound by the clip in the D2D backend but I might be wrong.

It's bound by the clip, but not the mask. When blitting a rotated buffer to the window, we issue multiple draw calls, but don't clip to each sub-rect individually.

> 
> ::: gfx/layers/ThebesLayerBuffer.cpp
> @@ +196,5 @@
> >  
> > +  if (aOperator == OP_SOURCE) {
> > +    // OP_SOURCE is unbounded in Azure, and we really don't want that behaviour here.
> > +    // We also can't do a ClearRect+FillRect since we need the drawing to happen
> > +    // as an atomic operation (to prevent flickering).
> 
> Wait, how would a buffer ever get -presented- while we're in the middle of
> this function? That seems like a bigger problem. Maybe this is true on WinXP
> I guess?

It was true on windows 7 with DWM too iirc. We (roc and I) had a simple testcase with a solid background colour, and different coloured layerized rectangles on top of it. We were seeing definite flickering when scrolling on windows, but not on OSX. See http://hg.mozilla.org/mozilla-central/rev/88a10532e555.

> 
> Also, can we only do this for transparent surfaces. Most our surfaces are
> opaque and we can safely use OVER? Or are we already taking care of that in
> the surrounding code?

Is this because OP_SOURCE is slower with d2d? I think we shouldn't be putting backend specific decisions like that into the calling code.
Attachment #797609 - Flags: review?(bas) → review?(roc)
Comment on attachment 802724 [details] [diff] [review]
Re-enable cairo azure content for windows

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

I take it we have a successful try run with d2d disabled? r=me if so.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +505,5 @@
>      }
>  #endif
>  
>      uint32_t canvasMask = 1 << BACKEND_CAIRO;
> +    uint32_t contentMask = BACKEND_CAIRO;

1 << BACKEND_CAIRO, right?
Attachment #802724 - Flags: review?(ncameron) → review+
Let's back out right away, then figure out what to do longer term.
This is a lot faster on windows.
Attachment #805756 - Flags: review?(bas)
This often requires us to allocate a gfxWindowsSurface or similar.
Attachment #805769 - Flags: review?(bas)
This fixes a couple of cases that I saw in profiles where we ended up having multiple Path objects around for the same DrawTarget, which forces cairo to make a (slow) copy of the older one.
Attachment #805800 - Flags: review?(bas)
Blocks: 907463
Comment on attachment 805756 [details] [diff] [review]
Use EXTEND_NONE for DrawSurface

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

Hrm, this technically is incorrect behavior when scaling is applied. I'm not sure what the right solution is though.
Attachment #805769 - Flags: review?(bas) → review+
Comment on attachment 805800 [details] [diff] [review]
Try avoid having multiple path objects around since this is slow with Cairo

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

::: layout/base/nsCSSRenderingBorders.cpp
@@ +1579,5 @@
>      builder->LineTo(strokeEnd);
>      RefPtr<Path> path = builder->Finish();
>      dt->Stroke(path, ColorPattern(Color::FromABGR(mBorderColors[i])), StrokeOptions(mBorderWidths[i]));
> +    builder = nullptr;
> +    path = nullptr;

I'd like to understand why this makes a difference, we should really figure out a way to deal with this in the backend - however, for now, let's just do it.
Attachment #805800 - Flags: review?(bas) → review+
(In reply to Bas Schouten (:bas.schouten) from comment #34)
> Comment on attachment 805800 [details] [diff] [review]
> Try avoid having multiple path objects around since this is slow with Cairo
> 
> Review of attachment 805800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsCSSRenderingBorders.cpp
> @@ +1579,5 @@
> >      builder->LineTo(strokeEnd);
> >      RefPtr<Path> path = builder->Finish();
> >      dt->Stroke(path, ColorPattern(Color::FromABGR(mBorderColors[i])), StrokeOptions(mBorderWidths[i]));
> > +    builder = nullptr;
> > +    path = nullptr;
> 
> I'd like to understand why this makes a difference, we should really figure
> out a way to deal with this in the backend - however, for now, let's just do
> it.

Cairo paths are usually stored on the context, and making a discrete copy of them is painfully slow. For this reason the Cairo Azure backend attempts to keep the path stored on the context, and only copies it out when a second path object is created for the same DrawTarget.

In this case both the existing PathBuilder* and Path* are holding references to the current path, and then creating a new PathBuilder object creates a new path. We have no way of knowing that the existing path won't ever be used again, so we have to copy it first.

Ensuring that all references to the existing path are gone before we create a new one avoids this.
(In reply to Bas Schouten (:bas.schouten) from comment #33)
> Comment on attachment 805756 [details] [diff] [review]
> Use EXTEND_NONE for DrawSurface
> 
> Review of attachment 805756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hrm, this technically is incorrect behavior when scaling is applied. I'm not
> sure what the right solution is though.

Can you please explain why this is? This was the one patch that actually made a noticeable difference to talos scores (tsvgr_opacity at least).
Flags: needinfo?(bas)
Comment on attachment 805756 [details] [diff] [review]
Use EXTEND_NONE for DrawSurface

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

This will give wrong edge filtering
Attachment #805756 - Flags: review?(bas) → review-
Depends on: 917703
(In reply to Matt Woodrow (:mattwoodrow) from comment #35)
> (In reply to Bas Schouten (:bas.schouten) from comment #34)
> > Comment on attachment 805800 [details] [diff] [review]
> > Try avoid having multiple path objects around since this is slow with Cairo
> > 
> > Review of attachment 805800 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/base/nsCSSRenderingBorders.cpp
> > @@ +1579,5 @@
> > >      builder->LineTo(strokeEnd);
> > >      RefPtr<Path> path = builder->Finish();
> > >      dt->Stroke(path, ColorPattern(Color::FromABGR(mBorderColors[i])), StrokeOptions(mBorderWidths[i]));
> > > +    builder = nullptr;
> > > +    path = nullptr;
> > 
> > I'd like to understand why this makes a difference, we should really figure
> > out a way to deal with this in the backend - however, for now, let's just do
> > it.
> 
> Cairo paths are usually stored on the context, and making a discrete copy of
> them is painfully slow. For this reason the Cairo Azure backend attempts to
> keep the path stored on the context, and only copies it out when a second
> path object is created for the same DrawTarget.
> 
> In this case both the existing PathBuilder* and Path* are holding references
> to the current path, and then creating a new PathBuilder object creates a
> new path. We have no way of knowing that the existing path won't ever be
> used again, so we have to copy it first.
> 
> Ensuring that all references to the existing path are gone before we create
> a new one avoids this.

We need to work on a way to make this more efficient in the cairo backend longer term. Future Moz2D-ifications shouldn't need to live with this terrible restriction. Since paths are discrete objects in cairo already there's no -need- for this to be expensive except limitations in the cairo public API.
Flags: needinfo?(bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0042a2722aab
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec63e6d6d874
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae02054863f7

Looks like this still is going to regress tresize somewhat, I'll look at this tomorrow.

Getting this landed now so that we can get as much testing as possible before the merge.
https://hg.mozilla.org/mozilla-central/rev/1c153d5c8bdd
https://hg.mozilla.org/mozilla-central/rev/ad266fafd429
https://hg.mozilla.org/mozilla-central/rev/e844f4ed3e1f
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 919471
Depends on: 922515
Depends on: 926023
Depends on: 935110
Depends on: 1060268
Depends on: 1176761
You need to log in before you can comment on or make changes to this bug.