[Spice-devel] [PATCH spice-streaming-agent 1/1] Adding gstreamer based plugin
Christophe de Dinechin
cdupontd at redhat.com
Fri Feb 23 12:14:47 UTC 2018
Cool!
> On 22 Feb 2018, at 17:20, Snir Sheriber <ssheribe at redhat.com> wrote:
>
> Gstreamer based plugin utilizing gstreamer elements to capture
> screen from X, convert and encode into h264 stream using x264enc
> gstreamer plugin.
> Configure with --with-gst, will be built as a separate plugin.
>
> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> configure.ac | 8 ++
> src/Makefile.am | 26 +++++++
> src/gst-plugin.cpp | 222 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 256 insertions(+)
> create mode 100644 src/gst-plugin.cpp
>
> diff --git a/configure.ac b/configure.ac
> index 5aab662..ee2ef68 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -52,6 +52,13 @@ AC_SUBST(JPEG_LIBS)
>
> AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find Catch dependency header (catch/catch.hpp)])])
>
> +AC_ARG_WITH([gst], AS_HELP_STRING([--with-gst], [Build with the gstreamer plugin]))
> +have_gst=no
> +if test "x$with_gst" = "xyes"; then
> + PKG_CHECK_MODULES(GST, [gstreamer-1.0 gstreamer-app-1.0], [have_gst=yes], [have_gst=no])
> +fi
> +AM_CONDITIONAL([HAVE_GST],[test "$have_gst" = "yes"])
> +
> dnl ===========================================================================
> dnl check compiler flags
>
> @@ -102,6 +109,7 @@ AC_MSG_NOTICE([
> prefix: ${prefix}
> C compiler: ${CC}
> C++ compiler: ${CXX}
> + Gstreamer plugin: ${have_gst}
>
> Now type 'make' to build $PACKAGE
> ])
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 3717b5c..c09a2d7 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -5,6 +5,8 @@
>
> NULL =
> SUBDIRS = . unittests
> +plugin_LTLIBRARIES =
> +plugindir = $(pkglibdir)/plugins
>
> AM_CPPFLAGS = \
> -DSPICE_STREAMING_AGENT_PROGRAM \
> @@ -56,3 +58,27 @@ spice_streaming_agent_SOURCES = \
> jpeg.cpp \
> jpeg.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..2d170a0
> --- /dev/null
> +++ b/src/gst-plugin.cpp
> @@ -0,0 +1,222 @@
> +#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>
> +
> +#include <spice-streaming-agent/plugin.hpp>
> +#include <spice-streaming-agent/frame-capture.hpp>
> +
> +using namespace spice::streaming_agent;
> +
> +namespace {
Why anonymous namespace and not namespace spice::streaming_agent? Or better yet, spice::streaming_agent::gstreamer_plugin?
> +struct GstSettings
> +{
> + int fps;
> + int encode_speed;
> +};
> +
> +class GstFrameCapture final: public FrameCapture
> +{
> +public:
> + GstFrameCapture(const GstSettings &settings);
> + ~GstFrameCapture();
> + FrameInfo CaptureFrame() override;
> + void Reset() override;
> + SpiceVideoCodecType VideoCodecType() const override {
> + return SPICE_VIDEO_CODEC_TYPE_H264;
> + }
> +private:
> + void free_buffer();
> +
> + GstElement *pipeline, *capture, *sink;
> + GstSettings settings;
> + bool is_first = true;
> + int w, h;
For both readability and alignment reasons, I’d put all the Gst things together, and then int w,h then bool is_first.
> + GstSample *sample = nullptr;
> + GstBuffer *buffer = nullptr;
> + GstMapInfo map = {};
Any reason you initialize some fields and not others?
> +};
> +
> +class GstreamerPlugin final: public Plugin
> +{
> +public:
> + FrameCapture *CreateCapture() override;
> + unsigned Rank() override;
> + void ParseOptions(const ConfigureOption *options);
> + SpiceVideoCodecType VideoCodecType() const override {
> + return SPICE_VIDEO_CODEC_TYPE_H264;
> + }
> +private:
> + GstSettings settings = { 25, 1 };
Please use attributes here for readability.
> +};
> +}
> +
> +GstFrameCapture::GstFrameCapture(const GstSettings& settings):
> + settings(settings)
> +{
> + GstElement *encoder, *convert;
> + GstCaps *caps;
> +
> + pipeline = gst_pipeline_new("pipeline");
> + capture = gst_element_factory_make("ximagesrc", "capture");
> + convert = gst_element_factory_make("videoconvert", "convert");
> + encoder = gst_element_factory_make("x264enc", "encoder"); //TODO: move to use encodebin and profiles - much more generic
For testing purpose, what about a simple getenv, with default to “x264enc”?
Why not make it part of GstSettings?
> + sink = gst_element_factory_make("appsink", "sink");
> + if (!capture || !convert || !encoder || !sink) {
> + //TODO: check elements existence in build time (and\or improve error in runtime)
- “at” build time?
- How would you check at build time?
- not freeing the resources you allocated? (Note that you are in ctor, can’t rely on dtor)
> + throw std::runtime_error("One or more gstreamr element cannot be created”);
…
> + }
> +
> + syslog(LOG_INFO, "fps = %d\n", settings.fps);
> + std::string str = "video/x-h264,stream-format=(string)byte-stream,framerate=" + std::to_string(settings.fps) + "/1”;
Wondering how this will generalize to other encoders… Good enough for a first pass.
> + caps = gst_caps_from_string(str.c_str());
> + g_object_set(sink,
> + "caps", caps,
> + "sync", FALSE,
> + "drop", TRUE,
> + "max-buffers", 1,
> + NULL);
> + gst_caps_unref(caps);
> +
> + g_object_set(capture,
> + "use-damage", 0,
> + NULL);
> +
> + gst_util_set_object_arg(G_OBJECT(encoder), "tune", "zerolatency");//stillimage,fastdecode,zerolatency
> + g_object_set(encoder,
> + "bframes", 0,
> + "speed-preset", 1, //1-ultrafast,6-med, 9-veryslow
> + NULL);
> +
> + gst_bin_add_many(GST_BIN(pipeline), capture, convert, encoder, sink, NULL);
> +
> + caps = gst_caps_from_string("video/x-raw,format=(string)I420");
> + if (gst_element_link(capture, convert) &&
> + gst_element_link_filtered(convert, encoder, caps) &&
> + gst_element_link(encoder, sink)) {
> + syslog(LOG_INFO, "Elements were linked successfully\n");
> + } else {
> + throw std::runtime_error("Link failed”);
…
> + }
> +
> + gst_caps_unref(caps);
> + gst_element_set_state(pipeline, GST_STATE_PLAYING);//TODO: Not sure playing state is ideal for this case (timing wise)
> +}
> +
> +void GstFrameCapture::free_buffer()
> +{
> + if (buffer) {
> + gst_buffer_unmap(buffer, &map);
> + // don't unref buffer, will be unref when sample is unref
That comment seems a bit strange given the if(sample) below…
> + buffer = nullptr;
> + }
> + if (sample) {
> + gst_sample_unref(sample);
> + sample = nullptr;
> + }
> +}
> +
> +GstFrameCapture::~GstFrameCapture()
> +{
> + free_buffer();
This suggest you may want a Buffer subclass encapsulating gst_sample_get_buffer, map, unmap, etc. (follow-up).
> + gst_element_set_state(pipeline, GST_STATE_NULL);
> + gst_object_unref(pipeline);
> +}
> +
> +void GstFrameCapture::Reset()
> +{
> + //TODO
> +}
> +
> +FrameInfo GstFrameCapture::CaptureFrame()
> +{
> + free_buffer();
> +
> + sample = gst_app_sink_pull_sample(GST_APP_SINK(sink));//block, timeout needed?
> +
> + FrameInfo info{};
> + if (sample) {
> + buffer = gst_sample_get_buffer(sample);
> + gst_buffer_map(buffer, &map, GST_MAP_READ);
> +
> + info.stream_start = is_first;
> + if (is_first) {
> + int sx,sy,ex,ey;
> + g_object_get(capture,
> + "endx", &ex,
> + "endy", &ey,
> + "startx", &sx ,
> + "starty", &sy,
> + NULL);
> + w = ex - sx;
> + h = ey - sy;
> + syslog(LOG_INFO, "%d x %d \n", w, h);
> + if ( h <= 0 || w <= 0 ) { // <=16?
> + throw std::runtime_error("Invalid stream size");
> + }
> + is_first = false;
> + }
> + info.size.width = w;
> + info.size.height = h;
> + info.buffer = map.data;
> + info.buffer_size = map.size;
> + } else {
> + syslog(LOG_ERR, "No sample- EOS or state change\n");
> + }
> +
> + return info;
> +}
> +
> +FrameCapture *GstreamerPlugin::CreateCapture()
> +{
> + return new GstFrameCapture(settings);
> +}
> +
> +unsigned GstreamerPlugin::Rank()
> +{
> + return SoftwareMin;
> +}
> +
> +void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
> +{
> +#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> +
> + for (; options->name; ++options) {
> + const char *name = options->name;
> + const char *value = options->value;
> +
> + if (strcmp(name, "framerate") == 0) {
> + int val = atoi(value);
> + if (val > 0) {
> + settings.fps = val;
> + }
> + else {
> + arg_error("wrong framerate arg %s\n", value);
> + }
> + }
> + }
> +}
> +
> +__attribute__ ((visibility ("default")))
> +bool spice_streaming_agent_plugin_init(Agent* agent)
> +{
> + if (agent->Version() != PluginVersion) {
> + return false;
> + }
> +
> + gst_init(NULL, NULL);
> +
> + std::unique_ptr<GstreamerPlugin> plugin(new GstreamerPlugin());
> +
> + plugin->ParseOptions(agent->Options());
> +
> + agent->Register(*plugin.release());
> +
> + return true;
> +}
> --
> 2.14.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list