[Spice-devel] [PATCH spice-streaming-agent 1/1] Adding gstreamer based plugin
Snir Sheriber
ssheribe at redhat.com
Sun Feb 25 09:08:35 UTC 2018
Hi, thanks for reviewing this
On 02/23/2018 02:14 PM, Christophe de Dinechin wrote:
> 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?
True, missed that.
>
>
>> +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?
Possible although other encoders may need additional elements in the
pipeline
I haven't try others
>
>> + 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)
Yep as toso mentioned (SPICE_CHECK_GSTREAMER_ELEMENTS defined in
spice-common)
>
>> + 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