It looks like we have two options to fix clipping: 1) Reimplement CombineWithGeometry. I have a plan/algorithm for doing this based on "A new algorithm for computing Boolean operations on polygons". In preliminary testing it performs at least as good D2D's and shouldn't suffer from the sloppiness problems. 2) Instead of computing the intersected clip we can accumulate it using Layers. Layer based clipping seems to be implemented by accumulating a list of Geometries. During rendering, we do a separate pass for each layer passing it through the rasterized geometry. This means that we shouldn't have to pay any unnecessary memory cost for Layers. The Layer based approach also seems like it should have similar performance characteristics to cairo's current implementation of clipping on win32. I'm leaning toward #2 as the better option.

(In reply to comment #0) > 2) Instead of computing the intersected clip we can accumulate it using Layers. > Layer based clipping seems to be implemented by accumulating a list of > Geometries. During rendering, we do a separate pass for each layer passing it > through the rasterized geometry. This means that we shouldn't have to pay any > unnecessary memory cost for Layers. The Layer based approach also seems like it > should have similar performance characteristics to cairo's current > implementation of clipping on win32. I'm not sure I completely understand this option. Are we talking Direct2D layers?

Yes, D2D layers.

(In reply to comment #2) > Yes, D2D layers. In that case I'm not sure what you mean, I can see how it can be done by pushing a layer for each path. But that would increase memory cost. I'm not sure how you'd mean doing it in multiple passes.

(In reply to comment #3) > (In reply to comment #2) > > Yes, D2D layers. > > In that case I'm not sure what you mean, I can see how it can be done by > pushing a layer for each path. But that would increase memory cost. I'm not > sure how you'd mean doing it in multiple passes. Yes, I meaning pushing a layer for each path. From my experimentation, it looks this does not cause an increase in memory cost beyond path storage. The multi-pass rendering that I described above is how D2D appears to draw when you use multiple layers

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Yes, D2D layers. > > > > In that case I'm not sure what you mean, I can see how it can be done by > > pushing a layer for each path. But that would increase memory cost. I'm not > > sure how you'd mean doing it in multiple passes. > > Yes, I meaning pushing a layer for each path. From my experimentation, it looks > this does not cause an increase in memory cost beyond path storage. I may have been wrong about this...

Another problem with the layer approach is that I'm not sure how we should handle CLEAR

I worked around the clear problem by only handling clear of rectangular regions. This seems sufficient for most uses. I've done no real performance testing and would be interested to see results.

My proof of concept seems to be really slow when resizing text areas.

(In reply to comment #11) > My proof of concept seems to be really slow when resizing text areas. It seems like we're spending 50% of the time here zeroing pages (MmZeroPageThread) My guess is the cause of this is texture allocation, which seems to be required for the layers patch. This may make this approach infeasible.

(In reply to comment #12) > (In reply to comment #11) > > My proof of concept seems to be really slow when resizing text areas. > > It seems like we're spending 50% of the time here zeroing pages > (MmZeroPageThread) > My guess is the cause of this is texture allocation, which seems to be required > for the layers patch. This may make this approach infeasible. I've investigated this further. It looks like there's a set of problems causing this. 1. We reuse full clip paths but not clip path parents. We can fix this by checking for a common ancestor in the clip path chain. 2. We only use PushAxisAlignedClip for boxes not regions. This causes us to use Layers when we get the regions created in the expose event handler instead of the cheaper PushAxisAlignedClip. 3. We build up a clip path based on the region in the expose event. We could simplify the region to a single rectangle. I'm not sure what the performance impact of such a change would be. I suspect we'll need to do some measurement to see what works best for D2D. I'd also like to better understand how PushAxisAlignedClip is implemented so that I can have a better idea what the performance implications of it are.

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > My proof of concept seems to be really slow when resizing text areas. > > > > It seems like we're spending 50% of the time here zeroing pages > > (MmZeroPageThread) > > My guess is the cause of this is texture allocation, which seems to be required > > for the layers patch. This may make this approach infeasible. > > I've investigated this further. It looks like there's a set of problems causing > this. > > 3. We build up a clip path based on the region in the expose event. We could > simplify the region to a single rectangle. I'm not sure what the performance > impact of such a change would be. I suspect we'll need to do some measurement > to see what works best for D2D. I'd also like to better understand how > PushAxisAlignedClip is implemented so that I can have a better idea what the > performance implications of it are. The bounding boxes are not really an option, I experimented with this (to avoid having to use layers in the traditional clipping model), but it didn't really work well, in some cases (like a bookmark link) the invalidation region covers the entire status war, and a little rectangle around the bookmark link button. The bounding box would make us redraw pretty much the entire window, as opposed to maybe 30k pixels. Also, I'd be -very- surprised if PushAxisAlignedClip was doing enything else than using scissor rects (after all the intersection of any number of axis aligned rectangles is still an axis aligned rectangle). The scissor test comes practically for free. So AxisAlignedClips are very cheap. I think we need to see if we can just cache layers more efficiently. After all we're currently also required to use layers for these complex regions generated by the event handler, so this at least would not have to cause a regression as far as I can see.

(In reply to comment #14) > > The bounding boxes are not really an option, I experimented with this (to avoid > having to use layers in the traditional clipping model), but it didn't really > work well, in some cases (like a bookmark link) the invalidation region covers > the entire status war, and a little rectangle around the bookmark link button. > The bounding box would make us redraw pretty much the entire window, as opposed > to maybe 30k pixels. What if we used a client side clipping scheme. Instead of setting the clip and then drawing in a single pass. We could do a separate paint for each box in the invalid region. Does that seem sane? > > Also, I'd be -very- surprised if PushAxisAlignedClip was doing enything else > than using scissor rects (after all the intersection of any number of axis > aligned rectangles is still an axis aligned rectangle). The scissor test comes > practically for free. So AxisAlignedClips are very cheap. > How would anti-aliased clips be implemented though? Scissor rects won't do that afaik. > I think we need to see if we can just cache layers more efficiently. After all > we're currently also required to use layers for these complex regions generated > by the event handler, so this at least would not have to cause a regression as > far as I can see. Indeed.

(In reply to comment #13) > 2. We only use PushAxisAlignedClip for boxes not regions. This causes us to use > Layers when we get the regions created in the expose event handler instead of > the cheaper PushAxisAlignedClip. This idea doesn't make sense and can be ignored.

(In reply to comment #15) > (In reply to comment #14) > > > > What if we used a client side clipping scheme. Instead of setting the clip and > then drawing in a single pass. We could do a separate paint for each box in the > invalid region. Does that seem sane? > I think that could work, I'm not sure how bad that will be for the layout code to deal with though. Potentially we'd be wasting a lot by drawing the widget background multiple times and such. > > > > Also, I'd be -very- surprised if PushAxisAlignedClip was doing enything else > > than using scissor rects (after all the intersection of any number of axis > > aligned rectangles is still an axis aligned rectangle). The scissor test comes > > practically for free. So AxisAlignedClips are very cheap. > > > > How would anti-aliased clips be implemented though? Scissor rects won't do that > afaik. Certainly a good point.

The version will reuse clip path ancestors. This let's avoid recreating the same clip area over and over again. This fixes the text area problem that I noticed.

There are builds at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmuizelaar@mozilla.com-ccde50b70c75/tryserver-win32/ with this change. It would be great if people could try them out and let me know of any performance changes.

Performance seems about the same to me or a little faster, though I can confirm bug 556196 is fixed in this try build on Win7.

Bug 556196 fixed here too. Grafxbot test for latest nightly http://brasstacks.mozilla.com/resultserv/data/testrun/217/ is similar to this build http://brasstacks.mozilla.com/resultserv/data/testrun/215/ Peacekeeper goes from about 3700 in latest nightly to 4000, although I noticed some 2-3 second paused during the 'complex graphics' section. http://www.craftymind.com/factory/guimark2/HTML5GamingTest.html is down to 18fps compared to 50-60fps in latest nightly. The other HTML5 tests have equal performance with latest nightly. Any other tests to try?

(In reply to comment #21) > Bug 556196 fixed here too. > http://www.craftymind.com/factory/guimark2/HTML5GamingTest.html is down to > 18fps compared to 50-60fps in latest nightly. The other HTML5 tests have equal > performance with latest nightly. > > Any other tests to try? Odd, this is still a lot better than non-D2D (atleast for me), but I wonder why it goes down so much.

(In reply to comment #22) For comparison I get about 12fps in both this build and the latest nightly with D2D DW turned off

(In reply to comment #22) > (In reply to comment #21) > > Bug 556196 fixed here too. > > > http://www.craftymind.com/factory/guimark2/HTML5GamingTest.html is down to > > 18fps compared to 50-60fps in latest nightly. The other HTML5 tests have equal > > performance with latest nightly. > > > > Any other tests to try? > > Odd, this is still a lot better than non-D2D (atleast for me), but I wonder why > it goes down so much. It looks like this is largely caused by antialiased axisalignedclips. Switching to aliased ones gets much of the performance back. I'll see what we can do to avoid the problem.

This cleans up the previous versions and fixes some bugs. Bas, I haven't gone over it myself, so it's likely a little rough yet, but I thought I'd get it up sooner rather than later. This patch still regresses the guimark2 demo, but I have some ideas and patches for handling that.

Comment on attachment 453502 [details] [diff] [review] v4 cleaned up version I think we should get this stuff in to fix the majority of the issues. But should on the very short term find a solution for the various sorts of fallback scenarios introduced here that I really think are a fairly bad idea! I think on the short term a CombineWithGeometry plus rounding solution. And on the slightly longer term drawing the mask to a texture using a temporary surface and then using D3D to composite them as you suggested on IRC. >+#pragma warning(disable : 4244) // 'conversion' conversion from 'type1' to 'type2', possible loss of data >+ Ugh, I'm not fond of this, I don't like implicitly truncating. But whatever is the coding style in Cairo I guess. >@@ -331,16 +325,17 @@ static const cairo_surface_backend_t cai > static void > _d2d_clear_surface(cairo_d2d_surface_t *surf) > { > surf->rt->BeginDraw(); > surf->rt->Clear(D2D1::ColorF(0, 0)); > surf->rt->EndDraw(); > } > >+ Don't think this is intentional >+ d2dsurf->rt->PushLayer(D2D1::LayerParameters( >+ D2D1::InfiniteRect(), >+ geom, >+ D2D1_ANTIALIAS_MODE_PER_PRIMITIVE, >+ D2D1::IdentityMatrix(), >+ 1.0, >+ 0, >+ options), >+ layer); We may want to see if bounding the layer to the bounding rectangle of the mask makes for a performance improvement. But I'd be surprised if D2D doesn't do that internally. >+/* finds the lowest common ancestor of a and b */ >+static cairo_clip_path_t * >+find_common_ancestor(cairo_clip_path_t *a, cairo_clip_path_t *b) >+{ >+ int a_depth = 0, b_depth = 0; >+ >+ cairo_clip_path_t *x; >+ >+ /* find the depths of the clip_paths */ >+ x = a; >+ while (x) { >+ a_depth++; >+ x = x->prev; >+ } >+ >+ x = b; >+ while (x) { >+ b_depth++; >+ x = x->prev; >+ } >+ >+ /* rewind the deeper chain to the depth of the shallowest chain */ >+ while (b_depth < a_depth && a) { >+ a = a->prev; >+ a_depth--; >+ } >+ >+ while (a_depth < b_depth && b) { >+ b = b->prev; >+ b_depth--; >+ } We could remove this last loop by doing: while (x) { if (a_depth > b_depth) { b_depth++; } else { b = b->prev; } x = x->prev; } But if this function isn't called a lot I'm more in favor of you code which is more readable. >+ >+ /* walk back until we find a common ancesstor */ >+ >+ /* b will be null if and only if a is null because the depths >+ * must match at this point */ I suggest we assert this. >+cairo_status_t >+_cairo_d2d_set_clip (cairo_d2d_surface_t *d2dsurf, cairo_clip_t *clip) >+{ >+ >+ if (clip == NULL && d2dsurf->clip.path == NULL) >+ return CAIRO_STATUS_SUCCESS; reset_clip will already check this right? Do we need this path? > >+static cairo_int_status_t >+_cairo_d2d_clear (cairo_d2d_surface_t *d2dsurf, >+ cairo_clip_t *clip) >+{ >+ cairo_region_t *region; >+ cairo_int_status_t status; >+ >+ if (!clip) { >+ _begin_draw_state(d2dsurf); >+ reset_clip(d2dsurf); > d2dsurf->rt->Clear(D2D1::ColorF(0, 0)); >- return; >+ return CAIRO_INT_STATUS_SUCCESS; > } > >- RefPtr<IDXGISurface> dxgiSurface; >- RefPtr<ID2D1Bitmap> bitmp; >+ status = _cairo_clip_get_region (clip, &region); So as per IRC discussion, I really think we need a better answer to this, perhaps using the rounding trick just in this case. The same goes for clearing of more complex fills and strokes. On the longer run we can do the D3D combine trick we discussed. This shouldn't be too hard. >+ /* Create a GDI region for the cairo region */ You're not ... are you? > if (op != CAIRO_OPERATOR_OVER && op != CAIRO_OPERATOR_ADD && >- op != CAIRO_OPERATOR_CLEAR) { >+ 1) {//op != CAIRO_OPERATOR_CLEAR) { This needs cleaning :)

(In reply to comment #26) > (From update of attachment 453502 [details] [diff] [review]) > >+#pragma warning(disable : 4244) // 'conversion' conversion from 'type1' to 'type2', possible loss of data > >+ > > Ugh, I'm not fond of this, I don't like implicitly truncating. But whatever is > the coding style in Cairo I guess. I took it out for now and added the casts. > >+ d2dsurf->rt->PushLayer(D2D1::LayerParameters( > >+ D2D1::InfiniteRect(), > >+ geom, > >+ D2D1_ANTIALIAS_MODE_PER_PRIMITIVE, > >+ D2D1::IdentityMatrix(), > >+ 1.0, > >+ 0, > >+ options), > >+ layer); > > We may want to see if bounding the layer to the bounding rectangle of the mask > makes for a performance improvement. But I'd be surprised if D2D doesn't do > that internally. I checked this and didn't see a difference. > >+/* finds the lowest common ancestor of a and b */ > >+static cairo_clip_path_t * > >+find_common_ancestor(cairo_clip_path_t *a, cairo_clip_path_t *b) > >+{ > > But if this function isn't called a lot I'm more in favor of you code which is > more readable. It's called once per operation. However the clip path depth is usually quite small (2-3) so it shouldn't matter. If it shows up on profiles we can always fix it. > >+cairo_status_t > >+_cairo_d2d_set_clip (cairo_d2d_surface_t *d2dsurf, cairo_clip_t *clip) > >+{ > >+ > >+ if (clip == NULL && d2dsurf->clip.path == NULL) > >+ return CAIRO_STATUS_SUCCESS; > > reset_clip will already check this right? Do we need this path? Nope we don't need this path, I took it out. Good catch.

Here's what I plan on pushing

Has this been pushed? If not, something else has broken http://www.craftymind.com/factory/guimark2/HTML5GamingTest.html so it only 'plays' when you are moving the mouse. The UI becomes mostly unresponsive to mouse clicks. The only thing that really responds is the awesomebar, which will only show a transparent rectangle where the results should be. The other Html5 tests on the site continue to work as normal. With build Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100629 Minefield/4.0b2pre

(In reply to comment #29) > Has this been pushed? > > If not, something else has broken > http://www.craftymind.com/factory/guimark2/HTML5GamingTest.html so it only > 'plays' when you are moving the mouse. The UI becomes mostly unresponsive to > mouse clicks. The only thing that really responds is the awesomebar, which will > only show a transparent rectangle where the results should be. The other Html5 > tests on the site continue to work as normal. > > With build Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) > Gecko/20100629 Minefield/4.0b2pre This hasn't been pushed, did you go anything special to get this to occur? I can't reproduce this neither on the nightly or on my own builds. How's memory usage during this scenario?

(In reply to comment #30) > This hasn't been pushed, did you go anything special to get this to occur? I > can't reproduce this neither on the nightly or on my own builds. How's memory > usage during this scenario? I've opened FF 3.6.6 since seeing this issue and have been trying to recreate it to no avail. I think it was just user error.

(In reply to comment #29) > Has this been pushed? > > If not, something else has broken > http://www.craftymind.com/factory/guimark2/HTML5GamingTest.html so it only > 'plays' when you are moving the mouse. The UI becomes mostly unresponsive to > mouse clicks. The only thing that really responds is the awesomebar, which will > only show a transparent rectangle where the results should be. The other Html5 > tests on the site continue to work as normal. I think I may have seen something like this. It sounds like it might be unrelated to this patch though.

Seems like this patch has reduced frame rates in things like FishIE, GUIMark 2. I know AA reduces framerates in games and such but is it also affecting Fx?

(In reply to comment #33) > Seems like this patch has reduced frame rates in things like FishIE, GUIMark 2. > I know AA reduces framerates in games and such but is it also affecting Fx? It is, there are a lot of cases that we can still optimize though! So don't give up hope. But for now getting it into a 'correct' state it our highest priority.

Nice, so maybe we mark this fixed do a followup for optimizing it?

http://hg.mozilla.org/mozilla-central/rev/2539a6c4e0c9 I have plans for fixing the perf bugs in bug 576169 and bug 576170

Anywhay i think we need to add a bug for fixing this performance drop

(In reply to comment #37) > Anywhay i think we need to add a bug for fixing this performance drop Like the ones listed in comment 36?

(In reply to comment #38) > (In reply to comment #37) > > Anywhay i think we need to add a bug for fixing this performance drop > Like the ones listed in comment 36? Yes like this one, because the drop is big in some test