[16/17] Remove client_codecs global variable, moved inside the 'Stream' class

Submitted by Christophe de Dinechin on Feb. 16, 2018, 4:15 p.m.

Details

Message ID 20180216161547.28110-17-christophe@dinechin.org
State New
Headers show
Series "WIP: Refactor the streaming agent towards a more standard C++ style" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 16, 2018, 4:15 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

This is not a really nice abstraction at this point, but still a step in the right way

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 src/spice-streaming-agent.cpp | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index b0c09d8..19f3c07 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -61,8 +61,11 @@  static uint64_t get_time(void)
 
 class Stream
 {
+    typedef std::set<SpiceVideoCodecType> codecs_t;
+
 public:
     Stream(const char *name)
+        : codecs()
     {
         streamfd = open(name, O_RDWR);
         if (streamfd < 0)
@@ -73,11 +76,12 @@  public:
         close(streamfd);
     }
 
+    const codecs_t &client_codecs() { return codecs; }
+
     int have_something_to_read(int timeout);
     int read_command_from_device(void);
     int read_command(bool blocking);
 
-
     template <typename Message, typename ...PayloadArgs>
     bool send(PayloadArgs... payload)
     {
@@ -96,6 +100,7 @@  public:
 private:
     int streamfd = -1;
     std::mutex mutex;
+    codecs_t codecs;
 };
 
 
@@ -312,7 +317,6 @@  void X11CursorThread::cursor_changes()
 
 static bool streaming_requested = false;
 static bool quit_requested = false;
-static std::set<SpiceVideoCodecType> client_codecs;
 
 int Stream::have_something_to_read(int timeout)
 {
@@ -369,9 +373,9 @@  int Stream::read_command_from_device()
     streaming_requested = msg[0] != 0; /* num_codecs */
     syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
            streaming_requested ? "START" : "STOP");
-    client_codecs.clear();
+    codecs.clear();
     for (int i = 1; i <= msg[0]; ++i)
-        client_codecs.insert((SpiceVideoCodecType) msg[i]);
+        codecs.insert((SpiceVideoCodecType) msg[i]);
     return 1;
 }
 
@@ -461,7 +465,7 @@  do_capture(Stream &stream, const char *streamport, FrameLog &frame_log)
         syslog(LOG_INFO, "streaming starts now\n");
         uint64_t time_last = 0;
 
-        std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture(client_codecs));
+        std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture(stream.client_codecs()));
         if (!capture)
             throw std::runtime_error("cannot find a suitable capture system");
 

Comments

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> This is not a really nice abstraction at this point, but still a step in the right way
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index b0c09d8..19f3c07 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -61,8 +61,11 @@ static uint64_t get_time(void)
>  
>  class Stream
>  {
> +    typedef std::set<SpiceVideoCodecType> codecs_t;
> +
>  public:
>      Stream(const char *name)
> +        : codecs()
>      {
>          streamfd = open(name, O_RDWR);
>          if (streamfd < 0)
> @@ -73,11 +76,12 @@ public:
>          close(streamfd);
>      }
>  
> +    const codecs_t &client_codecs() { return codecs; }
> +
>      int have_something_to_read(int timeout);
>      int read_command_from_device(void);
>      int read_command(bool blocking);
>  
> -
>      template <typename Message, typename ...PayloadArgs>
>      bool send(PayloadArgs... payload)
>      {
> @@ -96,6 +100,7 @@ public:
>  private:
>      int streamfd = -1;
>      std::mutex mutex;
> +    codecs_t codecs;
>  };
>  
>  
> @@ -312,7 +317,6 @@ void X11CursorThread::cursor_changes()
>  
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
> -static std::set<SpiceVideoCodecType> client_codecs;
>  
>  int Stream::have_something_to_read(int timeout)
>  {
> @@ -369,9 +373,9 @@ int Stream::read_command_from_device()
>      streaming_requested = msg[0] != 0; /* num_codecs */
>      syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
>             streaming_requested ? "START" : "STOP");
> -    client_codecs.clear();
> +    codecs.clear();
>      for (int i = 1; i <= msg[0]; ++i)
> -        client_codecs.insert((SpiceVideoCodecType) msg[i]);
> +        codecs.insert((SpiceVideoCodecType) msg[i]);
>      return 1;
>  }
>  
> @@ -461,7 +465,7 @@ do_capture(Stream &stream, const char *streamport, FrameLog &frame_log)
>          syslog(LOG_INFO, "streaming starts now\n");
>          uint64_t time_last = 0;
>  
> -        std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture(client_codecs));
> +        std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture(stream.client_codecs()));
>          if (!capture)
>              throw std::runtime_error("cannot find a suitable capture system");

How about we leave this patch out and do it properly straight away? I
can do it...

