[10/22] Remove client_codecs global variable, moved inside the 'Stream' class

Submitted by Christophe de Dinechin on Feb. 28, 2018, 3:43 p.m.

Details

Message ID 20180228154325.25791-11-christophe@dinechin.org
State New
Headers show
Series "streaming-agent: C++ refactoring" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 28, 2018, 3:43 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

It makes sense to place the codecs inside the Stream which is charged
with setting it up, until we create a proper abstraction for input
messages similar to what Message does for output messages in follow-up
patches.

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

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 4d24234..403e65c 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -67,8 +67,11 @@  struct CursorMessage
 
 class Stream
 {
+    typedef std::set<SpiceVideoCodecType> codecs_t;
+
 public:
     Stream(const char *name)
+        : codecs()
     {
         streamfd = open(name, O_RDWR);
         if (streamfd < 0) {
@@ -80,6 +83,8 @@  public:
         close(streamfd);
     }
 
+    const codecs_t &client_codecs() { return codecs; }
+
     int read_command(bool blocking);
     size_t write_all(const void *buf, const size_t len);
     int send_format(unsigned w, unsigned h, uint8_t c);
@@ -98,6 +103,7 @@  private:
 private:
     int streamfd = -1;
     std::mutex mutex;
+    codecs_t codecs;
 };
 
 }} // namespace spice::streaming_agent
@@ -105,7 +111,6 @@  private:
 static bool streaming_requested = false;
 static bool quit_requested = false;
 static bool log_binary = false;
-static std::set<SpiceVideoCodecType> client_codecs;
 
 int Stream::have_something_to_read(int timeout)
 {
@@ -139,9 +144,9 @@  void Stream::handle_stream_start_stop(uint32_t len)
     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]);
     }
 }
 
@@ -432,7 +437,7 @@  do_capture(Stream &stream, const char *streamport, FILE *f_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 Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> It makes sense to place the codecs inside the Stream which is charged
> with setting it up, until we create a proper abstraction for input
> messages similar to what Message does for output messages in follow-up
> patches.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4d24234..403e65c 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -67,8 +67,11 @@ struct CursorMessage
>  
>  class Stream
>  {
> +    typedef std::set<SpiceVideoCodecType> codecs_t;

Not sure I'd present a typedef for it, but that's a matter of style.

As for the naming, normally we use CamelCase with no "_t" suffix for
type names, do you wish to distinguish these member typedefs like this?

> +
>  public:
>      Stream(const char *name)
> +        : codecs()
>      {
>          streamfd = open(name, O_RDWR);
>          if (streamfd < 0) {
> @@ -80,6 +83,8 @@ public:
>          close(streamfd);
>      }
>  
> +    const codecs_t &client_codecs() { return codecs; }
> +
>      int read_command(bool blocking);
>      size_t write_all(const void *buf, const size_t len);
>      int send_format(unsigned w, unsigned h, uint8_t c);
> @@ -98,6 +103,7 @@ private:
>  private:
>      int streamfd = -1;
>      std::mutex mutex;
> +    codecs_t codecs;
>  };
>  
>  }} // namespace spice::streaming_agent
> @@ -105,7 +111,6 @@ private:
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
>  static bool log_binary = false;
> -static std::set<SpiceVideoCodecType> client_codecs;
>  
>  int Stream::have_something_to_read(int timeout)
>  {
> @@ -139,9 +144,9 @@ void Stream::handle_stream_start_stop(uint32_t len)
>      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]);
>      }
>  }
>  
> @@ -432,7 +437,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_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");
>          }
> 
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> It makes sense to place the codecs inside the Stream which is charged
> with setting it up, until we create a proper abstraction for input
> messages similar to what Message does for output messages in follow-up
> patches.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4d24234..403e65c 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -67,8 +67,11 @@ struct CursorMessage
>  
>  class Stream
>  {
> +    typedef std::set<SpiceVideoCodecType> codecs_t;
> +
>  public:
>      Stream(const char *name)
> +        : codecs()

this is not necessary

>      {
>          streamfd = open(name, O_RDWR);
>          if (streamfd < 0) {
> @@ -80,6 +83,8 @@ public:
>          close(streamfd);
>      }
>  
> +    const codecs_t &client_codecs() { return codecs; }
> +

I would add const here:

   const codecs_t &client_codecs() const { return codecs; }

>      int read_command(bool blocking);
>      size_t write_all(const void *buf, const size_t len);
>      int send_format(unsigned w, unsigned h, uint8_t c);
> @@ -98,6 +103,7 @@ private:
>  private:
>      int streamfd = -1;
>      std::mutex mutex;
> +    codecs_t codecs;

why not client_codecs?

>  };
>  
>  }} // namespace spice::streaming_agent
> @@ -105,7 +111,6 @@ private:
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
>  static bool log_binary = false;
> -static std::set<SpiceVideoCodecType> client_codecs;
>  
>  int Stream::have_something_to_read(int timeout)
>  {
> @@ -139,9 +144,9 @@ void Stream::handle_stream_start_stop(uint32_t len)
>      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]);
>      }
>  }
>  
> @@ -432,7 +437,7 @@ do_capture(Stream &stream, const char *streamport, FILE
> *f_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");
>          }

