Status

()

defect
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

(Depends on 2 bugs)

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

No description provided.
Posted patch work in progress (obsolete) — Splinter Review
Depends on: 544348
Bas,

Can you review the direct2d/directwrite changes?
Attachment #427655 - Flags: review?(bas.schouten)
One known problem with this is a hang/crash on linux/tsvg
Comment on attachment 427655 [details] [diff] [review]
v2 fixes a bunch of test failures

>@@ -244,20 +244,17 @@ _cairo_d2d_show_glyphs (void			*surface,
>  * \param extents Pointer to where to store the extents
>  * \param CAIRO_ERROR_SURFACE_TYPE_MISTMATCH or CAIRO_STATUS_SUCCESS
>  */
>-static cairo_int_status_t
>+static cairo_bool_t
> _cairo_d2d_getextents(void		       *surface,
> 		      cairo_rectangle_int_t    *extents);
>

Nit: The backend renamed this to get_extents, we should rename the function too for consistency.

>@@ -1605,9 +1606,11 @@ static cairo_int_status_t
> _cairo_d2d_paint(void			*surface,
> 		 cairo_operator_t	 op,
> 		 const cairo_pattern_t	*source,
>-		 cairo_rectangle_int_t  *extents)
>+		 cairo_clip_t		*clip)
> {
>     cairo_d2d_surface_t *d2dsurf = static_cast<cairo_d2d_surface_t*>(surface);
>+    cairo_int_status_t status;
>+
>     _begin_draw_state(d2dsurf);
>     if (op == CAIRO_OPERATOR_CLEAR) {
> 	cairo_solid_pattern_t *sourcePattern =
>@@ -1617,6 +1620,10 @@ _cairo_d2d_paint(void			*surface,
> 	return CAIRO_INT_STATUS_SUCCESS;
>     }
> 
>+    status = (cairo_int_status_t)_cairo_surface_clipper_set_clip (&d2dsurf->clipper, clip);
>+    if (unlikely(status))
>+	return status;
>+

This needs to happen before _begin_draw_state in all functions. Depending on the function it might be best to move _begin_draw_state down or move this one up. _begin_draw_state actually pushes the clip. In this order the clip will never be applied.

Note the glyph drawing code deserves particular attention. Since clipper_set_clip is done in cairo-dwrite-font.cpp but _begin_draw_state is currently done in cairo-d2d-surface.cpp
Attachment #427655 - Flags: review?(bas.schouten) → review-
We should enable the Cairo tests in our tree once this update is finished. (And maybe the cairo trace suites in talos?)
Posted patch v3 (obsolete) — Splinter Review
Attachment #423838 - Attachment is obsolete: true
Attachment #427655 - Attachment is obsolete: true
Bas, can you review the d2d changes?
Attachment #431187 - Attachment is obsolete: true
Attachment #432638 - Flags: review?(bas.schouten)
It feels like something is still wrong with x stuff...

(firefox-bin:6685): Gdk-CRITICAL **: gdk_screen_get_display: assertion `GDK_IS_SCREEN (screen)' failed

(firefox-bin:6685): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(firefox-bin:6685): GLib-GObject-CRITICAL **: g_signal_handlers_disconnect_matched: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

(firefox-bin:6685): GLib-GObject-CRITICAL **: g_object_ref: assertion `G_IS_OBJECT (object)' failed

(firefox-bin:6685): Gdk-CRITICAL **: gdk_screen_get_display: assertion `GDK_IS_SCREEN (screen)' failed

(firefox-bin:6685): GLib-GObject-CRITICAL **: g_object_get_data: assertion `G_IS_OBJECT (object)' failed

(firefox-bin:6685): Gdk-CRITICAL **: gdk_screen_get_display: assertion `GDK_IS_SCREEN (screen)' failed

(firefox-bin:6685): GLib-GObject-CRITICAL **: g_object_get_data: assertion `G_IS_OBJECT (object)' failed

(firefox-bin:6685): Gdk-CRITICAL **: gdk_screen_get_display: assertion `GDK_IS_SCREEN (screen)' failed

(firefox-bin:6685): GLib-GObject-CRITICAL **: g_object_get_data: assertion `G_IS_OBJECT (object)' failed

(firefox-bin:6685): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.14.1/gobject/gsignal.c:2180: invalid unclassed object pointer for value type `GdkScreen'

(firefox-bin:6685): Gdk-CRITICAL **: gdk_screen_get_display: assertion `GDK_IS_SCREEN (screen)' failed

(firefox-bin:6685): GLib-GObject-CRITICAL **: g_object_get_data: assertion `G_IS_OBJECT (object)' failed

(firefox-bin:6685): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.14.1/gobject/gsignal.c:2180: invalid unclassed object pointer for value type `GdkScreen'

(firefox-bin:6685): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed
This causes a tsvg regression on linux. The largest change happens in hixie-007.xml which is a stroked world map. Here are the results:

old: hixie-007.xml;1613;1612.25;1609;1629;1609;1629;1612;1614;1614           
new: hixie-007.xml;3973;3972.75;3972;4006;3973;4006;3973;3972;3973

I believe this is caused by running the tesellator on stroked paths, which is a correctness fix. If we disable stroking we get the following results:
           
old: hixie-007.xml;1100;1099   ;1093;1111;1111;1103;1093;1097;1103
new: hixie-007.xml;1039;1038.25;1034;1050;1034;1040;1041;1050;1038

which seems to confirm the hypothesis.

None of the other platforms should suffer from this problem.
If it's linux only, that's unfortunate, but probably fine...
We also seem to have a tp4 regression on linux. It seems to be worst on chinese pages we makes me lean towards the glyph cache, however I don't know that there have been many changes in that area. This might be tricky to track down...
(In reply to comment #11)
> We also seem to have a tp4 regression on linux. It seems to be worst on chinese
> pages we makes me lean towards the glyph cache, however I don't know that there
> have been many changes in that area. This might be tricky to track down...

This patch includes large chunks of changes which have some things to do with the glyph cache.  I think at its current state, it is extremely hard to tell what's wrong with this patch.  I also know that bisecting against cairo changesets is tricky because we have to merge our cairo patches.

Jeff, what does shark on the chinese pages tell you?
(In reply to comment #12)
> (In reply to comment #11)
> > We also seem to have a tp4 regression on linux. It seems to be worst on chinese
> > pages we makes me lean towards the glyph cache, however I don't know that there
> > have been many changes in that area. This might be tricky to track down...
> 
> This patch includes large chunks of changes which have some things to do with
> the glyph cache.  I think at its current state, it is extremely hard to tell
> what's wrong with this patch.  I also know that bisecting against cairo
> changesets is tricky because we have to merge our cairo patches.
> 
> Jeff, what does shark on the chinese pages tell you?

The problem is only noticeable on linux (the only place we use the glyph cache). I'll do some glyph cache instrumentation tomorrow to see if I can find a change in behaviour.
OS: Mac OS X → All
This causes a significant (10-20%) performance regression for Direct2D. Do we have any idea what might be causing this? Do we somehow perhaps call the clip function more often or something of the likes?
(In reply to comment #14)
> This causes a significant (10-20%) performance regression for Direct2D. Do we
> have any idea what might be causing this? 

What gets worse?

> Do we somehow perhaps call the clip
> function more often or something of the likes?

Perhaps. If I can reproduce the regression, I'll have a look.
(In reply to comment #15)
> (In reply to comment #14)
> > This causes a significant (10-20%) performance regression for Direct2D. Do we
> > have any idea what might be causing this? 
> 
> What gets worse?
> 
> > Do we somehow perhaps call the clip
> > function more often or something of the likes?
> 
> Perhaps. If I can reproduce the regression, I'll have a look.

Pretty much everything gets somewhat slower. It's especially noticable in debug mode when the D2D debug layer is turned on.
I have been digging into the linux tp4 regressions. I've disabled progressively more of the drawing and yet the regression persists:

baseline:
http://people.mozilla.org/~jmuizelaar/graph-server/detail.html?host=graphs-stage&old=1258771&new=1258807

disabled text, stroking and filling:
http://people.mozilla.org/~jmuizelaar/graph-server/detail.html?host=graphs-stage&old=1259691&new=1259879

disabled text, stroking, filling and painting (pretty much everything)
http://people.mozilla.org/~jmuizelaar/graph-server/detail.html?host=graphs-stage&old=1260459&new=1260531

I'm not exactly sure how to proceed.
I can not reproduce the d2d slowdown. 

Here are the times I get with the following bookmarklet:

javascript:var%20r%20=%20window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).getInterface(Components.interfaces.nsIDOMWindowUtils).redraw(500);%20alert(r%20+%20'%20ms');

old
 gmail 5657ms
 cnn 4023 ms
 mozilla 5741 ms
 tiger - 40 fps
 css-radius.heroku.com 3753 ms, 3745 ms, 3740 ms

new
 cnn 3870 ms
 mozilla 4832 ms
 gmail 5119 ms
 tiger - 42 fps
 css-radius.heroku.com 3732 ms, 3746 ms
Comment on attachment 432638 [details] [diff] [review]
getting closer

A review of adding Extend(PAD_EDGE) to the svg code wouldn't hurt.
Attachment #432638 - Flags: review?(bas.schouten) → review?(joe)
Posted patch The svg partSplinter Review
We need to use EXTEND_PAD on linux to get the reftests to pass with the cairo update. Using EXTEND_PAD_EDGE will do the right thing on all the platforms.
Attachment #437147 - Flags: review?
Attachment #437147 - Flags: review? → review?(jwatt)
Comment on attachment 432638 [details] [diff] [review]
getting closer

yay
Attachment #432638 - Flags: review?(joe) → review+
We need to try harder to trick cairo into applying the clip to the destination surface.
Attachment #437185 - Flags: review?(vladimir)
Attachment #437147 - Flags: review?(jwatt) → review+
Backed out for a windows build failure and some weird reftest failures on mac
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-opt-unittest-reftest/build/reftest/tests/layout/reftests/bugs/382916-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-opt-unittest-reftest/build/reftest/tests/layout/reftests/bugs/383488-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-opt-unittest-reftest/build/reftest/tests/layout/reftests/bugs/383883-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-opt-unittest-reftest/build/reftest/tests/layout/reftests/bugs/383883-3.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-opt-unittest-reftest/build/reftest/tests/layout/reftests/bugs/383884-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-opt-unittest-reftest/build/reftest/tests/layout/reftests/bugs/384322-1.html |
It seems as though the contents from the left frame of layout/reftests/bugs/381746-1.html are persisting into the following tests. Anyone have any idea why this would happen?
So more info:

- It looks like the problem comes from rebasing mozilla-central. Using m-c from March 12 passes the test.
- 381746-1.html, the frameset test, changed in this range
- one of the failing tests was also added.
- It's not clear whether the failure is new or if we're just exposing a problem that was also in the March 12 m-c.
Reverting the changes to 381746-1.html seems to make the problem go away.
gfxASurface::gfxSurfaceType should be updated in the patch to reflect the new Cairo surface types. It's already missing SurfaceTypeMeta and doesn't have D2D at the end (although the former is worse since it breaks DDraw). We should aim to keep it up to date probably.
Here's a paint log of the failing reftest:
http://pastebin.mozilla.org/713573
So my latest theory about the problem is that the clip when we draw the second frame in 381746-1.html is not be restored after drawWindow completes. This causes the followings tests to be clipped to the area of the second frame until the clip is restored.
The landing on m-c is showing painting / image issues.  
Visit: http://forums.mozillazine.org/viewforum.php?f=23
Note the icons on the left side of the forum are not displayed, but dragging the mouse down the page causes them to appear, and dragging it up the page they go away.  

I verified the regression range to the landing of this update: 


20100407201605 0770fe34e535  ok

20100407204139 9480726de986  bad cairo update

This is going to make the nightly builds pretty bad. :(
Depends on: 558024
Depends on: 558025
I'm also seeing a lot of painting problems (black squares in many places where new painting should happen, which don't get fully replaces by the actual content) in a current SeaMonkey trunk build on Linux.
This checkin seems to be the most likely to cause that...
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100408 Minefield/3.7a5pre

There doesn't appear to be a comment here with a changeset reference for the checkin. In fact, there are no changeset references for any of the checkins for this bug.

On Linux there are black rectangles all over the place, in content and in chrome. The scroll buttons, selected text, and also randomly. Caret doesn't seem to unpaint in textareas.  Scrolling a black rectangle out of view and back in again seems to repaint and remove it.

Please respin!
I'm not seeing these problems.  d2d/dw enabled for me.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100408 Minefield/3.7a5pre - Build ID: 20100408041901
For grins, I just enabled directwrite and Direct2D and the paint problems seems to have gone away.  Interesting!
Has anyone seen this problem on OS X?
I've backed this out.
Perhaps not so important info, but in Gmail, the check boxes by each message do not appear except on mouse pointer hover. Then, some remain while others disappear after the pointer moves off a check box.
Depends on: 558079
Depends on: 558083
When you land it again, could you make sure to list the new base version of cairo in gfx/cairo/README (it's not 1.8.2 any more)?
Depends on: 559660
Assignee: nobody → jmuizelaar
(In reply to comment #41)
> Created an attachment (id=439369) [details]
> A reftest for a problem that our current tests didn't catch

It looks like the reftest won't show the problem because of when we do drawWindow we always have a clip.
(In reply to comment #42)
> 
> It looks like the reftest won't show the problem because of when we do
> drawWindow we always have a clip.

Unlike when we do drawing to an actual window.
I'm getting some new unexplained reftest failures on win32.

REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/slave/win32-unit/mozilla/layout/reftests/font-face/synthetic-weight-style.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/slave/win32-unit/mozilla/layout/reftests/font-face/synthetic-variations.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/slave/win32-unit/mozilla/layout/reftests/margin-collapsing/block-clear-6e-left.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/slave/win32-unit/mozilla/layout/reftests/margin-collapsing/block-clear-6f-left.html |
Still not working right.  Using the latest hourly build with the update, images on some forums, CNN, MSNBC images are not appearing, but do when the page is scrolled.  

Addon Icons on the NavBar go transparent when hovered over.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre ID:20100426084628
FavIcons on bookmarks are not showing either.
A quick test reveals that things seem to be ok with DW/D2D enabled, but those 'off', the issue I stated above shows up.
(In reply to comment #48)
> A quick test reveals that things seem to be ok with DW/D2D enabled, but those
> 'off', the issue I stated above shows up.

Can you post a screenshot of this issue?
(In reply to comment #49)
> (In reply to comment #48)
> > A quick test reveals that things seem to be ok with DW/D2D enabled, but those
> > 'off', the issue I stated above shows up.
> 
> Can you post a screenshot of this issue?

Will be tomorrow, I just now arrived at work..
Depends on: 561827
When I fixed the win32-surface show_glyphs function I didn't fix dwrite parallel. This does so.
Attachment #441594 - Flags: review?(bas.schouten)
Here's a site having painting issues when scrolling up and down: http://www.history.ca/video/default.aspx

This is with all dw/d2d stuff OFF
Attachment #441594 - Flags: review?(bas.schouten) → review+
Depends on: 561856
(In reply to comment #52)
> Here's a site having painting issues when scrolling up and down:
> http://www.history.ca/video/default.aspx
> 
> This is with all dw/d2d stuff OFF

I'm hoping this is fixed by the fix to bug 561827. If not reopen.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Don't reopen! File more bugs and make them dependent.

Congrats on landing Cairo, Jeff!
Depends on: 561891
I don't see the #include <intrin.h> anywhere in these patches although it appears in cairo.c?
(In reply to comment #55)
> I don't see the #include <intrin.h> anywhere in these patches although it
> appears in cairo.c?

Indeed. I neglected to update that patch. I'll fix it when I merge upstream.
Depends on: 562616
Depends on: 562710
Depends on: 563255
blocking2.0: --- → ?
blocking2.0: ? → ---
Depends on: 566283
Depends on: 569669
Depends on: 574658
Depends on: 595671
Depends on: 672750
Depends on: 709850
Depends on: 743578
Depends on: 750224
Depends on: 567751
Depends on: 676001
Depends on: 591822
Depends on: 947817
Depends on: 1063486
Depends on: 1157112
You need to log in before you can comment on or make changes to this bug.