[Spice-devel] [PATCH v2 spice-streaming-agent 1/2] drop sstream and use dedicated functions instead

[Snir Sheriber]

Hi,


On 6/27/19 5:12 PM, Frediano Ziglio wrote:
>> Instead of manipulating a string and convert it to caps just
>> use dedicated functions instead
>>
>> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
>> ---
>> Changes:
>> -commit log
>> -Suggesting uniqptr for caps_str
>> --
>>   src/gst-plugin.cpp | 34 ++++++++++++++++++----------------
>>   1 file changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
>> index 9858beb..0dd7796 100644
>> --- a/src/gst-plugin.cpp
>> +++ b/src/gst-plugin.cpp
>> @@ -8,7 +8,6 @@
>>   #include <cstring>
>>   #include <exception>
>>   #include <stdexcept>
>> -#include <sstream>
>>   #include <memory>
>>   #include <syslog.h>
>>   #include <unistd.h>
>> @@ -132,34 +131,35 @@ GstElement
>> *GstreamerFrameCapture::get_encoder_plugin(const GstreamerEncoderSett
>>       GList *filtered;
>>       GstElement *encoder;
>>       GstElementFactory *factory = nullptr;
>> -    std::stringstream caps_ss;
>>   
>>       switch (settings.codec) {
>>       case SPICE_VIDEO_CODEC_TYPE_H264:
>> -        caps_ss << "video/x-h264" << ",stream-format=(string)byte-stream";
>> +        sink_caps.reset(gst_caps_new_simple("video/x-h264",
>> +                                            "stream-format", G_TYPE_STRING,
>> "byte-stream",
>> +                                            NULL));
>>           break;
>>       case SPICE_VIDEO_CODEC_TYPE_MJPEG:
>> -        caps_ss << "image/jpeg";
>> +        sink_caps.reset(gst_caps_new_empty_simple("image/jpeg"));
>>           break;
>>       case SPICE_VIDEO_CODEC_TYPE_VP8:
>> -        caps_ss << "video/x-vp8";
>> +        sink_caps.reset(gst_caps_new_empty_simple("video/x-vp8"));
>>           break;
>>       case SPICE_VIDEO_CODEC_TYPE_VP9:
>> -        caps_ss << "video/x-vp9";
>> +        sink_caps.reset(gst_caps_new_empty_simple("video/x-vp9"));
>>           break;
>>       case SPICE_VIDEO_CODEC_TYPE_H265:
>> -        caps_ss << "video/x-h265";
>> +        sink_caps.reset(gst_caps_new_empty_simple("video/x-h265"));
>>           break;
>>       default : /* Should not happen - just to avoid compiler's complaint */
>>           throw std::logic_error("Unknown codec");
>>       }
>> -    caps_ss << ",framerate=" << settings.fps << "/1";
>> +    gst_caps_set_simple(sink_caps.get(), "framerate", GST_TYPE_FRACTION,
>> settings.fps, 1, nullptr);
>> +    std::unique_ptr<gchar> caps_str(gst_caps_to_string(sink_caps.get()));
>>   
> Idea it's fine. This however will call ::operator new which it's not what
> you want. In the same file there are different deleters like
>
>
> struct GstSampleDeleter {
>      void operator()(GstSample* p)
>      {
>          gst_sample_unref(p);
>      }
> };
>
> using GstSampleUPtr = std::unique_ptr<GstSample, GstSampleDeleter>;
>
>
> you can base your one. Glibmm uses an utility (see
> https://gitlab.gnome.org/GNOME/glibmm/blob/b92ef23e38f9863ebe4704a916e3322eea2f1f2c/glib/glibmm/utility.h)
> but potentially each smart pointer will contain a pointer to g_free
> (and it's limited to g_free like a single deleter).


How about this oneliner instead?

std::unique_ptr<gchar, decltype(&g_free)> 
caps_str(gst_caps_to_string(sink_caps.get()), &g_free);

Snir.


>>       encoders =
>>       gst_element_factory_list_get_elements(GST_ELEMENT_FACTORY_TYPE_VIDEO_ENCODER,
>>       GST_RANK_NONE);
>> -    sink_caps.reset(gst_caps_from_string(caps_ss.str().c_str()));
>>       filtered = gst_element_factory_list_filter(encoders, sink_caps.get(),
>>       GST_PAD_SRC, false);
>>       if (filtered) {
>> -        gst_syslog(LOG_NOTICE, "Looking for encoder plugins which can
>> produce a '%s' stream", caps_ss.str().c_str());
>> +        gst_syslog(LOG_NOTICE, "Looking for encoder plugins which can
>> produce a '%s' stream", caps_str.get());
>>           for (GList *l = filtered; l != nullptr; l = l->next) {
>>               if (!factory &&
>>               !settings.encoder.compare(GST_ELEMENT_NAME(l->data))) {
>>                   factory = (GstElementFactory*)l->data;
>> @@ -169,13 +169,13 @@ GstElement
>> *GstreamerFrameCapture::get_encoder_plugin(const GstreamerEncoderSett
>>           if (!factory && !settings.encoder.empty()) {
>>               gst_syslog(LOG_WARNING,
>>                          "Specified encoder named '%s' cannot produce '%s'
>>                          stream, make sure matching gst.codec is specified
>>                          and plugin's availability",
>> -                       settings.encoder.c_str(), caps_ss.str().c_str());
>> +                       settings.encoder.c_str(), caps_str.get());
>>           }
>>           factory = factory ? factory : (GstElementFactory*)filtered->data;
>>           gst_syslog(LOG_NOTICE, "'%s' encoder plugin is used",
>>           GST_ELEMENT_NAME(factory));
>>   
>>       } else {
>> -        gst_syslog(LOG_ERR, "No suitable encoder was found for '%s'",
>> caps_ss.str().c_str());
>> +        gst_syslog(LOG_ERR, "No suitable encoder was found for '%s'",
>> caps_str.get());
>>       }
>>   
>>       encoder = factory ? gst_element_factory_create(factory, "encoder") :
>>       nullptr;
>> @@ -351,10 +351,12 @@ void GstreamerFrameCapture::xlib_capture()
>>           throw std::runtime_error("Failed to wrap image in gstreamer
>>           buffer");
>>       }
>>   
>> -    std::stringstream ss;
>> -
>> -    ss << "video/x-raw,format=BGRx,width=" << image->width << ",height=" <<
>> image->height << ",framerate=" << settings.fps << "/1";
>> -    GstCapsUPtr caps(gst_caps_from_string(ss.str().c_str()));
>> +    GstCapsUPtr caps(gst_caps_new_simple("video/x-raw",
>> +                                         "format", G_TYPE_STRING, "BGRx",
>> +                                         "width", G_TYPE_INT, image->width,
>> +                                         "height", G_TYPE_INT,
>> image->height,
>> +                                         "framerate", GST_TYPE_FRACTION,
>> settings.fps, 1,
>> +                                         NULL));
>>   
>>       // Push sample
>>       GstSampleUPtr appsrc_sample(gst_sample_new(buf, caps.get(), nullptr,
>>       nullptr));
> Frediano