Re: [spice v10 09/27] server: Let the video encoder manage the compressed buffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hey,

On Tue, Mar 01, 2016 at 04:53:03PM +0100, Francois Gouget wrote:
> This way the video encoder is not forced to use malloc()/free().
> This also allows more flexibility in how the video encoder manages the
> buffer which allows for a zero-copy implementation in both video
> encoders.

Yes, this is quite nice :)

> 
> Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> ---
>  server/dcc-send.c          | 25 +++++++-------
>  server/dcc.c               |  5 ---
>  server/dcc.h               |  3 --
>  server/gstreamer-encoder.c | 55 +++++++++++++++++++-----------
>  server/mjpeg-encoder.c     | 85 +++++++++++++++++++++++++++++-----------------
>  server/video-encoder.h     | 26 ++++++++++----
>  6 files changed, 122 insertions(+), 77 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 6cf16e2..68ea085 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -1649,6 +1649,12 @@ static void red_lossy_marshall_qxl_draw_text(RedChannelClient *rcc,
>      }
>  }
>  
> +static void red_release_video_encoder_buffer(uint8_t *data, void *opaque)
> +{
> +    VideoBuffer *buffer = (VideoBuffer*)opaque;
> +    buffer->free(buffer);
> +}
> +
>  static int red_marshall_stream_data(RedChannelClient *rcc,
>                                      SpiceMarshaller *base_marshaller, Drawable *drawable)
>  {
> @@ -1657,7 +1663,6 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>      Stream *stream = drawable->stream;
>      SpiceImage *image;
>      uint32_t frame_mm_time;
> -    int n;
>      int width, height;
>      int ret;
>  
> @@ -1689,7 +1694,6 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>  
>      StreamAgent *agent = &dcc->stream_agents[get_stream_id(display, stream)];
>      uint64_t time_now = spice_get_monotonic_time_ns();
> -    size_t outbuf_size;
>  
>      if (!dcc->use_video_encoder_rate_control) {
>          if (time_now - agent->last_send_time < (1000 * 1000 * 1000) / agent->fps) {
> @@ -1701,18 +1705,16 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>          }
>      }
>  
> +    VideoBuffer *outbuf;
>      /* workaround for vga streams */
>      frame_mm_time =  drawable->red_drawable->mm_time ?
>                          drawable->red_drawable->mm_time :
>                          reds_get_mm_time();
> -    outbuf_size = dcc->send_data.stream_outbuf_size;
>      ret = agent->video_encoder->encode_frame(agent->video_encoder,
>                                               frame_mm_time,
>                                               &image->u.bitmap, width, height,
>                                               &drawable->red_drawable->u.copy.src_area,
> -                                             stream->top_down,
> -                                             &dcc->send_data.stream_outbuf,
> -                                             &outbuf_size, &n);
> +                                             stream->top_down, &outbuf);
>      switch (ret) {
>      case VIDEO_ENCODER_FRAME_DROP:
>          spice_assert(dcc->use_video_encoder_rate_control);
> @@ -1728,7 +1730,6 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>          spice_error("bad return value (%d) from VideoEncoder::encode_frame", ret);
>          return FALSE;
>      }
> -    dcc->send_data.stream_outbuf_size = outbuf_size;
>  
>      if (!drawable->sized_stream) {
>          SpiceMsgDisplayStreamData stream_data;
> @@ -1737,7 +1738,7 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>  
>          stream_data.base.id = get_stream_id(display, stream);
>          stream_data.base.multi_media_time = frame_mm_time;
> -        stream_data.data_size = n;
> +        stream_data.data_size = outbuf->size;
>  
>          spice_marshall_msg_display_stream_data(base_marshaller, &stream_data);
>      } else {
> @@ -1747,7 +1748,7 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>  
>          stream_data.base.id = get_stream_id(display, stream);
>          stream_data.base.multi_media_time = frame_mm_time;
> -        stream_data.data_size = n;
> +        stream_data.data_size = outbuf->size;
>          stream_data.width = width;
>          stream_data.height = height;
>          stream_data.dest = drawable->red_drawable->bbox;
> @@ -1756,12 +1757,12 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>          rect_debug(&stream_data.dest);
>          spice_marshall_msg_display_stream_data_sized(base_marshaller, &stream_data);
>      }
> -    spice_marshaller_add_ref(base_marshaller,
> -                             dcc->send_data.stream_outbuf, n);
> +    spice_marshaller_add_ref_full(base_marshaller, outbuf->data, outbuf->size,
> +                                  &red_release_video_encoder_buffer, outbuf);
>      agent->last_send_time = time_now;
>  #ifdef STREAM_STATS
>      agent->stats.num_frames_sent++;
> -    agent->stats.size_sent += n;
> +    agent->stats.size_sent += outbuf->size;
>      agent->stats.end = frame_mm_time;
>  #endif
>  
> diff --git a/server/dcc.c b/server/dcc.c
> index df650c9..88196f9 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -385,10 +385,6 @@ DisplayChannelClient *dcc_new(DisplayChannel *display,
>      // TODO: tune quality according to bandwidth
>      dcc->jpeg_quality = 85;
>  
> -    size_t stream_buf_size;
> -    stream_buf_size = 32*1024;
> -    dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);
> -    dcc->send_data.stream_outbuf_size = stream_buf_size;
>      dcc->send_data.free_list.res =
>          spice_malloc(sizeof(SpiceResourceList) +
>                       DISPLAY_FREE_LIST_DEFAULT_SIZE * sizeof(SpiceResourceID));
> @@ -504,7 +500,6 @@ void dcc_stop(DisplayChannelClient *dcc)
>      dcc->pixmap_cache = NULL;
>      dcc_release_glz(dcc);
>      dcc_palette_cache_reset(dcc);
> -    free(dcc->send_data.stream_outbuf);
>      free(dcc->send_data.free_list.res);
>      dcc_destroy_stream_agents(dcc);
>      dcc_encoders_free(dcc);
> diff --git a/server/dcc.h b/server/dcc.h
> index c2ef871..35cb2a4 100644
> --- a/server/dcc.h
> +++ b/server/dcc.h
> @@ -88,9 +88,6 @@ struct DisplayChannelClient {
>      uint32_t palette_cache_items;
>  
>      struct {
> -        uint32_t stream_outbuf_size;
> -        uint8_t *stream_outbuf; // caution stream buffer is also used as compress bufs!!!
> -
>          FreeList free_list;
>          uint64_t pixmap_cache_items[MAX_DRAWABLE_PIXMAP_CACHE_ITEMS];
>          int num_pixmap_cache_items;
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 3dfbdfb..17dd0cb 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -37,6 +37,12 @@ typedef struct {
>      uint32_t bpp;
>  } SpiceFormatForGStreamer;
>  
> +typedef struct SpiceGstVideoBuffer {
> +    VideoBuffer base;
> +    GstBuffer *gst_buffer;
> +    GstMapInfo map;
> +} SpiceGstVideoBuffer;
> +
>  typedef struct SpiceGstEncoder {
>      VideoEncoder base;
>  
> @@ -80,6 +86,23 @@ typedef struct SpiceGstEncoder {
>  } SpiceGstEncoder;
>  
>  
> +/* ---------- The SpiceGstVideoBuffer implementation ---------- */
> +
> +static void gst_video_buffer_free(VideoBuffer *video_buffer)
> +{
> +    SpiceGstVideoBuffer *buffer = (SpiceGstVideoBuffer*)video_buffer;
> +    gst_buffer_unref(buffer->gst_buffer);

You need a if (buffer->gst_buffer != NULL) test.
pull_compressed_buffer() can call this method with a NULL gst_buffer in
error cases, and gst_buffer_unref() warns on NULL.

> +    free(buffer);
> +}
> +
> +static SpiceGstVideoBuffer* create_gst_video_buffer(void)
> +{
> +    SpiceGstVideoBuffer *buffer = spice_new0(SpiceGstVideoBuffer, 1);
> +    buffer->base.free = &gst_video_buffer_free;
> +    return buffer;
> +}
> +
> +
>  /* ---------- Miscellaneous SpiceGstEncoder helpers ---------- */
>  
>  static inline double get_mbps(uint64_t bit_rate)
> @@ -458,29 +481,22 @@ static int push_raw_frame(SpiceGstEncoder *encoder, const SpiceBitmap *bitmap,
>  
>  /* A helper for spice_gst_encoder_encode_frame() */
>  static int pull_compressed_buffer(SpiceGstEncoder *encoder,
> -                                  uint8_t **outbuf, size_t *outbuf_size,
> -                                  int *data_size)
> +                                  VideoBuffer **outbuf)
>  {
> -    spice_return_val_if_fail(outbuf && outbuf_size, VIDEO_ENCODER_FRAME_UNSUPPORTED);

Maybe g_return_val_if_fail(video_buffer != NULL, VIDEO_ENCODER_FRAME_UNSUPPORTED); ?

> -
>      GstSample *sample = gst_app_sink_pull_sample(encoder->appsink);
>      if (sample) {
> -        GstMapInfo map;
> -        GstBuffer *buffer = gst_sample_get_buffer(sample);
> -        if (buffer && gst_buffer_map(buffer, &map, GST_MAP_READ)) {
> -            int size = gst_buffer_get_size(buffer);
> -            if (!*outbuf || *outbuf_size < size) {
> -                free(*outbuf);
> -                *outbuf = spice_malloc(size);
> -                *outbuf_size = size;
> -            }
> -            /* TODO Try to avoid this copy by changing the GstBuffer handling */
> -            memcpy(*outbuf, map.data, size);
> -            *data_size = size;
> -            gst_buffer_unmap(buffer, &map);
> +        SpiceGstVideoBuffer *buffer = create_gst_video_buffer();
> +        buffer->gst_buffer = gst_sample_get_buffer(sample);
> +        if (buffer->gst_buffer &&
> +            gst_buffer_map(buffer->gst_buffer, &buffer->map, GST_MAP_READ)) {
> +            buffer->base.data = buffer->map.data;
> +            buffer->base.size = gst_buffer_get_size(buffer->gst_buffer);
> +            *outbuf = (VideoBuffer*)buffer;
> +            gst_buffer_ref(buffer->gst_buffer);
>              gst_sample_unref(sample);
>              return VIDEO_ENCODER_FRAME_ENCODE_DONE;
>          }
> +        buffer->base.free((VideoBuffer*)buffer);
>          gst_sample_unref(sample);

A *video_buffer = NULL; would not hurt in error cases in case the
caller is careless and tries to use the result without checking for
errors.

>      }
>      spice_debug("failed to pull the compressed buffer");
> @@ -502,8 +518,7 @@ static int spice_gst_encoder_encode_frame(VideoEncoder *video_encoder,
>                                            const SpiceBitmap *bitmap,
>                                            int width, int height,
>                                            const SpiceRect *src, int top_down,
> -                                          uint8_t **outbuf, size_t *outbuf_size,
> -                                          int *data_size)
> +                                          VideoBuffer **outbuf)
>  {
>      SpiceGstEncoder *encoder = (SpiceGstEncoder*)video_encoder;
>  
> @@ -531,7 +546,7 @@ static int spice_gst_encoder_encode_frame(VideoEncoder *video_encoder,
>  
>      int rc = push_raw_frame(encoder, bitmap, src, top_down);
>      if (rc == VIDEO_ENCODER_FRAME_ENCODE_DONE) {
> -        rc = pull_compressed_buffer(encoder, outbuf, outbuf_size, data_size);
> +        rc = pull_compressed_buffer(encoder, outbuf);
>      }
>      return rc;
>  }
> diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> index 7e66106..57ce3c3 100644
> --- a/server/mjpeg-encoder.c
> +++ b/server/mjpeg-encoder.c
> @@ -930,26 +949,30 @@ static int mjpeg_encoder_encode_frame(VideoEncoder *video_encoder,
>                                        const SpiceBitmap *bitmap,
>                                        int width, int height,
>                                        const SpiceRect *src, int top_down,
> -                                      uint8_t **outbuf, size_t *outbuf_size,
> -                                      int *data_size)
> +                                      VideoBuffer **outbuf)
>  {
>      MJpegEncoder *encoder = (MJpegEncoder*)video_encoder;
> +    MJpegVideoBuffer *buffer = create_mjpeg_video_buffer();
> +    if (!buffer) {
> +        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +    }
>  
>      int ret = mjpeg_encoder_start_frame(encoder, bitmap->format,
>                                          width, height,
> -                                        outbuf, outbuf_size,
> -                                        frame_mm_time);
> -    if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
> -        return ret;
> +                                        buffer, frame_mm_time);
> +    if (ret == VIDEO_ENCODER_FRAME_ENCODE_DONE) {
> +        if (encode_frame(encoder, src, bitmap, top_down)) {
> +            buffer->base.size = mjpeg_encoder_end_frame(encoder);
> +            *outbuf = (VideoBuffer*)buffer;
> +        } else {
> +            ret = VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +        }
>      }
>  
> -    if (!encode_frame(encoder, src, bitmap, top_down)) {
> -        return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> +    if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
> +        buffer->base.free((VideoBuffer*)buffer);
>      }
> -
> -    *data_size = mjpeg_encoder_end_frame(encoder);
> -
> -    return VIDEO_ENCODER_FRAME_ENCODE_DONE;
> +    return ret;
>  }

Same comment here about setting *video_buffer to NULL in error cases.

Apart from these minor issues,
Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>

Christophe

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]