[Spice-devel] [PATCH] RFC: Document display channel drawable trees, etc

[Frediano Ziglio]

> 
> On Thu, 2017-01-26 at 04:18 -0500, Frediano Ziglio wrote:
> > > 
> > > ---
> > > I started looking into a bug related to surfaces and drawing, but
> > > after a
> > > little initial investigation, I realized that I didn't really
> > > understand any
> > > of
> > > the code in this part of spice server. I discussed it a little bit
> > > with
> > > several
> > > others who also didn't have a good understanding of this part of
> > > the code. So
> > > I
> > > decided it needed to be documented so that we can understand it and
> > > work on
> > > it
> > > effectively. I've spent several long days trying to understand the
> > > details
> > > and
> > > trying to document them as well as I can. There is a lot of
> > > confusing
> > > terminology, and there are things I still don't fully understand,
> > > but I
> > > wanted
> > > to send this out to get feedback from others. Is it helpful? Do you
> > > see any
> > > cases where I misinterpreted things? Can anybody solve any pieces
> > > of the
> > > puzzle
> > > that I didn't quite figure out? etc. etc.
> > > 
> > > I know it's rather dense and will probably require others to spend
> > > some
> > > serious
> > > time understanding the code, but I'd love to get some feedback on
> > > it.
> > > 
> > 
> > Take some time to find this post, so before get really buried on the
> > ML...
> 
> Thanks for taking the time to review. Would love to hear from others as
> well.
> 
> > 
> > Some part are quite good and should be split and merged, other,
> > particularly exclude_region have too much questions to solve.
> 
> Yeah, not a bad idea. Some discussion below.
> 
> > 
> > >  server/display-channel-private.h |   5 +-
> > >  server/display-channel.c         | 256
> > >  +++++++++++++++++++++++++++++++++++++++
> > >  server/display-channel.h         |  10 +-
> > >  server/tree.c                    |  12 +-
> > >  server/tree.h                    |   6 +
> > >  5 files changed, 285 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/server/display-channel-private.h
> > > b/server/display-channel-private.h
> > > index 6e299b9..dc4d605 100644
> > > --- a/server/display-channel-private.h
> > > +++ b/server/display-channel-private.h
> > > @@ -32,7 +32,10 @@ struct DisplayChannelPrivate
> > >      int enable_jpeg;
> > >      int enable_zlib_glz_wrap;
> > >  
> > > -    Ring current_list; /* Drawable */
> > > +    /* A ring of pending drawables for this DisplayChannel,
> > > regardless of which
> > > +     * surface they're associated with. This list is mainly used
> > > to flush older
> > > +     * drawables when we need to make room for new drawables. */
> > > +    Ring current_list;
> > 
> > is this list containing ALL drawables allocated ?
> > Could be renamed to drawable_list ?
> 
> Not exactly, the list of all allocated drawables is
> DisplayChannelPrivate::drawables. This is a list of all drawables that

Not exactly DisplayChannelPrivate::drawables contains _Drawables, not
Drawables and it's a memory pool where Drawables are allocated.

> are pending for any surface associated with this DisplayChannel. In
> essence, it's roughly equivalent to the union of all
> DisplayChannelPrivate::surfaces[i].current_list rings. But the
> drawables are ordered by age so that if we want to find the oldest
> drawable in the queue (regardless of which surface it's associated
> with), we can just get the tail of this ring (see free_one_drawable()).
> So as far as I can tell, it's just a convenience ring to avoid
> iterating through all of the surfaces to find the oldest drawable.
> 

Maybe (just added a sentence):

  /* A ring of pending drawables for this DisplayChannel, regardless of which
   * surface they're associated with. This list is mainly used to flush older
   * drawables when we need to make room for new drawables.
   * The list is maintained in order of age being tail the oldest drawable.
   */


> > 
> > Wondering what these "current" are...
> 
> As far as I understand, it's basically all of the QXL commands that
> have been received from the device, but have not actually been drawn
> yet. It's not a great name though...
> 

So kind of "pending" that is current as not obsolete in some way.

....

> 
> > I though that the drawing was done using pipe items.
> 
> Well, the drawables are sent to the client via pipe items so that the
> client can draw them. But when I say "drawing" above, I'm talking about
> e.g. drawable_draw(). This function appears to draw the given drawable
> to the surface's canvas (on the server side).  Again, my understanding
> here is incomplete. Presumably we need to keep a cache of the surface
> on the server side in case we need to send the full surface image to a
> client, or something like that? Probably that stuff should be
> documented as well.
> 

Yes, they are keep for new clients or disconnection/connection or
just as no clients are connected. Screen is not updated all the time
but only when needed.

...

> 
> > 
> > >      QRegion rgn;
> > >  };
> > >  
> > >  /* A region "below" a copy, or the src region of the copy */
> > >  struct Shadow {
> > >      TreeItem base;
> > > +    /* holds the union of all parts of this source region that
> > > have been
> > > +     * obscured by a drawable item that has been subsequently
> > > added to the tree
> > > +     */
> > >      QRegion on_hold;
> > 
> > basically here, opposite to rgn we hold the not visible region...
> > I would ask why.
> 
> 
> I have a semi-understanding (or at least a partial working theory)
> about this field that I documented elsewhere as part of
> __exclude_region(). I'll reproduce it here:
> 
> 
>     -----------
>     If the item is a Shadow, we store the intersection between @rgn and
>     the Shadow's region in Shadow::on_hold and remove that region from
>     @rgn. This is done since a Shadow represents the source region for a
>     COPY_BITS operation, and we need to make sure that this source
>     region stays up-to-date until the copy operation is executed.
> 
>     Consider the following case:
>      1) the surface is fully black at the beginning
>      2) we add a new item to the tree which paints region A white
>      3) we add a new item to the tree which copies region A to region B
>      4) we add another new item to teh tree painting region A blue.
> 
>     After all operations are completed, region A should be blue, and
>     region B should be white. If there were no copy operation (step 3),
>     we could simply eliminate step 2 when we add item 4 to the tree,
>     since step 4 overwrites the same region with a different color.
>     However, if we did eliminate step 2, region B would be black after
>     all operations were completed. So these regions that would normally
>     be excluded are put "on hold" if they are part of a source region
>     for a copy operation.
>     ------------
> 
> This was just my attempt to understand this field from reading the code
> (for a long time). My understanding is still incomplete, and I'd love
> to hear whether it makes sense to others.
> 

Have you tried doing some testing and not only looking at the
code?
Maybe it's easier to understand. How hard to implement some
sort of sensible dumping (graphical would be better) I don't
know...

> 
> Thanks,
> Jonathon
> 

Do you have a repository for this?
I don't have idea how to manage all these comments/documentation.

Frediano