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

[Christophe de Dinechin]

> On 20 Feb 2018, at 15:36, Lukáš Hrázký  wrote:
> 
> On Tue, 2018-02-20 at 15:22 +0100, Christophe de Dinechin wrote:
>>> On 19 Feb 2018, at 18:29, Lukáš Hrázký  wrote:
>>> 
>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
 From: Christophe de Dinechin 
 
 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.
> 
> See my reply to 12/17 of the series, should probably be the issue.
> 
>> What guest type (e.g. f25, rhel…)? I’ve not seen that. I’ll try again and 
>> see if I can reproduce.
> 
> f27, should not be related.
> 
>>> 
 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?
> 
> Must've gotten lost... But the solution to put client_codecs and
> supposedly the streaming_requested is honestly ugly :)

Well, I really only put outfgoing messages in their own classes. Incoming 
messages remain to be done. That being said, the logic is really small, so I 
though that was OK for the moment.

> and seems like you started taking shortcuts :)

No, simply doing things in rough order of priorities.

> I was actually working on that part myself, so it turned out ok as I have 
> some rough solution, which I’ll need to incorporate with your design, if I 
> can manage that :)

Cool. So I won’t touch that part then.

> 
>>> 
>>> 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?
> 
> I don't have the patch yet, just an idea for the quit. As for
> streaming_requested, see above. I'd like to do that, on top of your
> patches, once the dust settles a bit...
> 
>>> 
>>> 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 

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

[Lukáš Hrázký]

On Tue, 2018-02-20 at 15:22 +0100, Christophe de Dinechin wrote:
> > On 19 Feb 2018, at 18:29, Lukáš Hrázký  wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin 
> > > 
> > > 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.

See my reply to 12/17 of the series, should probably be the issue.

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

f27, should not be related.

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

Must've gotten lost... But the solution to put client_codecs and
supposedly the streaming_requested is honestly ugly :) and seems like
you started taking shortcuts :) I was actually working on that part
myself, so it turned out ok as I have some rough solution, which I'll
need to incorporate with your design, if I can manage 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?

I don't have the patch yet, just an idea for the quit. As for
streaming_requested, see above. I'd like to do that, on top of your
patches, once the dust settles a bit...

> > 
> > 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  header
> > >  log_binary is really a boolean
> > >  Eliminate signed/unsigned warning
> > >  Do not create 

Re: [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ý  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> 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  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 +
>> 

Re: [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 
> 
> 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  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

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

[Christophe de Dinechin]

From: Christophe de Dinechin 

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.

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

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

-- 
2.13.5 (Apple Git-94)

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel