[Spice-devel] [PATCH v2 0/2] RHEL7 improvements

[Frediano Ziglio]

> 
> Hi,
> 
> On Thu, Dec 08, 2016 at 03:43:22PM +0000, Frediano Ziglio wrote:
> > These 2 patches attempt to join images split by RHEL7 graphic
> > stack (Mesa) decreasing commands handled by spice-server.
> >
> > You can see the difference between the 2 video:
> > - https://www.youtube.com/watch?v=OarV6zUmUdg (before)
> > - https://www.youtube.com/watch?v=5fTdCCbFeCg (after)
> > These video are realized with some additional changes:
> > - the statistics from the terminal have some additional
> >   "out_messages" counters. These counters show the messages
> >   sent to the client(s). These changes are part of my "stat"
> >   branch (partially sent couple of days ago);
> > - the replay utility, as you can see, was changed to replay
> >   using the real time to allow the video code (which is dependent
> >   on timing) to work correctly. The patch is currently not in
> >   a good shape (enough to be sent);
> > - the client utility was changed to remove the delay due to
> >   the lip sync. The glitches (present mostly before these patches)
> >   are much reduced.
> > 
> > Note the number of commands sent to the client reduced from 6097
> > to 2016 (current year, just a coincidence).
> > The network traffic reduced from 72M to 56M. This is due to the fact
> > that having a single stream (as you can see VP8 codec was used) the
> > compression on the stream is better.
> > 
> > These patches fix:
> > - https://bugzilla.redhat.com/show_bug.cgi?id=1158029;
> > - (probably) https://bugzilla.redhat.com/show_bug.cgi?id=1294564.
> >
> > Changes since RFC:
> > - moved code to DisplayChannel;
> > - define a constant for timeout.
> 
> As I did some test with streaming today/yesterday I came back to this
> and tested in top of [0] - It is super easy to see how much improvement
> this is with VP8. Tested on F25 and windows 7 guests.
> 
> [0]
> https://lists.freedesktop.org/archives/spice-devel/2016-December/034526.html
> 
> I don't feel knowledgeable enough for a review on this but feel free to
> add:
> 
> Tested-by: Victor Toso <victortoso at redhat.com>
> 
> Cheers,
>

Thanks. I actually think I'd nack them. First version has all code in
red-worker.c which is quite bad considering that are mainly functions
related to display. Second version move all to display however they
introduce a regression from version 1.
While the first version always check for new display commands before
getting a timeout and executing the joined command the second register
a timeout but it's possible (also considering the polling in the worker
loop) that the timeout procedure is triggered before display commands
are checked causing some commands to not be joined together.
Although this can be limited reducing the polling timeout (in red-worker)
or increasing the timeout (now in display-channel) I prefer code that
is more deterministic (like, unfortunately version 1 was).

Not that I'm saying that these patches do not solve the main issue of
rhbz#1158029 or improve stream quality, just that I don't like version
1 for the style and I don't like version 2 for the regression and not
determinism in some situations. Mainly a solution would be to implement
2/2 in a different way but nothing easy came into my mind :-(
Suggestions welcome.

Frediano