[Spice-devel] [PATCH v6 06/26] server: Add a GStreamer 1.0 MJPEG video encoder and use it by default

[Francois Gouget]

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 at codeweavers.com>