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@xxxxxxxxxxxxxxx> > --- > > ***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
Attachment:
pgpunWxSvUwll.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel