[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ý <lhrazky at redhat.com> wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin at 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 at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>