On Thu, 22 Oct 2015, Christophe Fergeau wrote: [...] > > +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 > > + PKG_CHECK_MODULES(GSTREAMER_1_0, [gstreamer-1.0, gstreamer-app-1.0], > > + [enable_gstreamer="yes" > > + have_gstreamer_1_0="yes"], > > + [have_gstreamer_1_0="no"]) > > + if test "x$have_gstreamer_1_0" = "xyes"; then > > + AC_SUBST(GSTREAMER_1_0_CFLAGS) > > + AC_SUBST(GSTREAMER_1_0_LIBS) > > + AS_VAR_APPEND([SPICE_REQUIRES], [" gstreamer-1.0 gstreamer-app-1.0"]) > > + AC_DEFINE([HAVE_GSTREAMER_1_0], [1], [Define if supporting GStreamer 1.0]) > > + elif 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 > > +else > > + have_gstreamer_1_0="no" > > +fi > > +AM_CONDITIONAL(SUPPORT_GSTREAMER_1_0, test "x$have_gstreamer_1_0" = "xyes") > > + > > Any chance this check can be moved to spice-common/common/spice-deps.m4 > and shared with spice-gtk? Or are they too different to make that > practical? There are relatively important differences between the Spice server (support for both GStreamer 1.0 and 0.10), and the Spice client (GStreamer 1.0 but for audio and video). Still I think I found some way to share most of the boilerplate. See the following patches: * [common 02/11] build-sys: Add SPICE_CHECK_GSTREAMER() http://lists.freedesktop.org/archives/spice-devel/2015-November/023049.html * [common 03/11] build-sys: Add SPICE_CHECK_GSTREAMER_ELEMENTS() http://lists.freedesktop.org/archives/spice-devel/2015-November/023050.html > > + /* 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_assert(dst - buffer == stream_stride * height); > > Do we have to have this assert? Could it be g_warn_if_fail() or > similar? Eh! As I initially wrote the Spice server code uses spice_assert() rather than g_return_xxx() so this is following current practice. But it does seem current practice is currently changing from under my feet :-/ > > + const uint32_t stream_stride = (src->right - src->left) * encoder->format->bpp / 8; > > + uint32_t len = stream_stride * height; > > + GstBuffer *buffer = gst_buffer_new_and_alloc(len); > > + GstMapInfo map; > > + gst_buffer_map(buffer, &map, GST_MAP_WRITE); > > + uint8_t *dst = map.data; > > + > > + /* Note that we should not reorder the lines, even if top_down is false. > > + * It just changes the number of lines to skip at the start of the bitmap. > > + */ > > + const uint32_t skip_lines = top_down ? src->top : bitmap->y - (src->bottom - 0); > > - 0 ? When top_down is false the height of the frame is (src->bottom - 0), so the first line to encode is bitmap->y - (src->bottom - 0). Sure it could be written as 'bitmap->y - src->bottom' but I'm not sure that would be clearer. In fact I don't see any way to make top_down==false clear. > > + GError *err = NULL; > > + if (!gst_init_check(NULL, NULL, &err)) { > > + spice_warning("GStreamer error: %s", err->message); > > + g_clear_error(&err); > > + return NULL; > > + } > > This gst_init() call also does not need to be done every time > gstreamer_encoder_new() is called, it can be just once. Only the first call takes a bit of time to run (7ms). All following calls take less than 0.5ms (i.e. they showed up as 0ms in my traces). So I don't think we should worry about that. > > +/* A helper for red_display_create_stream(). */ > > +static VideoEncoder* red_display_create_video_encoder(uint64_t starting_bit_rate, > > + VideoEncoderRateControlCbs *cbs, > > + void *cbs_opaque) > > +{ > > +#ifdef HAVE_GSTREAMER_1_0 > > + VideoEncoder* video_encoder = gstreamer_encoder_new(starting_bit_rate, cbs, cbs_opaque); > > + if (video_encoder) { > > + return video_encoder; > > + } > > +#endif > > + /* Use the builtin MJPEG video encoder as a fallback */ > > + return mjpeg_encoder_new(starting_bit_rate, cbs, cbs_opaque); > > +} > > + > > I'd add a note to the commit log that this is currently not checking > client's capabilities. Actually the client capabilities patch comes later in the series so it would be a bit premature. Though given that they go to separate repositories order is a bit undefined obviously. -- Francois Gouget <fgouget@xxxxxxxxxxxxxxx> _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel