> On 19 Feb 2018, at 18:29, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: > > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote: >> From: Christophe de Dinechin <dinechin@xxxxxxxxxx> >> >> 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: > > [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(-) _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel