[Spice-devel] [PATCH v2 spice-streaming-agent 1/2] drop sstream and use dedicated functions instead
Snir Sheriber
ssheribe at redhat.com
Thu Jun 27 15:48:41 UTC 2019
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
More information about the Spice-devel
mailing list