Hi, On 06/20/2018 11:39 AM, Lukáš Hrázký wrote:
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 FrameCaptureStyle: 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); + +#endifScattering #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.
Assuming that using this plugin is currently only for development and testing, the intention was to let the user\developer\tester to know which codec has been picked and print a list of other available codecs. (assuming whenever it runs this information is desirable)
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
thanks
+ } + + 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, ... :)
ohh, i see now, so maybe moving the comment to a better location will work :)
Snir.
Cheers, LukasYou 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 wellA 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