[Spice-devel] [PATCH spice-streaming-agent 2/2] Do not use an encoding not supported by the client

[Frediano Ziglio]

ping

> 
> > 
> > On Tue, 2017-11-21 at 09:49 +0000, Frediano Ziglio wrote:
> > > This allows for instance old clients to work correctly.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  src/concrete-agent.cpp        | 5 ++++-
> > >  src/concrete-agent.hpp        | 3 ++-
> > >  src/spice-streaming-agent.cpp | 6 +++++-
> > >  3 files changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> > > index 192054a..404ee74 100644
> > > --- a/src/concrete-agent.cpp
> > > +++ b/src/concrete-agent.cpp
> > > @@ -98,7 +98,7 @@ void ConcreteAgent::LoadPlugin(const char
> > > *plugin_filename)
> > >      }
> > >  }
> > >  
> > > -FrameCapture *ConcreteAgent::GetBestFrameCapture()
> > > +FrameCapture *ConcreteAgent::GetBestFrameCapture(const
> > > std::set<SpiceVideoCodecType>& codecs)
> > 
> > 
> > I notice that there's a bit of inconsistency about specifying the std::
> > namespace or just using the bare type. There is a "using namespace
> > std;" statement in this file, and most standard library types are used
> > without specifying the namespace. But there there is at least one place
> > where we specify the namespace, and you've added another in this patch.
> > 
> 
> Usually I tend to use in the header to avoid having a global
> unwanted "using namespace" statement and avoid in the source (at least
> for std). We can define to always use the explicit namespace for "std".
> 
> > (as a matter of personal preference, I generally dislike 'using
> > namespace' statements, and I prefer to specify the std:: namespace
> > explicitly)
> > 
> > 
> > > 
> > 
> > >  {
> > >      vector<pair<unsigned, shared_ptr<Plugin>>> sorted_plugins;
> > >  
> > > @@ -113,6 +113,9 @@ FrameCapture
> > > *ConcreteAgent::GetBestFrameCapture()
> > >          if (plugin.first == DontUse) {
> > >              break;
> > 
> > Unrelated to this patch, but shouldn't the statement above be
> > 'continue' rather than 'break'?
> > 
> 
> They are sorted based on rank so all others are DontUse too.
> 
> > 
> > >          }
> > > +        // check client supports the codec
> > > +        if (codecs.find(plugin.second->VideoCodecType()) ==
> > > codecs.end())
> > > +            continue;
> > >          FrameCapture *capture = plugin.second->CreateCapture();
> > >          if (capture) {
> > >              return capture;
> > > diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> > > index 828368b..6c300fa 100644
> > > --- a/src/concrete-agent.hpp
> > > +++ b/src/concrete-agent.hpp
> > > @@ -7,6 +7,7 @@
> > >  #define SPICE_STREAMING_AGENT_CONCRETE_AGENT_HPP
> > >  
> > >  #include <vector>
> > > +#include <set>
> > >  #include <memory>
> > >  #include <spice-streaming-agent/plugin.hpp>
> > >  
> > > @@ -33,7 +34,7 @@ public:
> > >      void LoadPlugins(const char *directory);
> > >      // pointer must remain valid
> > >      void AddOption(const char *name, const char *value);
> > > -    FrameCapture *GetBestFrameCapture();
> > > +    FrameCapture *GetBestFrameCapture(const
> > > std::set<SpiceVideoCodecType>& codecs);
> >      bool PluginVersionIsCompatible(unsigned pluginVersion) const
> > > override;
> > >  private:
> > >      void LoadPlugin(const char *plugin_filename);
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> > > agent.cpp
> > > index 20f2925..5c0bbb9 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -52,6 +52,7 @@ struct SpiceStreamDataMessage
> > >  };
> > >  
> > >  static int streaming_requested;
> > > +static std::set<SpiceVideoCodecType> client_codecs;
> > >  static bool quit;
> > >  static int streamfd = -1;
> > >  static bool stdin_ok;
> > > @@ -143,6 +144,9 @@ static int read_command_from_device(void)
> > >      streaming_requested = msg[0]; /* num_codecs */
> > >      syslog(LOG_INFO, "GOT START_STOP message -- request to %s
> > > streaming\n",
> > >             streaming_requested ? "START" : "STOP");
> > > +    client_codecs.clear();
> > > +    for (int i = 1; i <= msg[0]; ++i)
> > > +        client_codecs.insert((SpiceVideoCodecType) msg[i]);
> > >      return 1;
> > >  }
> > >  
> > > @@ -363,7 +367,7 @@ do_capture(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());
> > > +        std::unique_ptr<FrameCapture>
> > > capture(agent.GetBestFrameCapture(client_codecs));
> > >          if (!capture)
> > >              throw std::runtime_error("cannot find a suitable capture
> > > system");
> > >  
> > 
> > 
> > Otherwise it looks ok to me
> > 
> > Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> > 
> > 
>