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

[Kevin Pouget]

On Mon, Aug 12, 2019 at 9:43 AM Frediano Ziglio <fziglio at 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 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 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