[Spice-devel] [PATCH spice-streaming-agent 1/2] gst-plugin: Drop sstream and use dedicated functions instead

[Frediano Ziglio]

> 
> Hi,
> 
> 
> On 6/26/19 6:24 PM, Frediano Ziglio wrote:
> >> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
> > Can you explain why this is better? It's not clear from the code.
> 
> 
> Actually it is not much better, it just seems more reasonable to me
> to set this properties using a dedicated function than manipulating
> a string and and then convert it.
> 

Fine with me, just write it in the next commit message.

> 
> >
> >> ---
> >>   src/gst-plugin.cpp | 33 +++++++++++++++++----------------
> >>   1 file changed, 17 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> >> index 9858beb..cf660eb 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,33 @@ 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", nullptr));
> >>           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);
> >> +    gchar *caps_str = gst_caps_to_string(sink_caps.get());
> > As we are using exception I would avoid using naked pointers.
> 
> 
> Will send another proposal
> 
> Snir.
> 

Not much suggestions on encapsulating gchar* in a class or using std::string
and free gst_caps_to_string ASAP. That's why I liked the "defer" style macro
time ago.

> 
> >
> >>   
> >>       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);
> >>           for (GList *l = filtered; l != nullptr; l = l->next) {
> >>               if (!factory &&
> >>               !settings.encoder.compare(GST_ELEMENT_NAME(l->data))) {
> >>                   factory = (GstElementFactory*)l->data;
> >> @@ -169,14 +167,15 @@ 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);
> >>           }
> >>           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);
> >>       }
> >> +    g_free(caps_str);
> > This line should not exist.
> >
> >>   
> >>       encoder = factory ? gst_element_factory_create(factory, "encoder") :
> >>       nullptr;
> >>       if (encoder) { // Invalid properties will be ignored silently
> >> @@ -351,10 +350,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,
> >> +                                         nullptr));
> >>   
> >>       // Push sample
> >>       GstSampleUPtr appsrc_sample(gst_sample_new(buf, caps.get(), nullptr,
> >>       nullptr));