On Wed, 2018-06-20 at 10:04 +0300, Snir Sheriber wrote:
> HI,
>
> Thanks for reviewing this
>
>
> No comment = mostly agreed, will be changed :)
>
>
> On 06/18/2018 06:15 PM, Lukáš Hrázký wrote:
> > Hi Snir,
> >
> > mostly C++ and error handling notes :) I hope it's not too painful :D
> >
> >
> > On Sun, 2018-06-10 at 11:14 +0300, Snir Sheriber wrote:
> > > Gstreamer based plugin utilizing gstreamer elements to capture
> > > screen from X, convert and encode into h264/vp8/vp9/mjpeg stream
> > > Configure with --enable-gstreamer, will be built as a separate plugin.
> > >
> > > The plugin was made for testing purposes, it was mainly tested with
> > > the x264enc (h264 is the defualt codec) encoder.
> > > To choose codec type use: '-c gst.codec=<h264/vp8/vp9/mjpeg>'
> > > To specify a certain plugin use: '-c gst.encoder=<plugin name>' in
> > > addition to its matching codec type (gst.codec).
> > > ---
> > > configure.ac | 15 ++
> > > src/Makefile.am | 27 +++
> > > src/gst-plugin.cpp | 430 +++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 472 insertions(+)
> > > create mode 100644 src/gst-plugin.cpp
> > >
> > > diff --git a/configure.ac b/configure.ac
> > > index b59c447..b730bb2 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -60,6 +60,20 @@ AC_ARG_WITH(udevrulesdir,
> > > )
> > > AC_SUBST(UDEVRULESDIR)
> > >
> > > +AC_ARG_ENABLE(gstreamer,
> > > + AS_HELP_STRING([--enable-gstreamer=@<:@auto/yes/no@:>@],
> > > + [Enable GStreamer support]),,
> > > + [enable_gstreamer="no"])
> > > +if test "$enable_gstreamer" != "no"; then
> > > + PKG_CHECK_MODULES(GST, [gstreamer-1.0 gstreamer-app-1.0], [enable_gstreamer=yes],
> > > + [if test "$enable_gstreamer" = "yes"; then
> > > + AC_MSG_ERROR([Gstreamer libs are missing])
> > > + fi
> > > + enable_gstreamer=no
> > > + ])
> > > +fi
> > > +AM_CONDITIONAL([HAVE_GST],[test "$enable_gstreamer" = "yes"])
> > > +
> > > dnl ===========================================================================
> > > dnl check compiler flags
> > >
> > > @@ -129,6 +143,7 @@ AC_MSG_NOTICE([
> > > prefix: ${prefix}
> > > C compiler: ${CC}
> > > C++ compiler: ${CXX}
> > > + Gstreamer plugin: ${enable_gstreamer}
> > >
> > > Now type 'make' to build $PACKAGE
> > > ])
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index 18ed22c..56bae70 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -9,6 +9,9 @@ if ENABLE_TESTS
> > > SUBDIRS = . unittests
> > > endif
> > >
> > > +plugin_LTLIBRARIES =
> > > +plugindir = $(pkglibdir)/plugins
> > > +
> > > AM_CPPFLAGS = \
> > > -DSPICE_STREAMING_AGENT_PROGRAM \
> > > -I$(top_srcdir)/include \
> > > @@ -61,3 +64,27 @@ spice_streaming_agent_SOURCES = \
> > > stream-port.cpp \
> > > stream-port.hpp \
> > > $(NULL)
> > > +
> > > +if HAVE_GST
> > > +plugin_LTLIBRARIES += gst-plugin.la
> > > +
> > > +gst_plugin_la_LDFLAGS = \
> > > + -module -avoid-version \
> > > + $(RELRO_LDFLAGS) \
> > > + $(NO_INDIRECT_LDFLAGS) \
> > > + $(NULL)
> > > +
> > > +gst_plugin_la_LIBADD = \
> > > + $(GST_LIBS) \
> > > + $(NULL)
> > > +
> > > +gst_plugin_la_SOURCES = \
> > > + gst-plugin.cpp \
> > > + $(NULL)
> > > +
> > > +gst_plugin_la_CPPFLAGS = \
> > > + -I$(top_srcdir)/include \
> > > + $(SPICE_PROTOCOL_CFLAGS) \
> > > + $(GST_CFLAGS) \
> > > + $(NULL)
> > > +endif
> > > diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> > > new file mode 100644
> > > index 0000000..435f148
> > > --- /dev/null
> > > +++ b/src/gst-plugin.cpp
> > > @@ -0,0 +1,430 @@
> > > +/* Plugin implementation for gstreamer encoder
> > > + *
> > > + * \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#include <config.h>
> > > +#include <cstring>
> > > +#include <exception>
> > > +#include <stdexcept>
> > > +#include <sstream>
> > > +#include <memory>
> > > +#include <syslog.h>
> > > +#include <unistd.h>
> > > +#include <gst/gst.h>
> > > +#include <gst/app/gstappsink.h>
> > > +
> > > +#define XLIB_CAPTURE 1
> > > +#if XLIB_CAPTURE
> > > +#include <X11/Xlib.h>
> > > +#include <gst/app/gstappsrc.h>
> > > +#endif
> > > +
> > > +#include <spice-streaming-agent/plugin.hpp>
> > > +#include <spice-streaming-agent/frame-capture.hpp>
> > > +
> > > +namespace spice {
> > > +namespace streaming_agent {
> > > +namespace gstreamer_plugin {
> > > +
> > > +struct GstreamerEncoderSettings
> > > +{
> > > + int fps;
> > > + SpiceVideoCodecType codec;
> > > + char encoder[32];
> >
> > You could just use std::string here...
> >
> > > +};
> > > +
> > > +class GstreamerFrameCapture final: public FrameCapture
> >
> > Style: space before ":" I think...
> >
> > > +{
> > > +public:
> > > + GstreamerFrameCapture(const GstreamerEncoderSettings &settings);
> > > + GstreamerFrameCapture() = delete;
> >
> > You don't have to delete the default constructor, if you declare any
> > constructors yourself, the compiler will not generate the default one.
>
> That's really weird. I'm really remember i added this since i was
> getting an error
> here, but now it seems to work :p.
>
> > > + ~GstreamerFrameCapture();
> > > + FrameInfo CaptureFrame() override;
> > > + void Reset() override;
> > > + SpiceVideoCodecType VideoCodecType() const override {
> > > + return settings.codec;
> > > + }
> > > +private:
> > > + void free_sample();
> > > + GstElement *get_encoder_plugin(const GstreamerEncoderSettings& settings);
> > > + GstElement *get_capture_plugin(const GstreamerEncoderSettings& settings);
> > > + void pipeline_init(const GstreamerEncoderSettings& settings);
> > > +#if XLIB_CAPTURE
> > > + void xlib_capture();
> > > + Display *dpy;
> > > + XImage *image = nullptr;
> > > +#endif
> > > + GstElement *pipeline, *capture, *sink, *encoder, *convert;
> > > + GstSample *sample = nullptr;
> > > + GstCaps *sink_caps;
> > > + GstMapInfo map = {};
> > > + uint32_t last_width = ~0u, last_height = ~0u;
> > > + uint32_t cur_width = 0, cur_height = 0;
> > > + bool is_first = true;
> > > + GstreamerEncoderSettings settings; // will be set by plugin settings
> > > +};
> > > +
> > > +class GstreamerPlugin final: public Plugin
> > > +{
> > > +public:
> > > + FrameCapture *CreateCapture() override;
> > > + unsigned Rank() override;
> > > + void ParseOptions(const ConfigureOption *options);
> > > + SpiceVideoCodecType VideoCodecType() const override {
> > > + return settings.codec;
> > > + }
> > > +private:
> > > + GstreamerEncoderSettings settings = { .fps = 25, .codec = SPICE_VIDEO_CODEC_TYPE_H264, .encoder = {0} };
> > > +};
> > > +}}} //namespacespice::streaming_agent::gstreamer_plugin
> > > +
> > > +using namespacespice::streaming_agent;
> > > +using namespacespice::streaming_agent::gstreamer_plugin;
> >
> > Ending the namespace scope and adding a "using" clause has the same
> > effect here and only really adds noise. The common practice to enclose
> > everything in the namespace scope.
> >
> > > +
> > > +GstElement * GstreamerFrameCapture::get_capture_plugin(const GstreamerEncoderSettings& settings)
> >
> > Style: I belive we don't put a space after "*", you are inconsistent in
> > it too.
> >
> > > +{
> > > +GstElement *capture = NULL;
> >
> > Missing indent here. Also, use nullptr consistently (more NULLs below)?
> >
> > > +
> > > +#if XLIB_CAPTURE
> > > + capture = gst_element_factory_make("appsrc", "capture");
> > > +#else
> > > + capture = gst_element_factory_make("ximagesrc", "capture");
> > > + g_object_set(capture,
> > > + "use-damage", 0,
> > > + NULL);
> > > +
> > > +#endif
> >
> > Scattering #ifdefs across the code is really not clean in C++. The way
> > this is conventionally supposed to be solved is extension by
> > composition, which means you implement an interface (i.e. a virtual
> > class in C++) for the xlib capture functionality and then implement it
> > in a class.
> >
> > But in this case I'd be more inclined to use template specialization
> > for a compile-time conditional use of the code (apart from looking a
> > bit better here is saves the virtual function table overhead). A short
> > example:
> >
> > template<int T>
> > class XlibCapture
> > {
> > public:
> > void capture() {}
> > };
> >
> > template<>
> > class XlibCapture<1>
> > {
> > public:
> > void capture()
> > {
> > // do the Xlib capture
> > }
> > };
> >
> > // ...
> >
> > class GstreamerFrameCapture {
> > // ...
> > private:
> > XlibCapture<XLIB_CAPTURE> xlib_capture;
> > };
> >
> > Apart from the capture() method from the example you should use
> > constructor/destructor for initialization/cleanup and obviously you can
> > add more methods.
> >
> > For the one "#if !XLIB_CAPTURE" check, I belive that is kind-of special
> > and could probably be done as a runtime check, looks like it may not
> > always be Xlib specific anyway?
> >
> > I know there might be a few more issues to solve with this, so if you
> > go for it and get stuck, feel free to ping me.
>
> It would make sense, but actually the reason of having the xlib capture
> is mostly that ximagesrc still unable to handle runtime resolution change
> (And i still wonder if it may be possible to workaround it somehow by
> sending eos and restart the stream or similar ).
> So hopefully this part of the code will be dropped :p
>
> > > + return capture;
> > > +}
> > > +
> > > +GstElement * GstreamerFrameCapture::get_encoder_plugin(const GstreamerEncoderSettings& settings)
> > > +{
> > > + GList *encoders;
> > > + GList *filtered;
> > > + GstElement *encoder;
> > > + GstElementFactory *factory = NULL;
> > > + std::stringstream caps_ss;
> > > +
> > > + switch (settings.codec) {
> > > + case SPICE_VIDEO_CODEC_TYPE_H264:
> > > + caps_ss << "video/x-h264" << ",stream-format=(string)byte-stream";
> >
> > Nit: you can use one static string instead of two and save the
> > operator.
>
> It was a static string, but found it is more convenience to add numbers
> if needed (e.g. fps) .
>
> > > + break;
> > > + case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> > > + caps_ss << "image/jpeg";
> > > + break;
> > > + case SPICE_VIDEO_CODEC_TYPE_VP8:
> > > + caps_ss << "video/x-vp8";
> > > + break;
> > > + case SPICE_VIDEO_CODEC_TYPE_VP9:
> > > + caps_ss << "video/x-vp9";
> > > + break;
> > > + default :
> > > + syslog(LOG_ERR, "Invalid codec\n");
> > > + return NULL;
> > > + break;
> >
> > Looks like you should be throwing an exception here? You could also
> > include the codec type in the error message. And some context would be
> > useful for the user troubleshooting this (i.e. that this error comes
> > from the gstreamer plugin), but I'm thinking this problem is more
> > universal and we should look for a generic solution across the
> > streaming agent...
> >
> > > + }
> > > + if (!caps_ss.str().empty()) {
> > > + caps_ss << ",framerate=" << settings.fps << "/1";
> > > + }
> >
> > As Uri mentioned, caps_ss can't be empty here.
>
> True, a stub.
> > > +
> > > + encoders = gst_element_factory_list_get_elements(GST_ELEMENT_FACTORY_TYPE_VIDEO_ENCODER, GST_RANK_NONE);
> > > + sink_caps = gst_caps_from_string(caps_ss.str().c_str());
> > > + filtered = gst_element_factory_list_filter(encoders, sink_caps, GST_PAD_SRC, false);
> > > + if (filtered) {
> > > + printf("Available encoders:\n");
> >
> > The printf() calls should be syslog()s?
>
> No, Should be printed to user. (but should i use cout maybe? or
> something else?)
What is the intention with the messages then? They look like debug
messages to me. I still think it should be syslog(), if you run the
agent manually from the terminal, syslog does print to stderr. And I
think stderr is preferable to stdout.
Yeah, otherwise it's probably better to use std::cout/cerr.
> > > +
> > > + for (GList *l = filtered; l != NULL; l = l->next) {
> > > + if (!factory && settings.encoder[0] && !g_strcmp0(settings.encoder, GST_ELEMENT_NAME(l->data))) {
> > > + factory = (GstElementFactory*)l->data;
> > > + }
> > > + printf("\t%s\n", GST_ELEMENT_NAME(l->data));
> > > + }
> > > + if (!factory && settings.encoder[0]) {
> > > + printf("\t(Specified encoder '%s' was not found for the requested codec)\n", settings.encoder);
> > > + }
> > > + factory = factory ? factory : (GstElementFactory*)filtered->data;
> > > + printf("\n*** '%s' encoder plugin is used ***\n", GST_ELEMENT_NAME(factory));
> > > +
> > > + } else {
> > > + syslog(LOG_ERR, "No suitable encoder was found\n");
> >
> > Again, if it's an error, throw an exception.
> >
> > > + }
> > > + gst_plugin_feature_list_free(filtered);
> > > + gst_plugin_feature_list_free(encoders);
> > > +
> > > + encoder = factory ? gst_element_factory_create(factory, "encoder") : NULL;
> >
> > Not sure you can rely on factory being != NULL if you throw instead of
> > the syslog above.
> >
> > I'd check for encoder == NULL here and throw an error straight away.
> > And in general you can throw straight away and not use NULL pointers as
> > an error state ever in C++...
>
> Ok, actually now looking I'm not sure it could be null.. will do
> differently.
>
> > > + if (encoder) { // Invalid properties will be ignored silently
> > > + /* x264enc properties */
> > > + gst_util_set_object_arg(G_OBJECT(encoder), "tune", "zerolatency");// stillimage, fastdecode, zerolatency
> > > + gst_util_set_object_arg(G_OBJECT(encoder), "bframes", "0");
> > > + gst_util_set_object_arg(G_OBJECT(encoder), "speed-preset", "1");// 1-ultrafast, 6-med, 9-veryslow
> > > + }
> > > + return encoder;
> > > +}
> > > +
> > > +void GstreamerFrameCapture::pipeline_init(const GstreamerEncoderSettings& settings)
> > > +{
> > > + GstElement *encoder, *convert;
> > > + GstCaps *caps;
> > > + gboolean link;
> > > +
> > > + pipeline = gst_pipeline_new("pipeline");
> > > + capture = get_capture_plugin(settings);
> > > + convert = gst_element_factory_make("videoconvert", "convert");
> > > + encoder = get_encoder_plugin(settings);
> > > + sink = gst_element_factory_make("appsink", "sink");
> > > + if (!capture || !convert || !encoder || !sink) {
> > > + gst_object_unref (GST_OBJECT (pipeline));
> > > + gst_object_unref (GST_OBJECT (capture));
> > > + gst_object_unref (GST_OBJECT (convert));
> > > + gst_object_unref (GST_OBJECT (encoder));
> > > + gst_object_unref (GST_OBJECT (sink));
> > > + //TODO: check elements existence at build time (and\or improve error in runtime)
> > > + throw std::runtime_error("One or more gstreamer element cannot be created");
> >
> > As I said, throw at the spot of creating each of the pointers if it is
> > NULL, it'll be a more useful error message too.
>
> Yes, was mentioned before, missed it.
>
> > We should also be using the error hierarchy since we got it in place. I
> > think we should probably create a PluginError : public Error exception
> > and put it in a public header (though that's not necessary for this
> > plugin).
> >
> > Also, does gstreamer have any indication mechanism to specify what was
> > the actual problem?
>
> Debugging the application with gstreamer tools will shed more light on
> the problem, not sure it can be extracted somehow and to be added to our
> error msgs.
It might be completely off but I found this:
https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstGError.html
> > > + }
> > > +
> > > + g_object_set(sink,
> > > + "sync", FALSE,
> > > + "drop", TRUE,
> > > + "max-buffers", 1,
> > > + NULL);
> > > +
> > > + gst_bin_add_many(GST_BIN(pipeline), capture, convert, encoder, sink, NULL);
> > > +
> > > + caps = gst_caps_from_string("video/x-raw,format=(string)I420");
> > > + link = gst_element_link(capture, convert) &&
> > > + gst_element_link_filtered(convert, encoder, caps) &&
> > > + gst_element_link_filtered(encoder, sink, sink_caps);
> > > + gst_caps_unref(caps);
> > > + if (!link) {
> > > + throw std::runtime_error("Link failed");
> >
> > This error message is useless, please improve it.
> >
> > > + }
> > > +
> > > +#if XLIB_CAPTURE
> > > + dpy = XOpenDisplay(NULL);
> > > + if (!dpy) {
> > > + throw std::runtime_error("Unable to initialize X11");
> > > + }
> > > +#endif
> > > +
> > > + gst_element_set_state(pipeline, GST_STATE_PLAYING);
> > > +
> > > +#if !XLIB_CAPTURE
> > > + /* Some encoders cannot handle odd resolution make sure it's even number of pixels */
> > > + int sx, sy, ex, ey;
> > > + g_object_get(capture,
> > > + "endx", &ex,
> > > + "endy", &ey,
> > > + "startx", &sx,
> > > + "starty", &sy,
> > > + NULL);
> > > + cur_width = ex - sx;
> > > + cur_height = ey - sy;
> > > + if (cur_width < 16 || cur_height < 16) {
> > > + throw std::runtime_error("Invalid screen size");
> >
> > So IIUC this code is checking if the frame is smaller than 16x16
> > pixels, which is a check on top of what's mentioned in the comment
> > above, which I find confusing.
>
> It's in addition to the other check which is few rows after.
Yes :) what I wanted to say is that the comment above it: "Some
encoders cannot handle ..." talks about a different check. If I read
that and continue reading the code, I'm awaiting to see the check the
comment mentions, but find a completely different check first. Might
just be me, but I find it mind-boggling. I go re-reading the comment to
make sure what is says, then re-read the check to make sure what it
does, ... :)
Cheers,
Lukas
> > You could also say "smaller than 16x16" in the error message.
> > > + }
> > > + g_object_set(capture,
> > > + "endx", cur_width - !(cur_width%2),
> > > + "endy", cur_height - !(cur_height%2),
> >
> > Style: I'd prefer spaces around "%", is there a reason to have them
> > around "-" and not around "%"?
> >
> > > + "startx", 0,
> > > + "starty", 0,
> > > + NULL);
> > > +#endif
> > > +}
> > > +
> > > +GstreamerFrameCapture::GstreamerFrameCapture(const GstreamerEncoderSettings& settings):
> > > + settings(settings)
> > > +{
> > > + try {
> > > + pipeline_init(settings);
> >
> > The pipeline_init() method code really can be in the constructor. The
> > only advantage of the method is one level of indent... But still I'd
> > just put the code here.
> >
> > > + } catch (const std::exception &e) {
> > > + if (pipeline)
> > > + gst_object_unref(pipeline);
> > > + if (sink_caps)
> > > + gst_caps_unref(sink_caps);
> > > +
> > > + throw std::runtime_error(e.what());
> >
> > You can rethrow the original exception by a simple "throw;" (you can
> > also do "throw e;" but that actually creates a copy of the exception.
> > Not that it matters much here.)
> >
> > > + }
> > > +}
> > > +
> > > +void GstreamerFrameCapture::free_sample()
> > > +{
> > > + if (sample) {
> > > + gst_buffer_unmap(gst_sample_get_buffer(sample), &map);
> > > + gst_sample_unref(sample);
> > > + sample = nullptr;
> > > + }
> > > +#if XLIB_CAPTURE
> > > + if(image) {
> > > + image->f.destroy_image(image);
> > > + image = nullptr;
> > > + }
> > > +#endif
> > > +}
> >
> > It would be nice to have some sort of RAII for this (i.e. wrap in a
> > class, create in constructor and cleanup in destructor). Something like
> > that would be probably needed if you go with the XlibCapture wrapper.
> >
> > Not sure how the image works though. It seems to be local to the
> > xlib_capture() method, or is it somehow referenced from the sample
> > itself?
> >
> > > +
> > > +GstreamerFrameCapture::~GstreamerFrameCapture()
> > > +{
> > > + free_sample();
> > > + gst_element_set_state(pipeline, GST_STATE_NULL);
> > > + gst_object_unref(pipeline);
> > > + gst_caps_unref(sink_caps);
> > > +#if XLIB_CAPTURE
> > > + XCloseDisplay(dpy);
> > > +#endif
> > > +}
> > > +
> > > +void GstreamerFrameCapture::Reset()
> > > +{
> > > + //TODO
> > > +}
> > > +
> > > +#if XLIB_CAPTURE
> > > +void GstreamerFrameCapture::xlib_capture()
> > > +{
> > > + int screen = XDefaultScreen(dpy);
> > > +
> > > + Window win = RootWindow(dpy, screen);
> > > + XWindowAttributes win_info;
> > > + XGetWindowAttributes(dpy, win, &win_info);
> > > +
> > > + /* Some encoders cannot handle odd resolution */
> > > + cur_width = win_info.width - win_info.width%2;
> > > + cur_height = win_info.height - win_info.height%2;
> > > +
> > > + if (cur_width != last_width || cur_height != last_height) {
> > > + last_width = cur_width;
> > > + last_height = cur_height;
> > > + is_first = true;
> > > +
> > > + gst_app_src_end_of_stream (GST_APP_SRC (capture));
> > > + gst_element_set_state(pipeline, GST_STATE_NULL);//maybe ximagesrc needs eos as well
> >
> > A stray tab on this line.
> >
> > > + gst_element_set_state(pipeline, GST_STATE_PLAYING);
> > > + }
> > > +
> > > + // TODO handle errors
> > > + image = XGetImage(dpy, win, 0, 0,
> > > + cur_width, cur_height, AllPlanes, ZPixmap);
> > > + if (!image)
> > > + throw std::runtime_error("Cannot capture from X");
> >
> > Missing curly brackets, once more 3 lines down the code.
> >
> > > + GstBuffer *buf;
> > > + buf = gst_buffer_new_wrapped(image->data, image->height * image->bytes_per_line);
> > > + if (!buf)
> > > + throw std::runtime_error("Cannot wrap image");
> >
> > This message is also quite lacking :)
> >
> > > + GstSample *appsrc_sample;
> > > + GstCaps *caps;
> > > + std::stringstream ss;
> > > +
> > > + ss << "video/x-raw,format=BGRx,width=" << image->width << ",height=" << image->height << ",framerate=" << settings.fps << "/1";
> > > + caps = gst_caps_from_string(ss.str().c_str());
> > > +
> > > + // Push sample
> > > + appsrc_sample = gst_sample_new(buf, caps, NULL, NULL);
> > > + if (gst_app_src_push_sample (GST_APP_SRC (capture), appsrc_sample) != GST_FLOW_OK) {
> > > + throw std::runtime_error("appsrc cannot push sample");
> >
> > :)
> >
> > > + }
> > > + gst_sample_unref(appsrc_sample);
> > > +}
> > > +#endif
> > > +
> > > +FrameInfo GstreamerFrameCapture::CaptureFrame()
> > > +{
> > > + FrameInfo info;
> > > +
> > > + free_sample(); // free prev if exist
> > > +
> > > +#if XLIB_CAPTURE
> > > + xlib_capture();
> > > +#endif
> > > + info.size.width = cur_width;
> > > + info.size.height = cur_height;
> > > + info.stream_start = is_first;
> > > + if (is_first) {
> > > + is_first = false;
> > > + }
> > > +
> > > + // Pull sample
> > > + sample = gst_app_sink_pull_sample(GST_APP_SINK(sink)); // blocking
> > > +
> > > + if (sample) { //map after pipeline
> > > + if (!gst_buffer_map(gst_sample_get_buffer(sample), &map, GST_MAP_READ)) {
> > > + free_sample();
> > > + throw std::runtime_error("Buffer mapping failed");
> > > + }
> > > +
> > > + info.buffer = map.data;
> > > + info.buffer_size = map.size;
> > > + } else {
> > > + syslog(LOG_ERR, "No sample- EOS or state change\n");
> >
> > Throw an exception? Also the message :)
> >
> > > + }
> > > +
> > > + return info;
> > > +}
> > > +
> > > +FrameCapture *GstreamerPlugin::CreateCapture()
> > > +{
> > > + return new GstreamerFrameCapture(settings);
> > > +}
> > > +
> > > +unsigned GstreamerPlugin::Rank()
> > > +{
> > > + return SoftwareMin;
> > > +}
> > > +
> > > +void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
> > > +{
> > > + for (; options->name; ++options) {
> > > + const std::string name = options->name;
> > > + const std::string value = options->value;
> > > +
> > > + if (name == "framerate") {
> > > + try {
> > > + settings.fps = std::stoi(value);
> > > + } catch (const std::exception &e) {
> > > + throw std::runtime_error("Invalid value '" + value + "' for option 'framerate'.");
> > > + }
> > > + } else if (name == "gst.codec") {
> > > + try {
> > > + 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 {
> > > + throw std::runtime_error("Invalid value '" + value + "' for option 'gst.codec'.");
> > > + }
> > > + } catch (const std::exception &e) {
> > > + throw std::runtime_error(e.what());
> > > + }
> >
> > No need to catch and rethrow here, you can drop the try-catch block.
> >
> > > + } else if (name == "gst.encoder") {
> > > + try {
> > > + if (value.length() < 3 || value.length() > 31) {
> > > + syslog(LOG_WARNING, "Encoder name length is invalid, will be ignored");
> >
> > I think this should be an exception too? Though if you use std::string
> > for the settings.encoder, you can drop this altogether.
> >
> > > + } else {
> > > + value.copy(settings.encoder, value.length());
> > > + }
> > > + } catch (const std::exception &e) {
> > > + throw std::runtime_error("Invalid value '" + value + "' for option 'gst.encoder'.");
> > > + }
> >
> > There is no exception being thrown inside the block that would mean the
> > value is invalid, drop the try-catch block...
> >
> > Cheers,
> > Lukas
> >
> > > +
> > > + }
> > > + }
> > > +}
> > > +
> > > +SPICE_STREAMING_AGENT_PLUGIN(agent)
> > > +{
> > > + gst_init(NULL, NULL);
> > > +
> > > + std::unique_ptr<GstreamerPlugin> plugin(new GstreamerPlugin());
> > > +
> > > + plugin->ParseOptions(agent->Options());
> > > +
> > > + agent->Register(*plugin.release());
> > > +
> > > + return true;
> > > +}
>
>
> Thanks, Snir.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel