Closed
Bug 907926
Opened 11 years ago
Closed 11 years ago
Enable Azure content for windows BasicLayers
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
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.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #793696 -
Flags: review?(ncameron)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #793697 -
Flags: review?(ncameron)
Assignee | ||
Comment 3•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8f78d15188ce
Attachment #793698 -
Flags: review?(bas)
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
(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 | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf4201e29c80 https://hg.mozilla.org/integration/mozilla-inbound/rev/ea426252b4f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/6092de8ae0a9
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → matt.woodrow
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d46d0f4f4a8a
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bf4201e29c80 https://hg.mozilla.org/mozilla-central/rev/ea426252b4f7 https://hg.mozilla.org/mozilla-central/rev/6092de8ae0a9 https://hg.mozilla.org/mozilla-central/rev/d46d0f4f4a8a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Assignee | ||
Comment 12•11 years ago
|
||
Reopening this since it got disabled.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #797013 -
Flags: review?
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #797013 -
Flags: review? → review?(bas)
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
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)
Assignee | ||
Comment 18•11 years ago
|
||
I'm on PTO next week, so feel free to take over landing these Bas.
Updated•11 years ago
|
Attachment #797611 -
Flags: review?(bas) → review+
Comment 19•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #797013 -
Flags: review?(bas) → review+
Comment 20•11 years ago
|
||
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?
Assignee | ||
Comment 21•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Attachment #797609 -
Flags: review?(bas) → review?(roc)
Attachment #797609 -
Flags: review?(roc) → review+
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #802724 -
Flags: review?(ncameron)
Comment 23•11 years ago
|
||
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+
Assignee | ||
Comment 24•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/78f1f02b6b36 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f85a996314 https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d9ee3b3538 https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e569516f31 https://hg.mozilla.org/integration/mozilla-inbound/rev/091d82cb377e
Comment 25•11 years ago
|
||
It looks like this bug significantly regressed the tab animation test (TART) on Windows XP: http://graphs.mozilla.org/graph.html#tests=[[293,63,37]]&sel=1376315455050,1378907455050&displayrange=30&datatype=running https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/LKHX09DG66M
Comment 26•11 years ago
|
||
Let's back out right away, then figure out what to do longer term.
Comment 27•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea9ee28a3654 https://hg.mozilla.org/integration/mozilla-inbound/rev/1aaa574b3863 https://hg.mozilla.org/integration/mozilla-inbound/rev/e42f0bf501e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/1eede7e170a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/a368b28a51e9
Assignee | ||
Comment 28•11 years ago
|
||
Ah, sad. Relanded everything except the enable switch for the sake of my patch queue: https://hg.mozilla.org/integration/mozilla-inbound/rev/7502c616895d https://hg.mozilla.org/integration/mozilla-inbound/rev/15e9e36b7abc https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb3561451c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/8334dcf9e395
Whiteboard: [leave open]
Comment 29•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7502c616895d https://hg.mozilla.org/mozilla-central/rev/15e9e36b7abc https://hg.mozilla.org/mozilla-central/rev/3eb3561451c3 https://hg.mozilla.org/mozilla-central/rev/8334dcf9e395
Assignee | ||
Comment 30•11 years ago
|
||
This is a lot faster on windows.
Attachment #805756 -
Flags: review?(bas)
Assignee | ||
Comment 31•11 years ago
|
||
This often requires us to allocate a gfxWindowsSurface or similar.
Attachment #805769 -
Flags: review?(bas)
Assignee | ||
Comment 32•11 years ago
|
||
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)
Comment 33•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #805769 -
Flags: review?(bas) → review+
Comment 34•11 years ago
|
||
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+
Assignee | ||
Comment 35•11 years ago
|
||
(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.
Assignee | ||
Comment 36•11 years ago
|
||
(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 37•11 years ago
|
||
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-
Comment 38•11 years ago
|
||
(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)
Assignee | ||
Comment 39•11 years ago
|
||
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.
Comment 40•11 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/25ee493a6e17 so I could get at bug 740200 to back it out.
Assignee | ||
Comment 41•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3616b786e8f https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb0d1053128 https://hg.mozilla.org/integration/mozilla-inbound/rev/94a6733b01dc
Whiteboard: [leave open]
Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6d48465290
Assignee | ||
Comment 43•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c153d5c8bdd https://hg.mozilla.org/integration/mozilla-inbound/rev/ad266fafd429 https://hg.mozilla.org/integration/mozilla-inbound/rev/e844f4ed3e1f
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: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•