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

[Jonathon Jongsma]

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.

(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'?


>          }
> +        // 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>