Re: [Spice-devel] [PATCH 16/17] Remove client_codecs global variable, moved inside the 'Stream' class

[Lukáš Hrázký]

On Tue, 2018-02-20 at 17:20 +0100, Christophe de Dinechin wrote:
> > On 20 Feb 2018, at 16:51, Lukáš Hrázký  wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin 
> > > 
> > > This is not a really nice abstraction at this point, but still a step in 
> > > the right way
> > > 
> > > Signed-off-by: Christophe de Dinechin 
> > > ---
> > > 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 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 _codecs() { return codecs; }
> > > +
> > > int have_something_to_read(int timeout);
> > > int read_command_from_device(void);
> > > int read_command(bool blocking);
> > > 
> > > -
> > > template 
> > > 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 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 , const char *streamport, 
> > > FrameLog _log)
> > > syslog(LOG_INFO, "streaming starts now\n");
> > > uint64_t time_last = 0;
> > > 
> > > -std::unique_ptr 
> > > capture(agent.GetBestFrameCapture(client_codecs));
> > > +std::unique_ptr 
> > > 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
> 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 16/17] Remove client_codecs global variable, moved inside the 'Stream' class

[Christophe de Dinechin]

> On 20 Feb 2018, at 16:51, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> This is not a really nice abstraction at this point, but still a step in the 
>> right way
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> 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 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 _codecs() { return codecs; }
>> +
>> int have_something_to_read(int timeout);
>> int read_command_from_device(void);
>> int read_command(bool blocking);
>> 
>> -
>> template 
>> 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 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 , const char *streamport, 
>> FrameLog _log)
>> syslog(LOG_INFO, "streaming starts now\n");
>> uint64_t time_last = 0;
>> 
>> -std::unique_ptr 
>> capture(agent.GetBestFrameCapture(client_codecs));
>> +std::unique_ptr 
>> 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

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

Re: [Spice-devel] [PATCH 16/17] Remove client_codecs global variable, moved inside the 'Stream' class

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> This is not a really nice abstraction at this point, but still a step in the 
> right way
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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 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 _codecs() { return codecs; }
> +
>  int have_something_to_read(int timeout);
>  int read_command_from_device(void);
>  int read_command(bool blocking);
>  
> -
>  template 
>  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 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 , const char *streamport, 
> FrameLog _log)
>  syslog(LOG_INFO, "streaming starts now\n");
>  uint64_t time_last = 0;
>  
> -std::unique_ptr 
> capture(agent.GetBestFrameCapture(client_codecs));
> +std::unique_ptr 
> 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe de Dinechin]

From: Christophe de Dinechin 

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

Signed-off-by: Christophe de Dinechin 
---
 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 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 _codecs() { return codecs; }
+
 int have_something_to_read(int timeout);
 int read_command_from_device(void);
 int read_command(bool blocking);
 
-
 template 
 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 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 , const char *streamport, FrameLog 
_log)
 syslog(LOG_INFO, "streaming starts now\n");
 uint64_t time_last = 0;
 
-std::unique_ptr 
capture(agent.GetBestFrameCapture(client_codecs));
+std::unique_ptr 
capture(agent.GetBestFrameCapture(stream.client_codecs()));
 if (!capture)
 throw std::runtime_error("cannot find a suitable capture system");
 
-- 
2.13.5 (Apple Git-94)

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