>
> On Mon, Aug 12, 2019 at 9:43 AM Frediano Ziglio <fziglio@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel