[Spice-devel] [RFC spice-streaming-agent 1/2] gst-plugin: allow the instantiation of multiple GST encoder plugins
Snir Sheriber
ssheribe at redhat.com
Sun Aug 4 08:13:03 UTC 2019
HI,
On 8/1/19 6:27 PM, Kevin Pouget wrote:
> Hello Snir,
>
> On Thu, Aug 1, 2019 at 5:02 PM Snir Sheriber <ssheribe at redhat.com> wrote:
>> Hi,
>>
>>
>> On 8/1/19 3:43 PM, Kevin Pouget wrote:
>>> On Wed, Jul 31, 2019 at 12:09 PM Kevin Pouget <kpouget at redhat.com> wrote:
>>>> On Wed, Jul 31, 2019 at 11:50 AM Frediano Ziglio <fziglio at redhat.com> wrote:
>>>>>> With this patch, spice-streaming-agent can be launched with multiple
>>>>>> Gstreamer video codecs enabled:
>>>>>>
>>>>>>> spice-streaming-agent -c gst.codec=vp8 -c gst.codec=vp9 ...
>>>>>> ---
>>>>>> src/gst-plugin.cpp | 50 ++++++++++++++++++++++++++++------------------
>>>>>> 1 file changed, 31 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
>>>>>> index 9858beb..6252e42 100644
>>>>>> --- a/src/gst-plugin.cpp
>>>>>> +++ b/src/gst-plugin.cpp
>>>>>> @@ -102,7 +102,7 @@ class GstreamerPlugin final: public Plugin
>>>>>> public:
>>>>>> FrameCapture *CreateCapture() override;
>>>>>> unsigned Rank() override;
>>>>>> - void ParseOptions(const ConfigureOption *options);
>>>>>> + void ParseOptions(const ConfigureOption *options, SpiceVideoCodecType
>>>>>> codec);
>>>>>> SpiceVideoCodecType VideoCodecType() const override {
>>>>>> return settings.codec;
>>>>>> }
>>>>>> @@ -419,8 +419,10 @@ unsigned GstreamerPlugin::Rank()
>>>>>> return SoftwareMin;
>>>>>> }
>>>>>>
>>>>>> -void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
>>>>>> +void GstreamerPlugin::ParseOptions(const ConfigureOption *options,
>>>>>> SpiceVideoCodecType codec)
>>>>>> {
>>>>>> + settings.codec = codec;
>>>>>> +
>>>>>> for (; options->name; ++options) {
>>>>>> const std::string name = options->name;
>>>>>> const std::string value = options->value;
>>>>>> @@ -431,20 +433,6 @@ void GstreamerPlugin::ParseOptions(const ConfigureOption
>>>>>> *options)
>>>>>> } catch (const std::exception &e) {
>>>>>> throw std::runtime_error("Invalid value '" + value + "' for
>>>>>> option 'framerate'.");
>>>>>> }
>>>>>> - } else if (name == "gst.codec") {
>>>>>> - if (value == "h264") {
>>>>>> - settings.codec = SPICE_VIDEO_CODEC_TYPE_H264;
>>>>>> - } else if (value == "vp9") {
>>>>>> - settings.codec = SPICE_VIDEO_CODEC_TYPE_VP9;
>>>>>> - } else if (value == "vp8") {
>>>>>> - settings.codec = SPICE_VIDEO_CODEC_TYPE_VP8;
>>>>>> - } else if (value == "mjpeg") {
>>>>>> - settings.codec = SPICE_VIDEO_CODEC_TYPE_MJPEG;
>>>>>> - } else if (value == "h265") {
>>>>>> - settings.codec = SPICE_VIDEO_CODEC_TYPE_H265;
>>>>>> - } else {
>>>>>> - throw std::runtime_error("Invalid value '" + value + "' for
>>>>>> option 'gst.codec'.");
>>>>>> - }
>>>>>> } else if (name == "gst.encoder") {
>>>>>> settings.encoder = value;
>>>>>> }
>>>>>> @@ -459,11 +447,35 @@ SPICE_STREAMING_AGENT_PLUGIN(agent)
>>>>>> {
>>>>>> gst_init(nullptr, nullptr);
>>>>>>
>>>>>> - auto plugin = std::make_shared<GstreamerPlugin>();
>>>>>> + auto options = agent->Options();
>>>>>> + for (; options->name; ++options) {
>>>>>> + const std::string name = options->name;
>>>>>> + const std::string value = options->value;
>>>>>>
>>>>>> - plugin->ParseOptions(agent->Options());
>>>>>> + if (name == "gst.codec") {
>>>>>> + SpiceVideoCodecType codec_type;
>>>>>> + if (value == "mjpeg") {
>>>>>> + codec_type = SPICE_VIDEO_CODEC_TYPE_MJPEG;
>>>>>> + } else if (value == "h264") {
>>>>>> + codec_type = SPICE_VIDEO_CODEC_TYPE_H264;
>>>>>> + } else if (value == "h265") {
>>>>>> + codec_type = SPICE_VIDEO_CODEC_TYPE_H265;
>>>>>> + } else if (value == "vp8") {
>>>>>> + codec_type = SPICE_VIDEO_CODEC_TYPE_VP8;
>>>>>> + } else if (value == "vp9") {
>>>>>> + codec_type = SPICE_VIDEO_CODEC_TYPE_VP9;
>>>>>> + } else {
>>>>>> + throw std::runtime_error("Invalid value '" + value + "' for
>>>>>> option 'gst.codec'.");
>>>>>> + }
>>>>>> +
>>>>>> + auto plugin = std::make_shared<GstreamerPlugin>();
>>>>>> + plugin->ParseOptions(agent->Options(), codec_type);
>>>>>> + agent->Register(plugin);
>>>>>> + }
>>>>>> + }
>>>>>>
>>>>>> - agent->Register(plugin);
>>>>>> + // no default value at the moment
>>>>>> + // (used to be h264, see GstreamerEncoderSettings::codec)
>>>>>>
>>>>>> return true;
>>>>>> }
>>>>> I didn't test it but it makes perfectly sense.
>>>>> On the other hand I'm wondering what will happen to the other
>>>>> parameters. They will be "broadcasted" to all plugins.
>>>>> For "framerate" is fine but what will happen if you specify
>>>>> also properties (gst.prop) and encoder (gst.encoder) ?
>>>> maybe the easiest is to say that "gst.XXX=YYY" applies to all the
>>>> codecs, and "gst.CODEC.XXX=YYY" is only for one codec, with
>>>> XXX=prop|encoder at the moment.
>>> Hello,
>>>
>>> I did the modifications below to allow passing codec-specific gst parameters:
>>
>> Basically Gstreamer properties are more tied to the encoder than
>> the codec type e.g. vaapih264enc plugin has a property named "low-delay-b"
>> and x264enc hasn't.
>>
>> Now I'm thinking maybe i should have allow to set properties only when the
>> encoder is specified :P, anyway i think we do need to parse it in order
>> somehow
>> i.e. vp8 + its encoder + its props... , another vp8 + its props ... ,
> I am confused with this: how should the 2nd vp8 encoder get into execution?
> at the moment the server requests "vp8" playback, so a second gst.vp8
> plugin cannot be selected
>
> or you have something else in mind?
For the switching codec use case it is indeed doesn't matter
It's just make more sense to me to associate the properties in
such manner since the available properties are usually associated
with the encoder-plugin and not with the codec type itself
Another point is that may also be used for future purposes.
For example to register h264 encoder in two different
configurations and switch between them, or to register hw
encoder and sw encoder as a fallback with same codec...
Snir.
>
>> h264 + its encoder...
>> I did no try it yet, I'll test it and have a closer look on everything,
>> i like this feature!
>>
>> Thanks,
>> Snir.
>>
>>
>>>> gst.codec=CODEC
>>>> gst.prop | gst.CODEC.prop
>>>> gst.encoder | gst.CODEC.prop
>>>> framerate | gst.CODEC.framerate
>>> regards,
>>>
>>> Kevin
>>>
>>> ---
>>> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
>>> index 5469647..4194485 100644
>>> --- a/src/gst-plugin.cpp
>>> +++ b/src/gst-plugin.cpp
>>> @@ -102,7 +102,8 @@ class GstreamerPlugin final: public Plugin
>>> public:
>>> FrameCapture *CreateCapture() override;
>>> unsigned Rank() override;
>>> - void ParseOptions(const ConfigureOption *options,
>>> SpiceVideoCodecType codec);
>>> + void ParseOptions(const ConfigureOption *options,
>>> SpiceVideoCodecType codec,
>>> + const std::string codec_name);
>>> SpiceVideoCodecType VideoCodecType() const override {
>>> return settings.codec;
>>> }
>>> @@ -431,7 +432,8 @@ unsigned GstreamerPlugin::Rank()
>>> return SoftwareMin;
>>> }
>>>
>>> -void GstreamerPlugin::ParseOptions(const ConfigureOption *options,
>>> SpiceVideoCodecType codec)
>>> +void GstreamerPlugin::ParseOptions(const ConfigureOption *options,
>>> + SpiceVideoCodecType codec, const
>>> std::string codec_name)
>>> {
>>> settings.codec = codec;
>>>
>>> @@ -439,15 +441,17 @@ void GstreamerPlugin::ParseOptions(const
>>> ConfigureOption *options, SpiceVideoCo
>>> const std::string name = options->name;
>>> const std::string value = options->value;
>>>
>>> - if (name == "framerate") {
>>> + if (name == "framerate" || name == "gst." + codec_name +
>>> ".framerate") {
>>> try {
>>> settings.fps = std::stoi(value);
>>> +
>>> } catch (const std::exception &e) {
>>> - throw std::runtime_error("Invalid value '" + value +
>>> "' for option 'framerate'.");
>>> + throw std::runtime_error("Invalid value '" + value + '" "
>>> + "for option '" + name + "'.");
>>> }
>>> - } else if (name == "gst.encoder") {
>>> + } else if (name == "gst.encoder" || name == "gst." +
>>> codec_name +".encoder") {
>>> settings.encoder = value;
>>> - } else if (name == "gst.prop") {
>>> + } else if (name == "gst.prop" || name == "gst." + codec_name
>>> + ".prop") {
>>> size_t pos = value.find('=');
>>> if (pos == 0 || pos >= value.size() - 1) {
>>> gst_syslog(LOG_WARNING, "Property input is invalid
>>> ('%s' Ignored)", value.c_str());
>>> @@ -488,7 +492,7 @@ SPICE_STREAMING_AGENT_PLUGIN(agent)
>>> }
>>>
>>> auto plugin = std::make_shared<GstreamerPlugin>();
>>> - plugin->ParseOptions(agent->Options(), codec_type);
>>> + plugin->ParseOptions(agent->Options(), codec_type, value);
>>> agent->Register(plugin);
>>> }
>>> }
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list