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.
> + ~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} };
> +};
> +}}} //namespace spice::streaming_agent::gstreamer_plugin
> +
> +using namespace spice::streaming_agent;
> +using namespace spice::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.
> + 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.
> + 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.
> +
> + 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?
> +
> + 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++...
> + 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.
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?
> + }
> +
> + 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.
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;
> +}
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel