[Spice-devel] [PATCH v2 spice-streaming-agent 1/1] Adding gstreamer based plugin
Uri Lublin
uril at redhat.com
Sun Jun 17 15:12:29 UTC 2018
Hi Snir,
Thanks for working on this plugin.
See some comments below.
On 06/10/2018 11:14 AM, 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 +++++++++++++++++++++++++++++++++++++++++++++
Perhaps it would be better to have a src/plugins directory that contain
in-project plugins source files.
> 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];
> +};
> +
> +class GstreamerFrameCapture final: public FrameCapture
> +{
> +public:
> + GstreamerFrameCapture(const GstreamerEncoderSettings &settings);
> + GstreamerFrameCapture() = delete;
> + ~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;
> +
> +GstElement * GstreamerFrameCapture::get_capture_plugin(const GstreamerEncoderSettings& settings)
> +{
> +GstElement *capture = NULL;
> +
> +#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
> + 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";
> + 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;
> + }
> + if (!caps_ss.str().empty()) {
nit: I think caps_ss.str() is never empty here.
> + caps_ss << ",framerate=" << settings.fps << "/1";
> + }
> +
> + 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");
> +
> + 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");
> + }
> + gst_plugin_feature_list_free(filtered);
> + gst_plugin_feature_list_free(encoders);
Probably safer to free those lists only after using factory (creating
encoder).
> +
> + encoder = factory ? gst_element_factory_create(factory, "encoder") : NULL;
> + 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) {
You may want to check pipeline as well.
> + 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));
1. What happens when calling gst_object_unref( GST_OBJECT (NULL)) ?
One of them for sure will be.
2. Here you unref them all but below you do not unref e.g. convert.
3. Better set pipeline=NULL after unref, so it will not be unref
again in the constructor.
> + //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");
> + }
> +
> + 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");
Probably better to add format=I420 to the encoder filter (sinkpad ?) above.
> + 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) {
Here nothing is unrefed (but pipeline will be by the constructor)
> + throw std::runtime_error("Link failed");
> + }
> +
> +#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");
> + }
> + g_object_set(capture,
> + "endx", cur_width - !(cur_width%2),
> + "endy", cur_height - !(cur_height%2),
That's a bit weird code, better add a comment explaining why
it's needed.
> + "startx", 0,
> + "starty", 0,
> + NULL);
> +#endif
> +}
> +
> +GstreamerFrameCapture::GstreamerFrameCapture(const GstreamerEncoderSettings& settings):
> + settings(settings)
> +{
> + try {
> + pipeline_init(settings);
> + } 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());
> + }
> +}
> +
> +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
> +}
> +
> +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;
Please add another line explaining why here the calculation
is different than above (here X-X%2 above X-!(X%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
> + 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");
> +
> + GstBuffer *buf;
> + buf = gst_buffer_new_wrapped(image->data, image->height * image->bytes_per_line);
> + if (!buf)
> + throw std::runtime_error("Cannot wrap image");
> +
> + 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");
Better throw an exception here.
There is garbage in info.buffer and info.buffer_size.
> + }
> +
> + 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());
Maybe simpler to remove the try-catch block
and throw directly from the switch.
Uri.
> + }
> + } else if (name == "gst.encoder") {
> + try {
> + if (value.length() < 3 || value.length() > 31) {
> + syslog(LOG_WARNING, "Encoder name length is invalid, will be ignored");
> + } else {
> + value.copy(settings.encoder, value.length());
> + }
> + } catch (const std::exception &e) {
> + throw std::runtime_error("Invalid value '" + value + "' for option 'gst.encoder'.");
> + }
> +
> + }
> + }
> +}
> +
> +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;
> +}
>
More information about the Spice-devel
mailing list