Hey,
Mostly looks good, a few comments/questions below.
On Tue, Mar 01, 2016 at 04:50:57PM +0100, Francois Gouget wrote:
> This introduces a pared down GStreamer-based video encoder to serve as
> the basis for later enhancements.
> In this form the new encoder supports both regular and sized streams
> but lacks any rate control. It should still work fine if bandwidth is
> sufficient such as on LANs.
>
> Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> ---
> configure.ac | 25 +++
> server/Makefile.am | 8 +
> server/gstreamer-encoder.c | 531 +++++++++++++++++++++++++++++++++++++++++++++
> server/stream.c | 18 +-
> server/video-encoder.h | 4 +
> 5 files changed, 584 insertions(+), 2 deletions(-)
> create mode 100644 server/gstreamer-encoder.c
>
> diff --git a/configure.ac b/configure.ac
> index f5445c0..d9aa073 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -69,6 +69,30 @@ dnl =========================================================================
> dnl Check optional features
> SPICE_CHECK_SMARTCARD
>
> +AC_ARG_ENABLE(gstreamer,
> + AS_HELP_STRING([--enable-gstreamer=@<:@auto/yes/no@:>@],
> + [Enable GStreamer 1.0 support]),,
> + [enable_gstreamer="auto"])
> +
> +if test "x$enable_gstreamer" != "xno"; then
> + SPICE_CHECK_GSTREAMER(GSTREAMER_1_0, 1.0, [gstreamer-1.0 gstreamer-base-1.0 gstreamer-app-1.0 gstreamer-video-1.0],
> + [enable_gstreamer="yes"
> + SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-base 1.0], [appsrc videoconvert appsink])
> + SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gstreamer-libav 1.0], [avenc_mjpeg])
> + SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-good 1.0], [vp8enc])
> + SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-ugly 1.0], [x264enc])
Maybe the addition of the tests for vp8enc/x264enc should be moved to
the respective commits making use of these?
However, I don't think we can go with these compile-time tests and
disable gstreamer altogether when they are missing. This would make
it impossible to enable gstreamer support in fedora/rhel where
gstreamer1-libav and gstreamer1-plugins-ugly are not part of the main
repositories.
I'd address this after this series has been merged though, it has been
pending for too much time, probably better to avoid delaying it even
more.
> + ],
> + [if test "x$enable_gstreamer" = "xyes"; then
> + AC_MSG_ERROR([GStreamer 1.0 support requested but not found. You may set GSTREAMER_1_0_CFLAGS and GSTREAMER_1_0_LIBS to avoid the need to call pkg-config.])
> + fi
> + ])
> +fi
> +AM_CONDITIONAL(HAVE_GSTREAMER_1_0, test "x$have_gstreamer_1_0" = "xyes")
> +
> +if test x"$gstreamer_missing" != x; then
> + SPICE_WARNING([The following GStreamer $enable_gstreamer tools/elements are missing:$gstreamer_missing. The GStreamer video encoder can be built but may not work.])
> +fi
> +
> AC_ARG_ENABLE([automated_tests],
> AS_HELP_STRING([--enable-automated-tests], [Enable automated tests using spicy-screenshot (part of spice--gtk)]),,
> [enable_automated_tests="no"])
> @@ -241,6 +265,7 @@ AC_MSG_NOTICE([
>
> LZ4 support: ${enable_lz4}
> Smartcard: ${have_smartcard}
> + GStreamer 1.0: ${have_gstreamer_1_0}
> SASL support: ${have_sasl}
> Automated tests: ${enable_automated_tests}
> Manual: ${have_asciidoc}
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 72ea669..4bdddcb 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -12,6 +12,7 @@ AM_CPPFLAGS = \
> $(SASL_CFLAGS) \
> $(SLIRP_CFLAGS) \
> $(SMARTCARD_CFLAGS) \
> + $(GSTREAMER_1_0_CFLAGS) \
> $(SPICE_PROTOCOL_CFLAGS) \
> $(SSL_CFLAGS) \
> $(VISIBILITY_HIDDEN_CFLAGS) \
> @@ -45,6 +46,7 @@ libserver_la_LIBADD = \
> $(PIXMAN_LIBS) \
> $(SASL_LIBS) \
> $(SLIRP_LIBS) \
> + $(GSTREAMER_1_0_LIBS) \
> $(SSL_LIBS) \
> $(Z_LIBS) \
> $(SPICE_NONPKGCONFIG_LIBS) \
> @@ -152,6 +154,12 @@ libserver_la_SOURCES += \
> $(NULL)
> endif
>
> +if HAVE_GSTREAMER_1_0
> +libserver_la_SOURCES += \
> + gstreamer-encoder.c \
> + $(NULL)
> +endif
> +
> libspice_server_la_LIBADD = libserver.la
> libspice_server_la_SOURCES =
>
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> new file mode 100644
> index 0000000..3fb5181
> --- /dev/null
> +++ b/server/gstreamer-encoder.c
> @@ -0,0 +1,531 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> + Copyright (C) 2015 Jeremy White
> + Copyright (C) 2015 Francois Gouget
> +
> + This library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + This library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with this library; if not, see <<A HREF="http://www.gnu.org/licenses/";>http://www.gnu.org/licenses/</A>>.
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <gst/gst.h>
> +#include <gst/app/gstappsrc.h>
> +#include <gst/app/gstappsink.h>
> +
> +#include "red-common.h"
> +#include "video-encoder.h"
> +
> +
> +#define SPICE_GST_DEFAULT_FPS 30
> +
> +
> +typedef struct {
> + SpiceBitmapFmt spice_format;
> + const char *format;
> + uint32_t bpp;
> +} SpiceFormatForGStreamer;
> +
> +typedef struct SpiceGstEncoder {
> + VideoEncoder base;
> +
> + /* Rate control callbacks */
> + VideoEncoderRateControlCbs cbs;
> +
> + /* Spice's initial bit rate estimation in bits per second. */
> + uint64_t starting_bit_rate;
> +
> + /* ---------- Video characteristics ---------- */
> +
> + uint32_t width;
> + uint32_t height;
> + SpiceFormatForGStreamer *format;
> + SpiceBitmapFmt spice_format;
> +
> + /* ---------- GStreamer pipeline ---------- */
> +
> + /* Pointers to the GStreamer pipeline elements. If pipeline is NULL the
> + * other pointers are invalid.
> + */
> + GstElement *pipeline;
> + GstAppSrc *appsrc;
> + GstElement *gstenc;
> + GstAppSink *appsink;
> +
> + /* If src_caps is NULL the pipeline has not been configured yet. */
> + GstCaps *src_caps;
> +
> + /* The frame counter for GStreamer buffers */
> + uint32_t frame;
> +
> + /* The bit rate target for the outgoing network stream. (bits per second) */
> + uint64_t bit_rate;
> +
> + /* The minimum bit rate */
> +# define SPICE_GST_MIN_BITRATE (128 * 1024)
> +
> + /* The default bit rate */
> +# define SPICE_GST_DEFAULT_BITRATE (8 * 1024 * 1024)
> +} SpiceGstEncoder;
> +
> +
> +/* ---------- Miscellaneous SpiceGstEncoder helpers ---------- */
> +
> +static inline double get_mbps(uint64_t bit_rate)
> +{
> + return (double)bit_rate / 1024 / 1024;
> +}
> +
> +/* Returns the source frame rate which may change at any time so don't store
> + * the result.
> + */
> +static uint32_t get_source_fps(SpiceGstEncoder *encoder)
> +{
> + return encoder->cbs.get_source_fps ?
> + encoder->cbs.get_source_fps(encoder->cbs.opaque) : SPICE_GST_DEFAULT_FPS;
> +}
> +
> +static inline int is_pipeline_configured(SpiceGstEncoder *encoder)
> +{
> + return encoder->src_caps != NULL;
> +}
> +
> +static void free_pipeline(SpiceGstEncoder *encoder)
> +{
> + if (encoder->src_caps) {
> + gst_caps_unref(encoder->src_caps);
> + encoder->src_caps = NULL;
> + }
> + if (encoder->pipeline) {
> + gst_element_set_state(encoder->pipeline, GST_STATE_NULL);
> + gst_object_unref(encoder->appsrc);
> + gst_object_unref(encoder->gstenc);
> + gst_object_unref(encoder->appsink);
> + gst_object_unref(encoder->pipeline);
> + encoder->pipeline = NULL;
> + }
> +}
> +
> +/* The maximum bit rate we will use for the current video.
> + *
> + * This is based on a 10x compression ratio which should be more than enough
> + * for even MJPEG to provide good quality.
> + */
> +static uint64_t get_bit_rate_cap(SpiceGstEncoder *encoder)
> +{
> + uint32_t raw_frame_bits = encoder->width * encoder->height * encoder->format->bpp;
> + return raw_frame_bits * get_source_fps(encoder) / 10;
> +}
> +
> +static void adjust_bit_rate(SpiceGstEncoder *encoder)
> +{
> + if (encoder->bit_rate == 0) {
> + /* Use the default value, */
> + encoder->bit_rate = SPICE_GST_DEFAULT_BITRATE;
> + } else if (encoder->bit_rate < SPICE_GST_MIN_BITRATE) {
> + /* don't let the bit rate go too low */
> + encoder->bit_rate = SPICE_GST_MIN_BITRATE;
> + } else {
> + /* or too high */
> + encoder->bit_rate = MIN(encoder->bit_rate, get_bit_rate_cap(encoder));
> + }
> + spice_debug("adjust_bit_rate(%.3fMbps)", get_mbps(encoder->bit_rate));
> +}
> +
> +
> +/* ---------- GStreamer pipeline ---------- */
> +
> +/* A helper for spice_gst_encoder_encode_frame() */
> +static SpiceFormatForGStreamer *map_format(SpiceBitmapFmt format)
> +{
> + /* See GStreamer's part-mediatype-video-raw.txt and
> + * section-types-definitions.html documents.
> + */
> + static SpiceFormatForGStreamer format_map[] = {
Some constness could probably be added here and to the return
value.
> + {SPICE_BITMAP_FMT_RGBA, "BGRA", 32},
> + {SPICE_BITMAP_FMT_16BIT, "RGB15", 16},
> + /* TODO: Test the other formats */
> + {SPICE_BITMAP_FMT_32BIT, "BGRx", 32},
> + {SPICE_BITMAP_FMT_24BIT, "BGR", 24},
> + };
> +
> + int i;
> + for (i = 0; i < G_N_ELEMENTS(format_map); i++) {
> + if (format_map[i].spice_format == format) {
> + if (i > 1) {
> + spice_warning("The %d format has not been tested yet", format);
> + }
> + return &format_map[i];
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static void set_appsrc_caps(SpiceGstEncoder *encoder)
> +{
> + if (encoder->src_caps) {
> + gst_caps_unref(encoder->src_caps);
> + }
> + encoder->src_caps = gst_caps_new_simple(
> + "video/x-raw",
> + "format", G_TYPE_STRING, encoder->format->format,
> + "width", G_TYPE_INT, encoder->width,
> + "height", G_TYPE_INT, encoder->height,
> + "framerate", GST_TYPE_FRACTION, get_source_fps(encoder), 1,
> + NULL);
> + g_object_set(G_OBJECT(encoder->appsrc), "caps", encoder->src_caps, NULL);
> +}
> +
> +/* A helper for spice_gst_encoder_encode_frame() */
> +static gboolean create_pipeline(SpiceGstEncoder *encoder)
> +{
> + GError *err = NULL;
> + /* Set max-threads to ensure zero-frame latency */
> + const gchar *desc = "appsrc is-live=true format=time do-timestamp=true name=src ! videoconvert ! avenc_mjpeg max-threads=1 name=encoder ! appsink name=sink";
> + spice_debug("GStreamer pipeline: %s", desc);
> + encoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
> + if (!encoder->pipeline || err) {
> + spice_warning("GStreamer error: %s", err->message);
> + g_clear_error(&err);
> + if (encoder->pipeline) {
> + gst_object_unref(encoder->pipeline);
> + encoder->pipeline = NULL;
> + }
> + return FALSE;
> + }
> + encoder->appsrc = GST_APP_SRC(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "src"));
> + encoder->gstenc = gst_bin_get_by_name(GST_BIN(encoder->pipeline), "encoder");
> + encoder->appsink = GST_APP_SINK(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "sink"));
> + return TRUE;
> +}
> +
> +/* A helper for spice_gst_encoder_encode_frame() */
> +static gboolean configure_pipeline(SpiceGstEncoder *encoder,
> + const SpiceBitmap *bitmap)
> +{
> + if (!encoder->pipeline && !create_pipeline(encoder)) {
> + return FALSE;
> + }
> +
> + /* Configure the encoder bitrate */
> + adjust_bit_rate(encoder);
> + g_object_set(G_OBJECT(encoder->gstenc), "bitrate", encoder->bit_rate, NULL);
> +
> + /* See https://bugzilla.gnome.org/show_bug.cgi?id=753257 */
> + spice_debug("removing the pipeline clock");
> + gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
> +
> + /* Set the source caps */
> + set_appsrc_caps(encoder);
> +
> + /* Start playing */
> + spice_debug("setting state to PLAYING");
> + if (gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
> + spice_warning("GStreamer error: unable to set the pipeline to the playing state");
> + free_pipeline(encoder);
> + return FALSE;
> + }
> +
> + return TRUE;
> +}
> +
> +/* A helper for spice_gst_encoder_encode_frame() */
> +static void reconfigure_pipeline(SpiceGstEncoder *encoder)
> +{
> + if (!is_pipeline_configured(encoder)) {
> + return;
> + }
> + if (gst_element_set_state(encoder->pipeline, GST_STATE_PAUSED) == GST_STATE_CHANGE_FAILURE) {
> + spice_debug("GStreamer error: could not pause the pipeline, rebuilding it instead");
> + free_pipeline(encoder);
> + }
> + set_appsrc_caps(encoder);
> + if (gst_element_set_state(encoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
> + spice_debug("GStreamer error: could not restart the pipeline, rebuilding it instead");
> + free_pipeline(encoder);
> + }
> +}
> +
> +/* A helper for the *_copy() functions */
> +static int is_chunk_padded(const SpiceBitmap *bitmap, uint32_t index)
> +{
> + SpiceChunks *chunks = bitmap->data;
> + if (chunks->chunk[index].len % bitmap->stride != 0) {
> + spice_warning("chunk %d/%d is padded, cannot copy", index, chunks->num_chunks);
> + return TRUE;
> + }
> + return FALSE;
> +}
> +
> +/* A helper for push_raw_frame() */
> +static inline int line_copy(SpiceGstEncoder *encoder, const SpiceBitmap *bitmap,
> + uint32_t chunk_offset, uint32_t stream_stride,
> + uint32_t height, uint8_t *buffer)
> +{
> + uint8_t *dst = buffer;
> + SpiceChunks *chunks = bitmap->data;
> + uint32_t chunk_index = 0;
> + for (int l = 0; l < height; l++) {
> + /* We may have to move forward by more than one chunk the first
> + * time around.
> + */
> + while (chunk_offset >= chunks->chunk[chunk_index].len) {
> + if (is_chunk_padded(bitmap, chunk_index)) {
> + return FALSE;
> + }
> + chunk_offset -= chunks->chunk[chunk_index].len;
> + chunk_index++;
> + }
> +
> + /* Copy the line */
> + uint8_t *src = chunks->chunk[chunk_index].data + chunk_offset;
> + memcpy(dst, src, stream_stride);
> + dst += stream_stride;
> + chunk_offset += bitmap->stride;
> + }
> + spice_return_val_if_fail(dst - buffer == stream_stride * height, FALSE);
> + return TRUE;
> +}
> +
> +/* A helper for push_raw_frame() */
> +static inline int chunk_copy(SpiceGstEncoder *encoder, const SpiceBitmap *bitmap,
> + uint32_t chunk_offset, uint32_t len, uint8_t *dst)
> +{
> + SpiceChunks *chunks = bitmap->data;
> + uint32_t chunk_index = 0;
> + /* We can copy the bitmap chunk by chunk */
> + while (len && chunk_index < chunks->num_chunks) {
> + if (is_chunk_padded(bitmap, chunk_index)) {
> + return FALSE;
> + }
> + if (chunk_offset >= chunks->chunk[chunk_index].len) {
> + chunk_offset -= chunks->chunk[chunk_index].len;
> + chunk_index++;
> + continue;
> + }
I would split the loop here to make it more obvious that we have a first
loop skipping chunks we need to ignore, and then a loop doing the copy.
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel