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

[Frediano Ziglio]

> 
> 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.
> 

Maybe it will pass the 100 characters and you will need to split it in 2 lines,
but beside that I'm fine with it.
You can remove the "&" in front of the last g_free.

... omissis ...

Frediano