[Spice-devel] [RFC spice-streaming-agent 4/4] concrete-agent: prioritize requested codec for plugin selection

[Frediano Ziglio]

> 
> This patch gives more priority to the requested video codecs when
> selecting the FrameCapture plugin, instead of its hard-coded rank.
> 
> The client_codecs storage structure is changed from 'set' to 'vector',
> as the codec order is not preserved by the set structure..
> 
> Signed-off-by: Kevin Pouget <kpouget at redhat.com>
> ---
>  src/concrete-agent.cpp        | 38 +++++++++++++++++------------------
>  src/concrete-agent.hpp        |  2 +-
>  src/spice-streaming-agent.cpp |  2 +-
>  src/stream-port.cpp           |  2 +-
>  src/stream-port.hpp           |  4 ++--
>  5 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 336bd09..683fa26 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -111,7 +111,7 @@ void ConcreteAgent::LoadPlugin(const std::string
> &plugin_filename)
>      }
>  }
>  
> -FrameCapture *ConcreteAgent::GetBestFrameCapture(const
> std::set<SpiceVideoCodecType>& codecs)
> +FrameCapture *ConcreteAgent::GetBestFrameCapture(const
> std::vector<SpiceVideoCodecType>& codecs)
>  {
>      std::vector<std::pair<unsigned, std::shared_ptr<Plugin>>>
>      sorted_plugins;
>  
> @@ -121,24 +121,24 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const
> std::set<SpiceVideoCodecT
>      }
>      sort(sorted_plugins.rbegin(), sorted_plugins.rend());
>  
> -    // return first not null
> -    for (const auto& plugin: sorted_plugins) {
> -        if (plugin.first == DontUse) {
> -            break;
> -        }
> -        // check client supports the codec
> -        if (codecs.find(plugin.second->VideoCodecType()) == codecs.end())
> -            continue;
> -
> -        FrameCapture *capture;
> -        try {
> -            capture = plugin.second->CreateCapture();
> -        } catch (const std::exception &err) {
> -            syslog(LOG_ERR, "Error creating capture engine: %s",
> err.what());
> -            continue;
> -        }
> -        if (capture) {
> -            return capture;
> +    for (const auto& codec_type: codecs) {
> +        // find the plugin with the highest rank for this codec type
> +        for (const auto& plugin: sorted_plugins) {
> +            if (plugin.first == DontUse) {
> +                continue;
> +            }
> +
> +            // check client supports the codec
> +            if (plugin.second->VideoCodecType() != codec_type) {
> +                continue;
> +            }
> +
> +            try {
> +                return plugin.second->CreateCapture();
> +            } catch (const std::exception &err) {
> +                syslog(LOG_ERR, "Error creating capture engine: %s",
> err.what());
> +                continue;
> +            }
>          }
>      }
>      return nullptr;

I think that previous code was entirely in favour of agent preference
but the new one looks too much in favour of the client.
Also adding preference to a message not designed to do so is not
100% correct. It's not a big hack but was not designed to do so.
For instance is impossible for the client to specify that 2 codecs
have the same preference or how much one should be favourite instead
of the other.

I would generate a rank that take in account both agent and client
preference and use the sorted_plugins as was used before instead
of taking an already sorted array and ignoring most of the sorting
and ranking.

Maybe normalize rank for agent and client to a 0-255 range and
compute:

  total_rank = agent_rank * 16 + client_rank;

?? this will favour the agent but still give the possibility
for the client to override a bit.
The client_rank should come from a similar start/stop message with
added rank.

> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index 6d1be35..0399bab 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -33,7 +33,7 @@ public:
>      void Register(const std::shared_ptr<Plugin>& plugin) override;
>      const ConfigureOption* Options() const override;
>      void LoadPlugins(const std::string &directory);
> -    FrameCapture *GetBestFrameCapture(const std::set<SpiceVideoCodecType>&
> codecs);
> +    FrameCapture *GetBestFrameCapture(const
> std::vector<SpiceVideoCodecType>& codecs);
>      __attribute__ ((format (printf, 2, 3)))
>      void LogStat(const char* format, ...) override;
>  private:
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index d274b5f..15c2732 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -128,7 +128,7 @@ private:
>  static bool capture_in_progress = false;
>  static bool reset_requested = false;
>  static bool quit_requested = false;
> -static std::set<SpiceVideoCodecType> client_codecs;
> +static std::vector<SpiceVideoCodecType> client_codecs;
>  
>  static bool have_something_to_read(StreamPort &stream_port, bool blocking)
>  {
> diff --git a/src/stream-port.cpp b/src/stream-port.cpp
> index 2670120..0d08d50 100644
> --- a/src/stream-port.cpp
> +++ b/src/stream-port.cpp
> @@ -43,7 +43,7 @@ StartStopMessage
> InboundMessage::get_payload<StartStopMessage>()
>      }
>  
>      for (size_t i = 1; i <= data[0]; ++i) {
> -        msg.client_codecs.insert((SpiceVideoCodecType) data[i]);
> +        msg.client_codecs.push_back((SpiceVideoCodecType) data[i]);
>      }
>  
>      return msg;
> diff --git a/src/stream-port.hpp b/src/stream-port.hpp
> index 08473f7..b2f7520 100644
> --- a/src/stream-port.hpp
> +++ b/src/stream-port.hpp
> @@ -16,7 +16,7 @@
>  #include <string>
>  #include <memory>
>  #include <mutex>
> -#include <set>
> +#include <vector>
>  
>  
>  namespace spice {
> @@ -45,7 +45,7 @@ public:
>  struct StartStopMessage
>  {
>      bool start_streaming = false;
> -    std::set<SpiceVideoCodecType> client_codecs;
> +    std::vector<SpiceVideoCodecType> client_codecs;
>  };
>  
>  struct InCapabilitiesMessage {};

Frediano