[Spice-devel] [PATCH v5 17/20] spice-gtk: Add a GStreamer video decoder with support for MJPEG, VP8 and h264.
Christophe Fergeau
cfergeau at redhat.com
Mon Sep 21 08:07:38 PDT 2015
Hey,
Couple of minor comments below,
On Thu, Aug 27, 2015 at 09:03:06PM +0200, Francois Gouget wrote:
> Based on a patch by Jeremy White.
>
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>
> ***WARNNIG*** This is the first client patch that needs the protocol
> constants defined in patch 01.
>
> Changes since take 2:
> - None.
>
>
> configure.ac | 47 +++++++---
> src/Makefile.am | 12 ++-
> src/channel-display-gst.c | 216 +++++++++++++++++++++++++++++++++++++++++++++
> src/channel-display-priv.h | 1 +
> src/channel-display.c | 6 +-
> 5 files changed, 265 insertions(+), 17 deletions(-)
> create mode 100644 src/channel-display-gst.c
>
> diff --git a/configure.ac b/configure.ac
> index 0a60ec3..8ee71ca 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -301,7 +301,7 @@ AC_ARG_WITH([audio],
>
> case "$with_audio" in
> gstreamer|pulse|auto*)
> - PKG_CHECK_MODULES(GST, gstreamer-1.0 gstreamer-base-1.0 gstreamer-app-1.0 gstreamer-audio-1.0, [have_gst=yes], [have_gst=no])
> + PKG_CHECK_MODULES(GSTAUDIO, gstreamer-1.0 gstreamer-base-1.0 gstreamer-app-1.0 gstreamer-audio-1.0, [have_gst_audio=yes], [have_gst_audio=no])
> PKG_CHECK_MODULES(PULSE, libpulse libpulse-mainloop-glib, [have_pulse=yes], [have_pulse=no])
> ;;
> no*)
> @@ -312,7 +312,7 @@ esac
> AS_IF([test "x$with_audio" = "xauto" && test "x$have_pulse" = "xyes"],
> [with_audio=pulse])
>
> -AS_IF([test "x$with_audio" = "xauto" && test "x$have_gst" = "xyes"],
> +AS_IF([test "x$with_audio" = "xauto" && test "x$have_gst_audio" = "xyes"],
> [with_audio=gstreamer])
>
> AS_IF([test "x$with_audio" = "xauto"],
> @@ -321,22 +321,41 @@ AS_IF([test "x$with_audio" = "xauto"],
> AS_IF([test "x$with_audio" = "xpulse"],
> [AS_IF([test "x$have_pulse" = "xyes"],
> [AC_DEFINE([WITH_PULSE], 1, [Have pulseaudio?])],
> - [AC_MSG_ERROR([PulseAudio requested but not found])
> - ])
> -])
> + [AC_MSG_ERROR([PulseAudio requested but not found])]
> + )]
> +)
> AM_CONDITIONAL([WITH_PULSE], [test "x$have_pulse" = "xyes"])
> AC_SUBST(PULSE_CFLAGS)
> AC_SUBST(PULSE_LIBS)
>
> AS_IF([test "x$with_audio" = "xgstreamer"],
> - [AS_IF([test "x$have_gst" = "xyes"],
> - [AC_DEFINE([WITH_GSTAUDIO], 1, [Have GStreamer 1.0?])],
> - [AC_MSG_ERROR([GStreamer 1.0 requested but not found])
> - ])
> -])
> -AM_CONDITIONAL([WITH_GSTAUDIO], [test "x$have_gst" = "xyes"])
> -AC_SUBST(GST_CFLAGS)
> -AC_SUBST(GST_LIBS)
> + [AS_IF([test "x$have_gst_audio" = "xyes"],
> + [AC_DEFINE([WITH_GSTAUDIO], 1, [Have GStreamer 1.0 audio?])],
> + [AC_MSG_ERROR([GStreamer 1.0 audio requested but not found])]
> + )]
> +)
> +AM_CONDITIONAL([WITH_GSTAUDIO], [test "x$have_gst_audio" = "xyes"])
> +AC_SUBST(GSTAUDIO_CFLAGS)
> +AC_SUBST(GSTAUDIO_LIBS)
> +
Fwiw, all these changes could have gone in a separate preliminary commit
> +AC_ARG_ENABLE([gst-video],
> + AS_HELP_STRING([--enable-gst-video=@<:@auto/yes/no@:>@],
> + [Enable GStreamer video support @<:@default=auto@:>@]),
> + [],
> + [enable_gst_video="auto"])
> +
> +AS_IF([test "x$enable_gst_video" != "xno"],
> + [PKG_CHECK_MODULES(GSTVIDEO, gstreamer-1.0 gstreamer-base-1.0 gstreamer-app-1.0 gstreamer-video-1.0, [have_gst_video=yes], [have_gst_video=no])
> + AS_IF([test "x$have_gst_video" = "xyes"],
> + [AC_DEFINE([HAVE_GSTVIDEO], 1, [Have GStreamer 1.0 video?])],
> + [AS_IF([test "x$enable_gst_video" = "xyes"],
> + [AC_MSG_ERROR([GStreamer 1.0 video requested but not found])]
> + )]
> + )]
> +)
> +AM_CONDITIONAL([HAVE_GSTVIDEO], [test "x$have_gst_video" = "xyes"])]
> +AC_SUBST(GSTVIDEO_CFLAGS)
> +AC_SUBST(GSTVIDEO_LIBS)
>
> AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> AC_MSG_CHECKING([for jpeglib.h])
> @@ -702,7 +721,7 @@ SPICE_CFLAGS="$SPICE_CFLAGS $WARN_CFLAGS"
>
> AC_SUBST(SPICE_CFLAGS)
>
> -SPICE_GLIB_CFLAGS="$PIXMAN_CFLAGS $PULSE_CFLAGS $GST_CFLAGS $GLIB2_CFLAGS $GIO_CFLAGS $GOBJECT2_CFLAGS $SSL_CFLAGS $SASL_CFLAGS"
> +SPICE_GLIB_CFLAGS="$PROTOCOL_CFLAGS $PIXMAN_CFLAGS $PULSE_CFLAGS $GSTVIDEO_CFLAGS $GSTAUDIO_CFLAGS $GLIB2_CFLAGS $GIO_CFLAGS $GOBJECT2_CFLAGS $SSL_CFLAGS $SASL_CFLAGS"
PROTOCOL_CFLAGS is already part of COMMON_CFLAGS, any reason why you
needed to add it here too?
> SPICE_GTK_CFLAGS="$SPICE_GLIB_CFLAGS $GTK_CFLAGS "
>
> AC_SUBST(SPICE_GLIB_CFLAGS)
> diff --git a/src/Makefile.am b/src/Makefile.am
> index cf02198..925c75f 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -96,7 +96,8 @@ SPICE_COMMON_CPPFLAGS = \
> $(GOBJECT2_CFLAGS) \
> $(SSL_CFLAGS) \
> $(SASL_CFLAGS) \
> - $(GST_CFLAGS) \
> + $(GSTAUDIO_CFLAGS) \
> + $(GSTVIDEO_CFLAGS) \
> $(SMARTCARD_CFLAGS) \
> $(USBREDIR_CFLAGS) \
> $(GUDEV_CFLAGS) \
> @@ -207,7 +208,8 @@ libspice_client_glib_2_0_la_LIBADD = \
> $(PIXMAN_LIBS) \
> $(SSL_LIBS) \
> $(PULSE_LIBS) \
> - $(GST_LIBS) \
> + $(GSTAUDIO_LIBS) \
> + $(GSTVIDEO_LIBS) \
> $(SASL_LIBS) \
> $(SMARTCARD_LIBS) \
> $(USBREDIR_LIBS) \
> @@ -340,6 +342,12 @@ libspice_client_glib_2_0_la_SOURCES += \
> $(NULL)
> endif
>
> +if HAVE_GSTVIDEO
> +libspice_client_glib_2_0_la_SOURCES += \
> + channel-display-gst.c \
> + $(NULL)
> +endif
> +
> if WITH_PHODAV
> libspice_client_glib_2_0_la_SOURCES += \
> giopipe.c \
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> new file mode 100644
> index 0000000..3024356
> --- /dev/null
> +++ b/src/channel-display-gst.c
> @@ -0,0 +1,216 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> + Copyright (C) 2015 CodeWeavers, Inc
> +
> + 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 <http://www.gnu.org/licenses/>.
> +*/
> +#include "config.h"
> +
> +#include "spice-client.h"
> +#include "spice-common.h"
> +#include "spice-channel-priv.h"
> +
> +typedef struct GstDecoder GstDecoder;
I'm a bit wary of taking such a generic part of GStreamer namespace,
probably safer to make it SpiceGstDecoder.
> +#define VIDEO_DECODER_T GstDecoder
> +#include "channel-display-priv.h"
> +
> +#include <gst/gst.h>
> +#include <gst/app/gstappsrc.h>
> +#include <gst/app/gstappsink.h>
> +
> +
> +/* GStreamer decoder implementation */
> +
> +struct GstDecoder {
> + VideoDecoder base;
> +
> + /* ---------- Video characteristics ---------- */
> +
> + int width;
> + int height;
> +
> + /* ---------- GStreamer pipeline ---------- */
> +
> + GstElement *pipeline;
> + GstAppSrc *appsrc;
> + GstAppSink *appsink;
> +
> + /* ---------- Output frame data ---------- */
> +
> + GstSample *sample;
> + GstMapInfo mapinfo;
> +};
> +
> +
> +/* ---------- GStreamer pipeline ---------- */
> +
> +static void reset_pipeline(GstDecoder *decoder)
> +{
> + if (!decoder->pipeline) {
> + return;
> + }
> +
> + gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
> + gst_object_unref(decoder->appsrc);
> + gst_object_unref(decoder->appsink);
> + gst_object_unref(decoder->pipeline);
> + decoder->pipeline = NULL;
> +}
> +
> +static gboolean construct_pipeline(GstDecoder *decoder)
> +{
> + const gchar *src_caps, *gstdec_name;
> + switch (decoder->base.codec_type) {
> + case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> + src_caps = "image/jpeg";
> + gstdec_name = "jpegdec";
> + break;
> + case SPICE_VIDEO_CODEC_TYPE_VP8:
> + src_caps = "video/x-vp8";
> + gstdec_name = "vp8dec";
> + break;
> + case SPICE_VIDEO_CODEC_TYPE_H264:
> + src_caps = "video/x-h264";
> + gstdec_name = "h264parse ! avdec_h264";
> + break;
> + default:
> + spice_warning("Unknown codec type %d", decoder->base.codec_type);
> + return -1;
> + }
> +
> + GError *err = NULL;
> + gchar *desc = g_strdup_printf("appsrc name=src format=2 do-timestamp=1 caps=%s ! %s ! videoconvert ! appsink name=sink caps=video/x-raw,format=BGRx", src_caps, gstdec_name);
> + SPICE_DEBUG("GStreamer pipeline: %s", desc);
> + decoder->pipeline = gst_parse_launch_full(desc, NULL, GST_PARSE_FLAG_FATAL_ERRORS, &err);
> + g_free(desc);
> + if (!decoder->pipeline) {
> + spice_warning("GStreamer error: %s", err->message);
> + g_clear_error(&err);
> + return FALSE;
> + }
> +
> + decoder->appsrc = GST_APP_SRC(gst_bin_get_by_name(GST_BIN(decoder->pipeline), "src"));
> + decoder->appsink = GST_APP_SINK(gst_bin_get_by_name(GST_BIN(decoder->pipeline), "sink"));
> +
> + if (gst_element_set_state(decoder->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
> + SPICE_DEBUG("GStreamer error: Unable to set the pipeline to the playing state.");
> + reset_pipeline(decoder);
> + return FALSE;
> + }
> +
> + return TRUE;
> +}
> +
> +static gboolean push_compressed_buffer(GstDecoder *decoder,
> + SpiceMsgIn *frame_msg)
> +{
> + uint8_t *data;
> + uint32_t size = spice_msg_in_frame_data(frame_msg, &data);
> + if (size == 0) {
> + SPICE_DEBUG("got an empty frame buffer!");
> + return FALSE;
> + }
> +
> + // TODO. Grr. Seems like a wasted alloc
> + gpointer d = g_malloc(size);
> + memcpy(d, data, size);
> + GstBuffer *buffer = gst_buffer_new_wrapped(d, size);
I think you could hide it with gst_buffer_new() + gst_buffer_fill(), but
the memory would still end up being copied.
> +
> + if (gst_app_src_push_buffer(decoder->appsrc, buffer) != GST_FLOW_OK) {
> + SPICE_DEBUG("GStreamer error: unable to push frame of size %d", size);
> + return FALSE;
> + }
> +
> + return TRUE;
> +}
> +
> +static void release_last_frame(GstDecoder *decoder)
> +{
> + if (decoder->mapinfo.memory) {
> + gst_memory_unmap(decoder->mapinfo.memory, &decoder->mapinfo);
> + gst_memory_unref(decoder->mapinfo.memory);
This is opencoding gst_buffer_unmap(), which is gst_buffer_map()
documentation says should be used to release the resources which were
mapped.
> + decoder->mapinfo.memory = NULL;
> + }
> + if (decoder->sample) {
> + gst_sample_unref(decoder->sample);
> + decoder->sample = NULL;
> + }
> +}
> +
> +static uint8_t* pull_raw_frame(GstDecoder *decoder)
> +{
> + decoder->sample = gst_app_sink_pull_sample(decoder->appsink);
> + if (!decoder->sample) {
> + SPICE_DEBUG("GStreamer error: could not pull sample");
> + return NULL;
> + }
> +
> + GstBuffer *buffer = gst_sample_get_buffer(decoder->sample);
I'd take a ref on the returned buffer, and unref decoder->sample right
away (ie no need to have it as GstDecoder::sample)
> + if (gst_buffer_map(buffer, &decoder->mapinfo, GST_MAP_READ)) {
> + return decoder->mapinfo.data;
> + }
> + return NULL;
> +}
> +
> +
> +/* ---------- VideoDecoder's public API ---------- */
> +
> +static void gst_decoder_destroy(GstDecoder *decoder)
> +{
> + release_last_frame(decoder);
> + reset_pipeline(decoder);
> + g_free(decoder);
> + /* Don't call gst_deinit() as other parts may still be using GStreamer */
> +}
> +
> +static uint8_t* gst_decoder_decode_frame(GstDecoder *decoder,
> + SpiceMsgIn *frame_msg)
> +{
> + int width, height;
> +
> + stream_get_dimensions(decoder->base.stream, frame_msg, &width, &height);
> + if (width != decoder->width || height != decoder->height) {
> + SPICE_DEBUG("video format change: width %d -> %d, height %d -> %d", decoder->width, width, decoder->height, height);
> + decoder->width = width;
> + decoder->height = height;
> + reset_pipeline(decoder);
> + }
> + if (!decoder->pipeline && !construct_pipeline(decoder)) {
> + return NULL;
> + }
> +
> + /* Release the output frame buffer early so the pipeline can reuse it.
> + * This also simplifies error handling.
> + */
> + release_last_frame(decoder);
> +
> + if (push_compressed_buffer(decoder, frame_msg)) {
> + return pull_raw_frame(decoder);
> + }
> + return NULL;
> +}
> +
> +G_GNUC_INTERNAL
> +GstDecoder* create_gstreamer_decoder(int codec_type, display_stream *stream)
> +{
> + gst_init(NULL, NULL);
I'd do it much earlier in spice-gtk startup so that it's only done once.
Also, I suspect this can be slow, so better not to do it while a spice
session is going on if possible. GOnce could be used to force it to be
done only once. gst_init_check() would be more appropriate as gst_init()
can abort().
> +
> + GstDecoder *decoder = g_malloc0(sizeof(*decoder));
g_new0(GstDecoder, 1); maybe
> + decoder->base.destroy = &gst_decoder_destroy;
> + decoder->base.decode_frame = &gst_decoder_decode_frame;
> + decoder->base.codec_type = codec_type;
> + decoder->base.stream = stream;
> +
> + return decoder;
> +}
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index a01183b..0c9f508 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -70,6 +70,7 @@ struct VideoDecoder {
> * @return: A pointer to a structure implementing the VideoDecoder methods.
> */
> VIDEO_DECODER_T* create_mjpeg_decoder(int codec_type, display_stream *stream);
> +VIDEO_DECODER_T* create_gstreamer_decoder(int codec_type, display_stream *stream);
missing #ifdef HAVE_GSTVIDEO
>
>
> typedef struct display_surface {
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 4033b2f..e9cbf8d 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -596,6 +596,10 @@ static void spice_display_channel_reset_capabilities(SpiceChannel *channel)
> if (SPICE_DISPLAY_CHANNEL(channel)->priv->enable_adaptive_streaming) {
> spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_STREAM_REPORT);
> }
> + spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_MULTI_CODEC);
> + spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_CODEC_MJPEG);
> + spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_CODEC_VP8);
> + spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_CODEC_H264);
here too I think
> }
>
> static void destroy_surface(gpointer data)
> @@ -1009,7 +1013,7 @@ static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
> st->video_decoder = create_mjpeg_decoder(op->codec_type, st);
> break;
> default:
> - st->video_decoder = NULL;
> + st->video_decoder = create_gstreamer_decoder(op->codec_type, st);
Can it be the default decoder before the last patch in the series? Or
should it be
case SPICE_VIDEO_CODEC_TYPE_VP8:
case SPICE_VIDEO_CODEC_TYPE_H264:
st->video_decoder = create_gstreamer_decoder(op->codec_type, st);
default:
st->video_decoder = NULL;
?
This needs to be wrapped in #ifdef HAVE_GSTVIDEO
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150921/6db1d3e1/attachment-0001.sig>
More information about the Spice-devel
mailing list