Frediano
> On 1 Mar 2018, at 11:51, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> It makes sense to place the codecs inside the Stream which is charged
>> with setting it up, until we create a proper abstraction for input
>> messages similar to what Message does for output messages in follow-up
>> patches.
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 4d24234..403e65c 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -67,8 +67,11 @@ struct CursorMessage
>> 
>> class Stream
>> {
>> +    typedef std::set<SpiceVideoCodecType> codecs_t;
> 
> Not sure I'd present a typedef for it, but that's a matter of style.
> 
> As for the naming, normally we use CamelCase with no "_t" suffix for
> type names, do you wish to distinguish these member typedefs like this?

We use _t for typedefs ATM, I stuck with that. No strong reason to change it.

> 
>> +
>> public:
>>     Stream(const char *name)
>> +        : codecs()
>>     {
>>         streamfd = open(name, O_RDWR);
>>         if (streamfd < 0) {
>> @@ -80,6 +83,8 @@ public:
>>         close(streamfd);
>>     }
>> 
>> +    const codecs_t &client_codecs() { return codecs; }
>> +
>>     int read_command(bool blocking);
>>     size_t write_all(const void *buf, const size_t len);
>>     int send_format(unsigned w, unsigned h, uint8_t c);
>> @@ -98,6 +103,7 @@ private:
>> private:
>>     int streamfd = -1;
>>     std::mutex mutex;
>> +    codecs_t codecs;
>> };
>> 
>> }} // namespace spice::streaming_agent
>> @@ -105,7 +111,6 @@ private:
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> -static std::set<SpiceVideoCodecType> client_codecs;
>> 
>> int Stream::have_something_to_read(int timeout)
>> {
>> @@ -139,9 +144,9 @@ void Stream::handle_stream_start_stop(uint32_t len)
>>     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]);
>>     }
>> }
>> 
>> @@ -432,7 +437,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_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");
>>         }
> On 1 Mar 2018, at 11:56, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> It makes sense to place the codecs inside the Stream which is charged
>> with setting it up, until we create a proper abstraction for input
>> messages similar to what Message does for output messages in follow-up
>> patches.
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 4d24234..403e65c 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -67,8 +67,11 @@ struct CursorMessage
>> 
>> class Stream
>> {
>> +    typedef std::set<SpiceVideoCodecType> codecs_t;
>> +
>> public:
>>     Stream(const char *name)
>> +        : codecs()
> 
> this is not necessary

also harmless. But OK.

> 
>>     {
>>         streamfd = open(name, O_RDWR);
>>         if (streamfd < 0) {
>> @@ -80,6 +83,8 @@ public:
>>         close(streamfd);
>>     }
>> 
>> +    const codecs_t &client_codecs() { return codecs; }
>> +
> 
> I would add const here:
> 
>   const codecs_t &client_codecs() const { return codecs; }

OK.

> 
>>     int read_command(bool blocking);
>>     size_t write_all(const void *buf, const size_t len);
>>     int send_format(unsigned w, unsigned h, uint8_t c);
>> @@ -98,6 +103,7 @@ private:
>> private:
>>     int streamfd = -1;
>>     std::mutex mutex;
>> +    codecs_t codecs;
> 
> why not client_codecs?

that’s the name of the method.

> 
>> };
>> 
>> }} // namespace spice::streaming_agent
>> @@ -105,7 +111,6 @@ private:
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> -static std::set<SpiceVideoCodecType> client_codecs;
>> 
>> int Stream::have_something_to_read(int timeout)
>> {
>> @@ -139,9 +144,9 @@ void Stream::handle_stream_start_stop(uint32_t len)
>>     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]);
>>     }
>> }
>> 
>> @@ -432,7 +437,7 @@ do_capture(Stream &stream, const char *streamport, FILE
>> *f_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");
>>         }
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, Mar 01, 2018 at 08:28:12PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 1 Mar 2018, at 11:51, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin <dinechin@redhat.com>
> >> 
> >> It makes sense to place the codecs inside the Stream which is charged
> >> with setting it up, until we create a proper abstraction for input
> >> messages similar to what Message does for output messages in follow-up
> >> patches.
> >> 
> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> >> ---
> >> src/spice-streaming-agent.cpp | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> >> index 4d24234..403e65c 100644
> >> --- a/src/spice-streaming-agent.cpp
> >> +++ b/src/spice-streaming-agent.cpp
> >> @@ -67,8 +67,11 @@ struct CursorMessage
> >> 
> >> class Stream
> >> {
> >> +    typedef std::set<SpiceVideoCodecType> codecs_t;
> > 
> > Not sure I'd present a typedef for it, but that's a matter of style.
> > 
> > As for the naming, normally we use CamelCase with no "_t" suffix for
> > type names, do you wish to distinguish these member typedefs like this?
> 
> We use _t for typedefs ATM, I stuck with that. No strong reason to change it.

I could not find any _t typedef in spice-streaming-agent codebase
(actually, I could not find many typedefs at all ;)

Christophe