Summary: | [GTK] Crash in cairo_surface_mark_dirty_rectangle() in accelerated compositing mode under X11 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | john.frankish | ||||||||
Component: | WebKitGTK | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aperez, bugs-noreply, cgarcia, mcatanzaro | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Local Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=214177 | ||||||||||
Attachments: |
|
Description
john.frankish
2020-04-16 22:13:12 PDT
Created attachment 396763 [details]
cairo surface patch
I recompiled webkitgtk with the attached patch and it seems to have fixed things.
From Uli in https://gitlab.freedesktop.org/cairo/cairo/-/issues/400: """ That code is clearly wrong. Well, at least from the comment, I guess that there is no call to cairo_surface_flush() anywhere. The docs for cairo_surface_mark_dirty() state that one must call cairo_surface_flush() first. I guess the easiest fix would be to add a call to cairo_surface_flush(m_surface.get()); at the end of that function. """ Created attachment 396768 [details]
Patch
Comment on attachment 396768 [details]
Patch
I wonder if this might unbreak some of the layout tests which
have been crashing 🤾♂️️
Comment on attachment 396768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396768&action=review > Source/WebKit/ChangeLog:1 > +2020-04-17 Michael Catanzaro <mcatanzaro@gnome.org> You should credit John or even Uli, I think. > Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:373 > + cairo_surface_flush(m_surface.get()); > + This is not needed in Wayland. In this case we are writing the pixel data directly, so there aren't snapshots, and the surface is never modified after the paint, like in X11. The documentation of cairo_surface_mark_dirty() says: "Tells cairo that drawing has been done to surface using means other than cairo, and that cairo should reread any cached areas. Note that you must call cairo_surface_flush() before doing such drawing." (In reply to Carlos Garcia Campos from comment #5) > This is not needed in Wayland. In this case we are writing the pixel data > directly, so there aren't snapshots, and the surface is never modified after > the paint, like in X11. Are we "drawing to the surface using means other than cairo"? If so: it needs both mark_dirty() and flush(). If not: it should not use mark_dirty() or flush(). Right? (In reply to Michael Catanzaro from comment #6) > The documentation of cairo_surface_mark_dirty() says: "Tells cairo that > drawing has been done to surface using means other than cairo, and that > cairo should reread any cached areas. Note that you must call > cairo_surface_flush() before doing such drawing." Right. > (In reply to Carlos Garcia Campos from comment #5) > > This is not needed in Wayland. In this case we are writing the pixel data > > directly, so there aren't snapshots, and the surface is never modified after > > the paint, like in X11. > > Are we "drawing to the surface using means other than cairo"? > > If so: it needs both mark_dirty() and flush(). > > If not: it should not use mark_dirty() or flush(). Right? I'll try to explain how this works in both X11 and Wayland. - X11: We create an xlib surface for a pixmap that is rendered by the web process using a redirected xcomposite window. Using xdamage we detect updates in the pixmap and use the surface as a source to draw in the view cairo context. We use mark dirty as the comment says, because between the xdamage notification and the draw signal the web process might have modified the pixmap. When cairo is configured to use xcb instead of xlib, it might use an image surface attached to the xlib one as snapshot. In that case flush() is needed to detach that snapshot after we have drawn the surface in the context. - Wayland: We only create a cairo surface when gdk_cairo_draw_from_gl() can't be used. In that case we create an image surface, read all the pixels using glReadPixels and write them directly in the image surface data. We need to use mark_dirty here because we have modified the pixels data, and cairo doesn't know it. And then we use the surface as a source to draw in the view cairo context. After that the surface is not used at all, it's reused only as an optimization to avoid allocating a new one on every draw, but we are always writing all the pixels data and there's no other operation in the surface. The comment before mark_dirty was copied and it's indeed wrong, but we don't need to flush for sure. I hope it's clear now. OK... maybe makes more sense for you to prepare the patch, since you understand? :) Created attachment 397340 [details]
Patch
Committed r260570: <https://trac.webkit.org/changeset/260570> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397340 [details]. |