Re: gsk-render review

Hi Alex;

thanks so much for this review.

On 8 July 2016 at 10:15, Alexander Larsson <alexl redhat com> wrote:
So, I had a quick look at the gsk-render branch. I don't really have
much time for an in depth review, but here are some issues i saw:

The first one is a detail in the gl renderer. As opposed to a regular
3d engine we will mostly be drawing things with z=0, with a lot of
overdraw. So, we have to be very careful with z-overlap. In practice
we're probably using something like the CSS z-index where in addition
to the z value we have an order for rendernodes that share z. This
order is implicit when we draw the tree in a simple fashion, but
becomes more important when we start to do more advanced reordering
during rendering (and we may also want to support explicit z-indexes
in css).

Right now the default rendering of opaque nodes is front to back. With
the default depth test (LESS) this ends up doing the right
thing. However, for the alpha nodes we render back-to-front, and I
think the LESS depth test is wrong, is it not? I mean if you draw the
back-most thing at Z=0, and then render another item at Z=0 it will
not be drawn at all. Should it not be LEQUAL in this case?

Indeed, it should. I actually disabled depth testing at some point
because I planned to add Z-indexing in the future, likely weighted by
additional Z transformations.

And when we start doing more interesting things such as reordering to
minimize state changes then we need to explictily keep track of the
node z-index and apply it as a depth offset. I think glPolygonOffset
can be used for the z-index here, as its units is multiplied with "the
smallest value that is guaranteed to produce a resolvable offset for a
given implementation". How does servo handle this?

Servo doesn't really handle this, as it assumes that all the elements
have a Z index value already set in the transformation; it defaults to
LEQUAL, whenever it enables depth testing for compositing batches,
whereas rasterization batches (e.g. when building a border shadow)
disable depth testing altogether and assume all elements of the batch
rest on the same Z=0 plane (which is a sensible thing). Various parts
of the scene are grouped together as "layers", for instance for
scrolling content and for fixed content; each layer has its own
rendering target and all targets are flattened and composited at the

In general, the rules for depth testing inside HTML and CSS are
probably safe to use in our case: use the Z-index to re-order the
batching, and if something decides to use a depth tranformation then
it "breaks out" of the layer.

The second issue I found was in the Gtk+ integration. In particular,
the handling of queue_draw/resize. Traditionally, whenever you called
queue_draw the affected area would be invalidated on the toplevel
window and *everything* in that area would be redrawn from
scratch. Currently this happens in the gsk-render case too, i.e. we
re-render to a surface for each node. However, in an ideal world we
would often not want to do this. For instance, if a button becomes
focused we don't want to re-draw anything but that surface of the
button, and then just re-compose all the other previously drawn
surfaces. And in the case of a simple move of a widget we don't want
to redraw anything at all, just recompose.

Yep, that's how I started writing GskRenderNode and GskRenderer, until
you convinced me to throw away the tree at every frame. ;-)

I think we need to extend the api with which a widget can tell gtk how
to redraw it. The sub-tree issues above is very similar to the ones we
had when the pixel-cache was introduced. To handle that i added
gdk_window_set_invalidate_handler() which can handle the catching and
propagation of invalidation of child invalidations. We need something
similar but on the render tree level. We also I think need to separate
the calls to queue a compose of your level in the tree, and the
marking of your cairo surface to be re-rendered.

Yes, I had that exact same API:

which invalidated render nodes and propagated the invalidation through
the render tree.

Personally, I think the new behaviour of throwing away the render tree
after each frame makes the code much more resilient, especially in the
perspective of a future "off the main thread" rendering scenario —
though I agree that caching of the render tree in the widget tree
would probably be a good thing to have.

Right now, once you call GskRenderer.render(), the renderer will mark
the render tree elements as immutable. This would allow widget
implementations to keep a pointer around to the nodes they create, and
if they do not wish to change them, they can reuse them as they are —
otherwise they could throw them away and create new instances; at the
end of each frame, the render nodes would be detached from the tree,
so you could add it to a new render tree when asked; we could also add
API to GskRenderNode to "group" a tree sub-section so that only the
group's top-level node would be detached from its parent.


[@] ebassi []

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]