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

[Lukáš Hrázký]

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.

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

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.

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(-)
>