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

Submitted by Kevin Pouget on Aug. 6, 2019, 3:34 p.m.

Details

Message ID 20190806153453.20616-8-kpouget@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 3 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Kevin Pouget Aug. 6, 2019, 3:34 p.m.
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@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(-)

Patch hide | download patch | download mbox

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;
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 {};

Comments

On 8/6/19 6:34 PM, Kevin Pouget wrote:
> This patch gives more priority to the requested video codecs when
> selecting the FrameCapture plugin, instead of its hard-coded rank.


We may want to mention this priority also in the header file.

Snir.

> 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@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;
> 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 {};
> 
> 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@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
On Mon, Aug 12, 2019 at 9:43 AM Frediano Ziglio <fziglio@redhat.com> wrote:
>
> >
> > 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@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 favor 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.

my idea was to delegate the codec choice to the server (it's the
server who sends the codec list received by the agent), so that a) the
client gives its preference, maybe with a weight, b) the agent gives
its capabilities, maybe with a weight, c) the admin or a quality
adjustment plugin gives it's preference, and the agent executes
without any re-computation

> 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.

if going this way, I would rather say something like this:

> agent rank between 0 and 127
> server rank between 0 and 255

> don't use plugin if (agent+server) < 128

> server doesn't want the plugin: rank 0 --> don't use
> agent doesn't like the codec: rank 0, plugin likes it --> rank 127
> agent likes the codec: rank 127, server doesn't like: rank 1 --> rank 128
> agent likes the codec: rank 127, server likes it --> rank 382

this gives more value to the dynamic value (coming from the server)
instead of the hardcoded one (coming from the plugin)

and one thing we must keep in mind, it's that in the near future, we
would like to be able a) to change the encoding parameters b) possibly
to change the encoder for a given codec (eg, to switch between SW and
HW encoding)

> 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
> 
> On Mon, Aug 12, 2019 at 9:43 AM Frediano Ziglio <fziglio@redhat.com> wrote:
> >
> > >
> > > 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@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 favor 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.
> 
> my idea was to delegate the codec choice to the server (it's the
> server who sends the codec list received by the agent), so that a) the
> client gives its preference, maybe with a weight, b) the agent gives
> its capabilities, maybe with a weight, c) the admin or a quality
> adjustment plugin gives it's preference, and the agent executes
> without any re-computation
> 

It makes sense. I had a look again at the various messages.
I though "preferred" message (from client to server) had weight, I was
surprised he had just an ordered list! The message from server to
agent (streaming agent) is supposed to be unsorted ("supported") but
can be "reinterpret" to be ordered (I don't see much troubles).
However ordered does not allow to specify weight which are basically
specified by the order so you cannot specify the 2 codecs have the same
priority or that one should be used as a really fallback.
Another thing you would like to consider is the codec support in the
agent. One could choose a codec as it has better support in the guest,
for instance because the guest has H/W support for it. This knowledge
is not in the server so this would require either to provide this information
to the server (agent -> server) or make the message to the agent more
complicated (being able to tell "prefer this if h/w encoding is supported").

Not telling that I have the solution on my drawer

> > 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.
> 
> if going this way, I would rather say something like this:
> 
> > agent rank between 0 and 127
> > server rank between 0 and 255
> 
> > don't use plugin if (agent+server) < 128
> 
> > server doesn't want the plugin: rank 0 --> don't use
> > agent doesn't like the codec: rank 0, plugin likes it --> rank 127
> > agent likes the codec: rank 127, server doesn't like: rank 1 --> rank 128
> > agent likes the codec: rank 127, server likes it --> rank 382
> 
> this gives more value to the dynamic value (coming from the server)
> instead of the hardcoded one (coming from the plugin)
> 
> and one thing we must keep in mind, it's that in the near future, we
> would like to be able a) to change the encoding parameters b) possibly
> to change the encoder for a given codec (eg, to switch between SW and
> HW encoding)
> 

Is not much clear why this should affect codec selection and why one should
prefer S/W encoding to H/W one if both are available and why later should
be able to change its mind. Beside testing purpose I would say, but in this
case I would go hard and just try to set the one I want not leaving much
choice.

> > 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
>