[Spice-devel] [PATCH spice-gtk v2 04/15] build-sys: drop gstvideo option, make it required

Marc-André Lureau marcandre.lureau at gmail.com
Sun Jan 13 16:36:30 UTC 2019


On Sun, Jan 13, 2019 at 7:07 PM Snir Sheriber <ssheribe at redhat.com> wrote:
>
> Hi,
>
>
> On 1/9/19 12:09 PM, Frediano Ziglio wrote:
> > From: Marc-André Lureau<marcandre.lureau at redhat.com>
> >
> > 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 at redhat.com>
> > ---
> >   .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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau


More information about the Spice-devel mailing list