Re: gsk-render review

[Emmanuele Bassi <ebassi gmail com>]

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
end.

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:

https://git.gnome.org/browse/gtk+/commit/?h=wip/ebassi/gsk-renderer&id=06ba2eb03c68d424c445d12243a89ce1f97f2198

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.

Ciao,
 Emmanuele.

-- 
https://www.bassi.io
[@] ebassi [@gmail.com]