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

[Christophe de Dinechin]

Ack

> On 29 Jan 2018, at 10:56, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> ping
> 
>> 
>>> 
>>> 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.
>>> 
>> 
>> Usually I tend to use in the header to avoid having a global
>> unwanted "using namespace" statement and avoid in the source (at least
>> for std). We can define to always use the explicit namespace for "std".
>> 
>>> (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'?
>>> 
>> 
>> They are sorted based on rank so all others are DontUse too.
>> 
>>> 
>>>>         }
>>>> +        // 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>
>>> 
>>> 
>> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel