On Sun, Jan 13, 2019 at 7:07 PM Snir Sheriber <ssheribe@xxxxxxxxxx> wrote:
>
> Hi,
>
>
> On 1/9/19 12:09 PM, Frediano Ziglio wrote:
> > From: Marc-André Lureau<marcandre.lureau@xxxxxxxxxx>
> >
> > GStreamer is being increasingly used by spice-gtk. Let's make it a
> > core requirement.
>
>
> So if we are making the gst-video stuff required too, won't be worth to
> make the builtin mjpeg
> option a feature and avoid the mandatory dependency? Or it's too
> fragile\early\wrong to assume
> gstreamer can always handle mjpeg?
>
Good question, I haven't evaluated the gstreamer mjpeg support. I
propose we open a bug for 0.37 to check if we can drop builtin mjpeg.
> Other than this looks good to me too (also the gstaudio one), maybe
> worth squash the
> gstvideo+gstaudio? (too big?)
I rather not, they are different options, different code paths. I
don't feel strongly about it either.
>
> Snir.
>
> > Signed-off-by: Marc-André Lureau<marcandre.lureau@xxxxxxxxxx>
> > ---
> > .gitlab-ci.yml | 2 --
> > configure.ac | 41 ++++++++++++++------------------------
> > meson.build | 17 +---------------
> > meson_options.txt | 5 -----
> > src/Makefile.am | 7 +------
> > src/channel-display-priv.h | 10 +---------
> > src/channel-display.c | 7 -------
> > src/meson.build | 5 +----
> > src/spice-widget-priv.h | 4 ----
> > src/spice-widget.c | 13 +++---------
> > 10 files changed, 22 insertions(+), 89 deletions(-)
> >
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index 78b339e7..c956adda 100644
> > --- a/.gitlab-ci.yml
> > +++ b/.gitlab-ci.yml
> > @@ -32,7 +32,6 @@ makecheck_simple:
> > script:
> > - ./autogen.sh --enable-static
> > --enable-lz4=no
> > - --enable-gstvideo=no
> > --enable-webdav=no
> > --with-sasl=no
> > --with-coroutine=auto
> > @@ -46,7 +45,6 @@ makecheck_simple:
> > makecheck_simple-meson:
> > script:
> > - meson build -Dauto_features=disabled
> > - -Dgstvideo=false
> > -Dusbredir=false
> > -Ddbus=false || (cat build/meson-logs/meson-log.txt && exit 1)
> > - ninja -C build
> > diff --git a/configure.ac b/configure.ac
> > index 85827b1d..ea4e8ff2 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -221,28 +221,21 @@ SPICE_CHECK_GSTREAMER(GSTAUDIO, 1.0,
> > ],
> > [AC_MSG_ERROR([Required GStreamer packages missing])])
> >
> > -AC_ARG_ENABLE([gstvideo],
> > - AS_HELP_STRING([--enable-gstvideo=@<:@auto/yes/no@:>@],
> > - [Enable GStreamer video support @<:@default=auto@:>@]),
> > - [],
> > - [enable_gstvideo="auto"])
> > -AS_IF([test "x$enable_gstvideo" != "xno"],
> > - [SPICE_CHECK_GSTREAMER(GSTVIDEO, 1.0,
> > - [gstreamer-1.0 gstreamer-base-1.0 gstreamer-app-1.0 gstreamer-video-1.0],
> > - [missing_gstreamer_elements=""
> > - SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-base 1.0], [appsrc videoconvert appsink])
> > - SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-good 1.0], [jpegdec vp8dec vp9dec])
> > - SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-bad 1.0], [h264parse h265parse])
> > - SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gstreamer-libav 1.0], [avdec_h264 avdec_h265])
> > - AS_IF([test x"$missing_gstreamer_elements" = "xyes"],
> > - SPICE_WARNING([The GStreamer video decoder can be built but may not work.]))
> > - ],
> > - [AS_IF([test "x$enable_gstvideo" = "xyes"],
> > - AC_MSG_ERROR([GStreamer 1.0 video requested but not found]))
> > - ])
> > - ], [have_gstvideo="no"]
> > -)
> > -AM_CONDITIONAL([HAVE_GSTVIDEO], [test "x$have_gstvideo" = "xyes"])
> > +SPICE_CHECK_GSTREAMER(GSTVIDEO, 1.0,
> > + [gstreamer-1.0 gstreamer-base-1.0 gstreamer-app-1.0 gstreamer-video-1.0],
> > + [missing_gstreamer_elements=""
> > + SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0,
> > + [gst-plugins-base 1.0], [appsrc videoconvert appsink])
> > + SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0,
> > + [gst-plugins-good 1.0], [jpegdec vp8dec vp9dec])
> > + SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0,
> > + [gst-plugins-bad 1.0], [h264parse h265parse])
> > + SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0,
> > + [gstreamer-libav 1.0], [avdec_h264 avdec_h265])
> > + AS_IF([test x"$missing_gstreamer_elements" = "xyes"],
> > + SPICE_WARNING([The GStreamer video decoder can be built but may not work.]))
> > + ],
> > + [AC_MSG_ERROR([Required GStreamer packages missing])])
> >
> > AC_ARG_ENABLE([builtin-mjpeg],
> > AS_HELP_STRING([--enable-builtin-mjpeg], [Enable the builtin mjpeg video decoder @<:@default=yes@:>@]),
> > @@ -252,9 +245,6 @@ AS_IF([test "x$enable_builtin_mjpeg" = "xyes"],
> > [AC_DEFINE([HAVE_BUILTIN_MJPEG], 1, [Use the builtin mjpeg decoder?])])
> > AM_CONDITIONAL(HAVE_BUILTIN_MJPEG, [test "x$enable_builtin_mjpeg" != "xno"])
> >
> > -AS_IF([test "x$enable_builtin_mjpeg$enable_gstvideo" = "xnono"],
> > - [SPICE_WARNING([No builtin MJPEG or GStreamer decoder, video will not be streamed])])
> > -
> > AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > AC_MSG_CHECKING([for jpeglib.h])
> > AC_TRY_CPP(
> > @@ -547,7 +537,6 @@ AC_MSG_NOTICE([
> > Gtk: ${with_gtk}
> > Coroutine: ${with_coroutine}
> > PulseAudio: ${enable_pulse}
> > - GStreamer Video: ${have_gstvideo}
> > SASL support: ${have_sasl}
> > Smartcard support: ${have_smartcard}
> > USB redirection support: ${have_usbredir} ${with_usbredir_hotplug}
> > diff --git a/meson.build b/meson.build
> > index 4ade991e..e1e5e919 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -161,22 +161,11 @@ if d.found()
> > spice_gtk_has_pulse = true
> > endif
> >
> > -deps = ['gstreamer-1.0', 'gstreamer-base-1.0', 'gstreamer-app-1.0', 'gstreamer-audio-1.0']
> > +deps = ['gstreamer-1.0', 'gstreamer-base-1.0', 'gstreamer-app-1.0', 'gstreamer-audio-1.0', 'gstreamer-video-1.0']
> > foreach dep : deps
> > spice_glib_deps += dependency(dep)
> > endforeach
> >
> > -# gstvideo
> > -spice_gtk_has_gstvideo = false
> > -if get_option('gstvideo')
> > - deps = ['gstreamer-video-1.0']
> > - foreach dep : deps
> > - spice_glib_deps += dependency(dep)
> > - endforeach
> > - spice_gtk_config_data.set('HAVE_GSTVIDEO', '1')
> > - spice_gtk_has_gstvideo = true
> > -endif
> > -
> > # builtin-mjpeg
> > spice_gtk_has_builtin_mjpeg = false
> > if get_option('builtin-mjpeg')
> > @@ -184,10 +173,6 @@ if get_option('builtin-mjpeg')
> > spice_gtk_has_builtin_mjpeg = true
> > endif
> >
> > -if not spice_gtk_has_gstvideo and not spice_gtk_has_builtin_mjpeg
> > - warning('No builtin MJPEG or GStreamer decoder, video will not be streamed')
> > -endif
> > -
> > # usbredir
> > spice_gtk_has_usbredir = false
> > if get_option('usbredir')
> > diff --git a/meson_options.txt b/meson_options.txt
> > index 08efaceb..8ac6c684 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -10,11 +10,6 @@ option('pulse',
> > type : 'feature',
> > description: 'Enable the PulseAudio backend')
> >
> > -option('gstvideo',
> > - type : 'boolean',
> > - value : true,
> > - description : 'Enable GStreamer video support')
> > -
> > option('builtin-mjpeg',
> > type : 'boolean',
> > value : true,
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index f8335ca7..b50c4262 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -241,6 +241,7 @@ libspice_client_glib_impl_la_SOURCES = \
> > channel-webdav.c \
> > channel-cursor.c \
> > channel-display.c \
> > + channel-display-gst.c \
> > channel-display-priv.h \
> > channel-inputs.c \
> > channel-main.c \
> > @@ -326,12 +327,6 @@ libspice_client_glib_impl_la_SOURCES += \
> > $(NULL)
> > endif
> >
> > -if HAVE_GSTVIDEO
> > -libspice_client_glib_impl_la_SOURCES += \
> > - channel-display-gst.c \
> > - $(NULL)
> > -endif
> > -
> > if WITH_PHODAV
> > libspice_client_glib_impl_la_SOURCES += \
> > giopipe.c \
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index 72ad90ff..6c6b6c40 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -31,9 +31,7 @@
> > #include "common/quic.h"
> > #include "common/rop3.h"
> >
> > -#ifdef HAVE_GSTVIDEO
> > -#include "gst/gst.h"
> > -#endif
> > +#include <gst/gst.h>
> >
> > G_BEGIN_DECLS
> >
> > @@ -90,12 +88,8 @@ struct VideoDecoder {
> > #ifdef HAVE_BUILTIN_MJPEG
> > VideoDecoder* create_mjpeg_decoder(int codec_type, display_stream *stream);
> > #endif
> > -#ifdef HAVE_GSTVIDEO
> > VideoDecoder* create_gstreamer_decoder(int codec_type, display_stream *stream);
> > gboolean gstvideo_has_codec(int codec_type);
> > -#else
> > -# define gstvideo_has_codec(codec_type) FALSE
> > -#endif
> >
> >
> > typedef struct display_surface {
> > @@ -205,9 +199,7 @@ void stream_dropped_frame_on_playback(display_stream *st);
> > #define SPICE_UNKNOWN_STRIDE 0
> > void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t width, uint32_t height, int stride, uint8_t* data);
> > guintptr get_window_handle(display_stream *st);
> > -#ifdef HAVE_GSTVIDEO
> > gboolean hand_pipeline_to_widget(display_stream *st, GstPipeline *pipeline);
> > -#endif
> >
> >
> > G_END_DECLS
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 56799c04..3dfd721e 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -29,9 +29,6 @@
> > #include "spice-session-priv.h"
> > #include "channel-display-priv.h"
> > #include "decode.h"
> > -#ifdef HAVE_GSTVIDEO
> > -#include "gst/gst.h"
> > -#endif
> >
> > /**
> > * SECTION:channel-display
> > @@ -1295,9 +1292,7 @@ static display_stream *display_stream_create(SpiceChannel *channel,
> > break;
> > #endif
> > default:
> > -#ifdef HAVE_GSTVIDEO
> > st->video_decoder = create_gstreamer_decoder(codec_type, st);
> > -#endif
> > break;
> > }
> > if (st->video_decoder == NULL) {
> > @@ -1420,7 +1415,6 @@ void stream_display_frame(display_stream *st, SpiceFrame *frame,
> > }
> > }
> >
> > -#ifdef HAVE_GSTVIDEO
> > G_GNUC_INTERNAL
> > gboolean hand_pipeline_to_widget(display_stream *st, GstPipeline *pipeline)
> > {
> > @@ -1432,7 +1426,6 @@ gboolean hand_pipeline_to_widget(display_stream *st, GstPipeline *pipeline)
> > }
> > return res;
> > }
> > -#endif
> >
> > /* after a sequence of 3 drops, push a report to the server, even
> > * if the report window is bigger */
> > diff --git a/src/meson.build b/src/meson.build
> > index 5754f1fc..d47d58dc 100644
> > --- a/src/meson.build
> > +++ b/src/meson.build
> > @@ -88,6 +88,7 @@ spice_client_glib_sources = [
> > 'bio-gio.c',
> > 'bio-gio.h',
> > 'channel-base.c',
> > + 'channel-display-gst.c',
> > 'channel-display-priv.h',
> > 'channel-playback-priv.h',
> > 'channel-usbredir-priv.h',
> > @@ -128,10 +129,6 @@ if spice_gtk_has_builtin_mjpeg
> > spice_client_glib_sources += 'channel-display-mjpeg.c'
> > endif
> >
> > -if spice_gtk_has_gstvideo
> > - spice_client_glib_sources += 'channel-display-gst.c'
> > -endif
> > -
> > if spice_gtk_has_polkit
> > spice_client_glib_sources += ['usb-acl-helper.c',
> > 'usb-acl-helper.h']
> > diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
> > index 54fe177a..65eb4047 100644
> > --- a/src/spice-widget-priv.h
> > +++ b/src/spice-widget-priv.h
> > @@ -32,9 +32,7 @@
> > #include "spice-common.h"
> > #include "spice-gtk-session.h"
> >
> > -#ifdef HAVE_GSTVIDEO
> > #include <gst/video/videooverlay.h>
> > -#endif
> >
> > G_BEGIN_DECLS
> >
> > @@ -154,9 +152,7 @@ struct _SpiceDisplayPrivate {
> > } egl;
> > #endif // HAVE_EGL
> > double scroll_delta_y;
> > -#ifdef HAVE_GSTVIDEO
> > GWeakRef overlay_weak_ref;
> > -#endif
> > };
> >
> > int spice_cairo_image_create (SpiceDisplay *display);
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index af0301c1..c71cc534 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -121,10 +121,8 @@ static void size_allocate(GtkWidget *widget, GtkAllocation *conf, gpointer data)
> > static gboolean draw_event(GtkWidget *widget, cairo_t *cr, gpointer data);
> > static void update_size_request(SpiceDisplay *display);
> > static GdkDevice *spice_gdk_window_get_pointing_device(GdkWindow *window);
> > -#ifdef HAVE_GSTVIDEO
> > static void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data);
> > static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer data);
> > -#endif
> >
> > /* ---------------------------------------------------------------- */
> >
> > @@ -657,14 +655,13 @@ static void spice_display_init(SpiceDisplay *display)
> > NULL);
> > gtk_stack_add_named(d->stack, area, "gl-area");
> > #endif
> > -#ifdef HAVE_GSTVIDEO
> > area = gtk_drawing_area_new();
> > gtk_stack_add_named(d->stack, area, "gst-area");
> > g_object_connect(area,
> > "signal::draw", gst_draw_event, display,
> > "signal::size-allocate", gst_size_allocate, display,
> > NULL);
> > -#endif
> > +
> > d->label = gtk_label_new(NULL);
> > gtk_label_set_selectable(GTK_LABEL(d->label), true);
> > gtk_stack_add_named(d->stack, d->label, "label");
> > @@ -2140,9 +2137,7 @@ static void unrealize(GtkWidget *widget)
> > spice_egl_unrealize_display(display);
> > }
> > #endif
> > -#ifdef HAVE_GSTVIDEO
> > g_weak_ref_set(&display->priv->overlay_weak_ref, NULL);
> > -#endif
> >
> > GTK_WIDGET_CLASS(spice_display_parent_class)->unrealize(widget);
> > }
> > @@ -2570,7 +2565,7 @@ static void queue_draw_area(SpiceDisplay *display, gint x, gint y,
> > x, y, width, height);
> > }
> >
> > -#if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
> > +#if defined(GDK_WINDOWING_X11)
> > static void gst_sync_bus_call(GstBus *bus, GstMessage *msg, SpiceDisplay *display)
> > {
> > switch(GST_MESSAGE_TYPE(msg)) {
> > @@ -2599,7 +2594,6 @@ static void gst_sync_bus_call(GstBus *bus, GstMessage *msg, SpiceDisplay *displa
> > }
> > #endif
> >
> > -#ifdef HAVE_GSTVIDEO
> > static gboolean gst_draw_event(GtkWidget *widget, cairo_t *cr, gpointer data)
> > {
> > SpiceDisplay *display = SPICE_DISPLAY(data);
> > @@ -2629,14 +2623,13 @@ static void gst_size_allocate(GtkWidget *widget, GdkRectangle *a, gpointer data)
> > gst_object_unref(overlay);
> > }
> > }
> > -#endif
> >
> > /* This callback should pass to the widget a pointer of the pipeline
> > * so that we can set pipeline and overlay related calls from here.
> > */
> > static gboolean set_overlay(SpiceChannel *channel, void* pipeline_ptr, SpiceDisplay *display)
> > {
> > -#if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
> > +#if defined(GDK_WINDOWING_X11)
> > SpiceDisplayPrivate *d = display->priv;
> >
> > /* GstVideoOverlay is currently used only under x */
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel