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