On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin <dinec...@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(-) > _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