Lukas
> On 20 Feb 2018, at 16:51, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> This is not a really nice abstraction at this point, but still a step in the right way
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index b0c09d8..19f3c07 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -61,8 +61,11 @@ static uint64_t get_time(void)
>> 
>> class Stream
>> {
>> +    typedef std::set<SpiceVideoCodecType> codecs_t;
>> +
>> public:
>>     Stream(const char *name)
>> +        : codecs()
>>     {
>>         streamfd = open(name, O_RDWR);
>>         if (streamfd < 0)
>> @@ -73,11 +76,12 @@ public:
>>         close(streamfd);
>>     }
>> 
>> +    const codecs_t &client_codecs() { return codecs; }
>> +
>>     int have_something_to_read(int timeout);
>>     int read_command_from_device(void);
>>     int read_command(bool blocking);
>> 
>> -
>>     template <typename Message, typename ...PayloadArgs>
>>     bool send(PayloadArgs... payload)
>>     {
>> @@ -96,6 +100,7 @@ public:
>> private:
>>     int streamfd = -1;
>>     std::mutex mutex;
>> +    codecs_t codecs;
>> };
>> 
>> 
>> @@ -312,7 +317,6 @@ void X11CursorThread::cursor_changes()
>> 
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> -static std::set<SpiceVideoCodecType> client_codecs;
>> 
>> int Stream::have_something_to_read(int timeout)
>> {
>> @@ -369,9 +373,9 @@ int Stream::read_command_from_device()
>>     streaming_requested = msg[0] != 0; /* num_codecs */
>>     syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
>>            streaming_requested ? "START" : "STOP");
>> -    client_codecs.clear();
>> +    codecs.clear();
>>     for (int i = 1; i <= msg[0]; ++i)
>> -        client_codecs.insert((SpiceVideoCodecType) msg[i]);
>> +        codecs.insert((SpiceVideoCodecType) msg[i]);
>>     return 1;
>> }
>> 
>> @@ -461,7 +465,7 @@ do_capture(Stream &stream, const char *streamport, FrameLog &frame_log)
>>         syslog(LOG_INFO, "streaming starts now\n");
>>         uint64_t time_last = 0;
>> 
>> -        std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture(client_codecs));
>> +        std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture(stream.client_codecs()));
>>         if (!capture)
>>             throw std::runtime_error("cannot find a suitable capture system");
> 
> How about we leave this patch out and do it properly straight away? I can do it…

I don’t mind, but maybe it’s still enough progress that we keep it, and then we have all your refactoring (including input messages) in one place?

(Here, I’m concerned about tracing the evolution of the code. The step I did seems obvious from the existing code. Maybe yours is too? Not having looked at it yet, I don’t know)

I don’t mind either way.

> 
> Lukas
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, 2018-02-20 at 17:20 +0100, Christophe de Dinechin wrote:
> > On 20 Feb 2018, at 16:51, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > 
> > > This is not a really nice abstraction at this point, but still a step in the right way
> > > 
> > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > ---
> > > src/spice-streaming-agent.cpp | 14 +++++++++-----
> > > 1 file changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index b0c09d8..19f3c07 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -61,8 +61,11 @@ static uint64_t get_time(void)
> > > 
> > > class Stream
> > > {
> > > +    typedef std::set<SpiceVideoCodecType> codecs_t;
> > > +
> > > public:
> > >     Stream(const char *name)
> > > +        : codecs()
> > >     {
> > >         streamfd = open(name, O_RDWR);
> > >         if (streamfd < 0)
> > > @@ -73,11 +76,12 @@ public:
> > >         close(streamfd);
> > >     }
> > > 
> > > +    const codecs_t &client_codecs() { return codecs; }
> > > +
> > >     int have_something_to_read(int timeout);
> > >     int read_command_from_device(void);
> > >     int read_command(bool blocking);
> > > 
> > > -
> > >     template <typename Message, typename ...PayloadArgs>
> > >     bool send(PayloadArgs... payload)
> > >     {
> > > @@ -96,6 +100,7 @@ public:
> > > private:
> > >     int streamfd = -1;
> > >     std::mutex mutex;
> > > +    codecs_t codecs;
> > > };
> > > 
> > > 
> > > @@ -312,7 +317,6 @@ void X11CursorThread::cursor_changes()
> > > 
> > > static bool streaming_requested = false;
> > > static bool quit_requested = false;
> > > -static std::set<SpiceVideoCodecType> client_codecs;
> > > 
> > > int Stream::have_something_to_read(int timeout)
> > > {
> > > @@ -369,9 +373,9 @@ int Stream::read_command_from_device()
> > >     streaming_requested = msg[0] != 0; /* num_codecs */
> > >     syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
> > >            streaming_requested ? "START" : "STOP");
> > > -    client_codecs.clear();
> > > +    codecs.clear();
> > >     for (int i = 1; i <= msg[0]; ++i)
> > > -        client_codecs.insert((SpiceVideoCodecType) msg[i]);
> > > +        codecs.insert((SpiceVideoCodecType) msg[i]);
> > >     return 1;
> > > }
> > > 
> > > @@ -461,7 +465,7 @@ do_capture(Stream &stream, const char *streamport, FrameLog &frame_log)
> > >         syslog(LOG_INFO, "streaming starts now\n");
> > >         uint64_t time_last = 0;
> > > 
> > > -        std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture(client_codecs));
> > > +        std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture(stream.client_codecs()));
> > >         if (!capture)
> > >             throw std::runtime_error("cannot find a suitable capture system");
> > 
> > How about we leave this patch out and do it properly straight away? I can do it…
> 
> I don’t mind, but maybe it’s still enough progress that we keep it, and then we have all your refactoring (including input messages) in one place?
> 
> (Here, I’m concerned about tracing the evolution of the code. The step I did seems obvious from the existing code. Maybe yours is too? Not having looked at it yet, I don’t know)

To me it doesn't seem as a step in the right direction. The Stream
class should not contain any state related to the agent.

> I don’t mind either way.

I don't mind it as a temporary measure either. But I felt I got called
out when I introduced temporary measures in my patch, hence I mention
it :)

At this point, I'm willing to go for whatever is faster for us,
actually. The sooner we split the files, the better.

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