[Spice-devel] [PATCH 00/17] WIP: Refactor the streaming agent towards a more standard C++ style

[Christophe de Dinechin]

> On 19 Feb 2018, at 18:29, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin at redhat.com>
>> 
>> The streaming agent started as C code. This series converts
>> the style to something that is more usual for C++, notably:
>> 
>> - Adds encapsulation and RAII for resources such as the stream
>> - Splits functionality into several classes with clear roles
>> - Puts message formatting and writing in a reused based class
>> - Isolate what's specific to each message in three derived classes
>> - Isolate X11-specific code in separate classes, one for cursor messages,
>>  one for the thread polling the data.
>> 
>> Reasons for marking this WIP:
>> 
>> 1. This series is, unfortunately, not correctly tested, because on my
>>   machine, I currently have a black screen with the 'master' streaming
>>   agent, server, protocol, common, plugin and spicy. Fallback to MJPEG
>>   leads to a large repetition of messages like:
>> 
>>   (spicy:4217): GSpice-CRITICAL **: need more input data
>> 
>>   What I have tested using the -d option is that the syslog output
>>   from the agent looks similar (size of data captured, etc) relative
>>   to master both for the MJPEG plugin and with fallback. But without
>>   a picture, I am still concerned about the lack of testing.
> 
> Christophe already knows, but in case anyone will benefit from this,
> it's the incomplete frame bug, fix should be on the ML here:
> 
> [Spice-devel] [PATCH spice-server 8/8] stream-channel: Send the full
> frame in a single message
> 
> Christophe, I've tested your patches, it's streaming, but the cursor
> disappears after a while, so something's not entirely right.

So there is probably an issue with the cursor message… I will look into it.

What guest type (e.g. f25, rhel…)? I’ve not seen that. I’ll try again and see if I can reproduce.

> 
>> 2. The classes were isolated, but not moved in separate headers. This
>>   is intentional. I prefer to make sure that the changes on the code
>>   are agreed on before we move the individual classes to their own
>>   headers. It thinks it will also faclitate history browsing
> 
> I'd like to point out the classes are not entirely isolated, there is
> still the static bool streaming_requested (besides the necessary
> quit_requested, although I have an idea for that), which ties together
> the ConcreteAgent::CaptureLoop and Stream::read_command_from_device.

Interesting. That will force me to re-review the whole thing, because I definitely remember moving streaming_requested within the Stream class, but I don’t see that anymore. Unless I ran into some issue and decided to postpone that?

> 
> I have an unfinished patch for this, which will need heavy rebasing. In
> case you are/will be looking into it, Christophe, let me know.

My reading is that you have a patch for the coupled case (quit). Is it OK if I add one for streaming_requested to the next spin, or do you plan to do that?

> 
> Lukas
> 
>> 3. This series compiles without warnings both with GCC in C++11 mode
>>   and by clang in gnu++11 mode. However, Frediano pointed out that it
>>   uses a designated intiializer syntax that is presently a GNU
>>   extension (the C99 designated initializer syntax). We may consider
>>   it a problem or not. If it's a problem, it's sufficient to remove
>>   the designators, but I think they add to readability.
>> 
>> 4. Our current configure.ac requires a warning if all initializers are
>>   not present. Since padding was made explicit in the protocol, this
>>   requires the code to initialize padding fields, which I don't like.
>> 
>> 5. The series integrates off-topic patches sent in a separate series, but
>>   which are necessary to successfully build with both clang and gcc.
>> 
>> This series can also be browsed at
>> https://gitlab.com/c3d/spice-streaming-agent/merge_requests/1/commits
>> 
>> Christophe de Dinechin (17):
>>  Add missing <string> header
>>  log_binary is really a boolean
>>  Eliminate signed/unsigned warning
>>  Do not create std::string for constants
>>  Use RAII to cleanup stream in case of exception or return
>>  Replace inefficient C-style initialization with C++-style
>>  Get rid of C-style memset initializations, use C++ style aggregates
>>  Use C++ style for cursor message initialization instead of C style
>>  Reorder headers according to style guide
>>  Since we use a namespace, simplify name of local classes
>>  Move read, write and locking into the 'Stream' class
>>  Convert message writing from C style to C++ style
>>  Add more meaningful syslog reporting
>>  Create a class encapsulating the X11 display cursor capture
>>  Create FrameLog class to abstract logging of frames
>>  Remove client_codecs global variable, moved inside the 'Stream' class
>>  Move the capture loop in the ConcreteAgent, get rid of global agent
>>    variable
>> 
>> src/concrete-agent.cpp        |   1 +
>> src/concrete-agent.hpp        |   4 +
>> src/mjpeg-fallback.cpp        |   2 +-
>> src/spice-streaming-agent.cpp | 521 +++++++++++++++++++++++++-----------------
>> 4 files changed, 311 insertions(+), 217 deletions(-)