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.
The current implementations also ensure that there is no reallocation of the VideoBuffer structure.
Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
---
This is a new patch which makes it possible to avoid copying the output
buffer in the GStreamer video encoder.
Before:
- Spice was handling the initial output buffer allocation, and was also
freeing it when destroying the stream.
- When calling encode_frame() Spice knew that the buffer was no
longer needed for the previous frame. So it just reused it, saving on
reallocations.
- The video encoder was responsible for reallocating the buffer when
needed. In the mjpeg_encoder case this was actually done by
the jpeg library, and thus out of the hands of the encoder itself.
- The GStreamer pipeline allocates its own output buffer which forced
the video encoder to copy it to the Spice-provided output buffer.
Now:
- The video encoders do all the output buffer allocations /
deallocations and give a pointer to the buffer to the rest of the
Spice code.
- The video encoders could assume that the buffer is no longer used by
the time a new call to encode_frame() is made. But I prefer to
decouple this a bit more. So the Spice code is reponsible for
indicating when it no longer needs a given output buffer. This means
it could hold on to more than one output buffer at a time (for
buffering or whatever).
- The video encoders optimize the case where the previous frame's
output buffer has been freed before the next encode_frame() call.
- The GStreamer video encoder can now pass the (wrapped) GStreamer
output buffer to the Spice code and thus avoid a copy. When Spice
releases the output buffer the corresponding GStreamer buffer it
released.
server/gstreamer_encoder.c | 81 ++++++++++++++++++++++++++--------
server/mjpeg_encoder.c | 106 +++++++++++++++++++++++++++++++--------------
server/red_worker.c | 31 ++++++-------
server/video_encoder.h | 45 ++++++++++++++++---
4 files changed, 187 insertions(+), 76 deletions(-)
diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
index 140261e..f7938ae 100644
--- a/server/gstreamer_encoder.c
+++ b/server/gstreamer_encoder.c
@@ -26,6 +26,8 @@
#include "red_common.h"
+typedef struct GstVideoBuffer GstVideoBuffer;
+#define VIDEO_BUFFER_T GstVideoBuffer
typedef struct GstEncoder GstEncoder;
#define VIDEO_ENCODER_T GstEncoder
#include "video_encoder.h"
@@ -44,6 +46,12 @@ typedef struct {
int red_mask;
} SpiceFormatForGStreamer;
+struct GstVideoBuffer {
+ VideoBuffer base;
+ GstBuffer *gst_buffer;
+ gboolean persistent;
+};
+
struct GstEncoder {
VideoEncoder base;
@@ -72,6 +80,9 @@ struct GstEncoder {
GstElement *gstenc;
GstAppSink *appsink;
+ /* The default video buffer */
+ GstVideoBuffer *default_buffer;
+
/* The frame counter for GStreamer buffers */
uint32_t frame;
@@ -83,6 +94,35 @@ struct GstEncoder {
};
+/* ---------- The GstVideoBuffer implementation ---------- */
+
+static inline GstVideoBuffer* gst_video_buffer_ref(GstVideoBuffer *buffer)
+{
+ buffer->base.ref_count++;
+ return buffer;
+}
+
+static void gst_video_buffer_unref(GstVideoBuffer *buffer)
+{
+ if (--buffer->base.ref_count == 0) {
+ gst_buffer_unref(buffer->gst_buffer);
+ if (!buffer->persistent) {
+ free(buffer);
+ }
+ }
+}
+
+static GstVideoBuffer* create_gst_video_buffer(gboolean persistent)
+{
+ GstVideoBuffer *buffer = spice_new0(GstVideoBuffer, 1);
+ buffer->base.ref = &gst_video_buffer_ref;
+ buffer->base.unref = &gst_video_buffer_unref;
+ buffer->persistent = persistent;
+ buffer->base.ref_count = persistent ? 0 : 1;
+ return buffer;
+}
+
+
/* ---------- Miscellaneous GstEncoder helpers ---------- */
static inline double get_mbps(uint64_t bit_rate)
@@ -358,24 +398,24 @@ static int push_raw_frame(GstEncoder *encoder, const SpiceBitmap *bitmap,
}
/* A helper for gst_encoder_encode_frame(). */
-static int pull_compressed_buffer(GstEncoder *encoder,
- uint8_t **outbuf, size_t *outbuf_size,
- int *data_size)
+static int pull_compressed_buffer(GstEncoder *encoder, GstVideoBuffer **buffer)
{
- GstBuffer *buffer = gst_app_sink_pull_buffer(encoder->appsink);
- if (buffer) {
- int len = GST_BUFFER_SIZE(buffer);
- spice_assert(outbuf && outbuf_size);
- if (!*outbuf || *outbuf_size < len) {
- *outbuf = spice_realloc(*outbuf, len);
- *outbuf_size = len;
- }
- /* TODO Try to avoid this copy by changing the GstBuffer handling */
- memcpy(*outbuf, GST_BUFFER_DATA(buffer), len);
- gst_buffer_unref(buffer);
- *data_size = len;
+ GstVideoBuffer *video_buffer;
+ if (encoder->default_buffer->base.ref_count == 0) {
+ /* The default buffer is unused so we can reuse it. */
+ video_buffer = encoder->default_buffer;
+ video_buffer->base.ref(video_buffer);
+ } else {
+ video_buffer = create_gst_video_buffer(FALSE);
+ }
+ video_buffer->gst_buffer = gst_app_sink_pull_buffer(encoder->appsink);
+ if (video_buffer->gst_buffer) {
+ video_buffer->base.data = GST_BUFFER_DATA(video_buffer->gst_buffer);
+ video_buffer->base.size = GST_BUFFER_SIZE(video_buffer->gst_buffer);
+ *buffer = video_buffer;
return VIDEO_ENCODER_FRAME_ENCODE_DONE;
}
+ video_buffer->base.unref(video_buffer);
return VIDEO_ENCODER_FRAME_UNSUPPORTED;
}
@@ -384,6 +424,11 @@ static int pull_compressed_buffer(GstEncoder *encoder,
static void gst_encoder_destroy(GstEncoder *encoder)
{
+ if (encoder->default_buffer->base.ref_count == 0) {
+ free(encoder->default_buffer);
+ } else {
+ encoder->default_buffer->persistent = FALSE;
+ }
reset_pipeline(encoder);
free(encoder);
}
@@ -393,8 +438,7 @@ static int gst_encoder_encode_frame(GstEncoder *encoder,
int width, int height,
const SpiceRect *src, int top_down,
uint32_t frame_mm_time,
- uint8_t **outbuf, size_t *outbuf_size,
- int *data_size)
+ GstVideoBuffer **buffer)
{
if (width != encoder->width || height != encoder->height ||
encoder->spice_format != bitmap->format) {
@@ -417,7 +461,7 @@ static int gst_encoder_encode_frame(GstEncoder *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, buffer);
}
return rc;
}
@@ -482,6 +526,7 @@ GstEncoder *create_gstreamer_encoder(SpiceVideoCodecType codec_type, uint64_t st
encoder->base.get_bit_rate = &gst_encoder_get_bit_rate;
encoder->base.get_stats = &gst_encoder_get_stats;
encoder->base.codec_type = codec_type;
+ encoder->default_buffer = create_gst_video_buffer(TRUE);
if (cbs) {
encoder->cbs = *cbs;
diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index f8990a5..c7fb884 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -21,6 +21,8 @@
#include "red_common.h"
+typedef struct MJpegVideoBuffer MJpegVideoBuffer;
+#define VIDEO_BUFFER_T MJpegVideoBuffer
typedef struct MJpegEncoder MJpegEncoder;
#define VIDEO_ENCODER_T MJpegEncoder
#include "video_encoder.h"
@@ -73,6 +75,9 @@ static const int mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
*/
#define MJPEG_WARMUP_TIME 3000LL // 3 sec
+/* The compressed buffer initial size. */
+#define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
+
enum {
MJPEG_QUALITY_EVAL_TYPE_SET,
MJPEG_QUALITY_EVAL_TYPE_UPGRADE,
@@ -157,12 +162,19 @@ typedef struct MJpegEncoderRateControl {
uint64_t warmup_start_time;
} MJpegEncoderRateControl;
+struct MJpegVideoBuffer {
+ VideoBuffer base;
+ size_t maxsize;
+};
+
struct MJpegEncoder {
VideoEncoder base;
uint8_t *row;
uint32_t row_size;
int first_frame;
+ MJpegVideoBuffer* default_buffer;
+
struct jpeg_compress_struct cinfo;
struct jpeg_error_mgr jerr;
@@ -184,6 +196,31 @@ static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
uint64_t byte_rate,
uint32_t latency);
+static inline MJpegVideoBuffer* mjpeg_buffer_ref(MJpegVideoBuffer *buffer)
+{
+ buffer->base.ref_count++;
+ return buffer;
+}
+
+static void mjpeg_buffer_unref(MJpegVideoBuffer *buffer)
+{
+ if (--buffer->base.ref_count == 0) {
+ free(buffer->base.data);
+ free(buffer);
+ }
+}
+
+static MJpegVideoBuffer* create_mjpeg_video_buffer(void)
+{
+ MJpegVideoBuffer *buffer = spice_new0(MJpegVideoBuffer, 1);
+ buffer->base.ref = &mjpeg_buffer_ref;
+ buffer->base.unref = &mjpeg_buffer_unref;
+ buffer->maxsize = MJPEG_INITIAL_BUFFER_SIZE;
+ buffer->base.data = malloc(buffer->maxsize);
+ buffer->base.ref_count = 1;
+ return buffer;
+}
+
static inline int rate_control_is_active(MJpegEncoder* encoder)
{
return encoder->cbs.get_roundtrip_ms != NULL;
@@ -192,6 +229,7 @@ static inline int rate_control_is_active(MJpegEncoder* encoder)
static void mjpeg_encoder_destroy(MJpegEncoder *encoder)
{
jpeg_destroy_compress(&encoder->cinfo);
+ encoder->default_buffer->base.unref(encoder->default_buffer);
free(encoder->row);
free(encoder);
}
@@ -284,23 +322,20 @@ static void term_mem_destination(j_compress_ptr cinfo)
/*
* Prepare for output to a memory buffer.
- * The caller may supply an own initial buffer with appropriate size.
- * Otherwise, or when the actual data output exceeds the given size,
- * the library adapts the buffer size as necessary.
- * The standard library functions malloc/free are used for allocating
- * larger memory, so the buffer is available to the application after
- * finishing compression, and then the application is responsible for
- * freeing the requested memory.
+ * The caller must supply its own initial buffer and size.
+ * When the actual data output exceeds the given size, the library
+ * will adapt the buffer size as necessary using the malloc()/free()
+ * functions. The buffer is available to the application after the
+ * compression and the application is then responsible for freeing it.
*/
-
static void
spice_jpeg_mem_dest(j_compress_ptr cinfo,
unsigned char ** outbuffer, size_t * outsize)
{
mem_destination_mgr *dest;
-#define OUTPUT_BUF_SIZE 4096 /* choose an efficiently fwrite'able size */
- if (outbuffer == NULL || outsize == NULL) /* sanity check */
+ if (outbuffer == NULL || *outbuffer == NULL ||
+ outsize == NULL || *outsize == 0) /* sanity check */
ERREXIT(cinfo, JERR_BUFFER_SIZE);
/* The destination object is made permanent so that multiple JPEG images
@@ -316,13 +351,6 @@ spice_jpeg_mem_dest(j_compress_ptr cinfo,
dest->pub.term_destination = term_mem_destination;
dest->outbuffer = outbuffer;
dest->outsize = outsize;
- if (*outbuffer == NULL || *outsize == 0) {
- /* Allocate initial buffer */
- *outbuffer = malloc(OUTPUT_BUF_SIZE);
- if (*outbuffer == NULL)
- ERREXIT1(cinfo, JERR_OUT_OF_MEMORY, 10);
- *outsize = OUTPUT_BUF_SIZE;
- }
dest->pub.next_output_byte = dest->buffer = *outbuffer;
dest->pub.free_in_buffer = dest->bufsize = *outsize;
@@ -699,7 +727,7 @@ static void mjpeg_encoder_adjust_fps(MJpegEncoder *encoder, uint64_t now)
static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
SpiceBitmapFmt format,
int width, int height,
- uint8_t **dest, size_t *dest_len,
+ MJpegVideoBuffer *buffer,
uint32_t frame_mm_time)
{
uint32_t quality;
@@ -783,7 +811,7 @@ static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
}
}
- spice_jpeg_mem_dest(&encoder->cinfo, dest, dest_len);
+ spice_jpeg_mem_dest(&encoder->cinfo, &buffer->base.data, &buffer->maxsize);
encoder->cinfo.image_width = width;
encoder->cinfo.image_height = height;
@@ -924,23 +952,34 @@ static int mjpeg_encoder_encode_frame(MJpegEncoder *encoder,
int width, int height,
const SpiceRect *src,
int top_down, uint32_t frame_mm_time,
- uint8_t **outbuf, size_t *outbuf_size,
- int *data_size)
+ MJpegVideoBuffer **buffer)
{
- int ret;
-
- 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;
-
- if (!encode_mjpeg_frame(encoder, src, bitmap, top_down))
- return VIDEO_ENCODER_FRAME_UNSUPPORTED;
+ MJpegVideoBuffer *video_buffer;
+ if (encoder->default_buffer->base.ref_count == 1) {
+ /* Only the MJpegEncoder is referencing the default buffer
+ * so we can reuse it.
+ */
+ video_buffer = encoder->default_buffer;
+ video_buffer->base.ref(video_buffer);
+ } else {
+ video_buffer = create_mjpeg_video_buffer();
+ }
- *data_size = mjpeg_encoder_end_frame(encoder);
+ int ret = mjpeg_encoder_start_frame(encoder, bitmap->format, width, height,
+ video_buffer, frame_mm_time);
+ if (ret == VIDEO_ENCODER_FRAME_ENCODE_DONE) {
+ if (encode_mjpeg_frame(encoder, src, bitmap, top_down)) {
+ video_buffer->base.size = mjpeg_encoder_end_frame(encoder);
+ *buffer = video_buffer;
+ } else {
+ ret = VIDEO_ENCODER_FRAME_UNSUPPORTED;
+ }
+ }
- return VIDEO_ENCODER_FRAME_ENCODE_DONE;
+ if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
+ video_buffer->base.unref(video_buffer);
+ }
+ return ret;
}
@@ -1352,6 +1391,7 @@ MJpegEncoder *create_mjpeg_encoder(SpiceVideoCodecType codec_type,
encoder->base.get_stats = &mjpeg_encoder_get_stats;
encoder->base.codec_type = codec_type;
encoder->first_frame = TRUE;
+ encoder->default_buffer = create_mjpeg_video_buffer();
encoder->rate_control.byte_rate = starting_bit_rate / 8;
encoder->starting_bit_rate = starting_bit_rate;
diff --git a/server/red_worker.c b/server/red_worker.c
index 7d8242c..0f88c14 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -700,9 +700,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!!!
-
RedCompressBuf *used_compress_bufs;
FreeList free_list;
@@ -8525,6 +8522,12 @@ static inline void display_begin_send_message(RedChannelClient *rcc)
red_channel_client_begin_send_message(rcc);
}
+static void red_release_video_encoder_buffer(uint8_t *data, void *opaque)
+{
+ VideoBuffer *buffer = (VideoBuffer*)opaque;
+ buffer->unref(buffer);
+}
+
static inline int red_marshall_stream_data(RedChannelClient *rcc,
SpiceMarshaller *base_marshaller, Drawable *drawable)
{
@@ -8534,7 +8537,7 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
SpiceImage *image;
RedWorker *worker = dcc->common.worker;
uint32_t frame_mm_time;
- int n;
+ VideoBuffer *outbuf;
int width, height;
int ret;
@@ -8567,7 +8570,6 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
StreamAgent *agent = &dcc->stream_agents[get_stream_id(worker, stream)];
uint64_t time_now = red_now();
- size_t outbuf_size;
if (!dcc->use_video_encoder_rate_control) {
if (time_now - agent->last_send_time < (1000 * 1000 * 1000) / agent->fps) {
@@ -8584,12 +8586,11 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
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,
&image->u.bitmap, width, height,
&drawable->red_drawable->u.copy.src_area,
stream->top_down, frame_mm_time,
- &dcc->send_data.stream_outbuf, &outbuf_size, &n);
+ &outbuf);
switch (ret) {
case VIDEO_ENCODER_FRAME_DROP:
@@ -8606,7 +8607,6 @@ static inline 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;
@@ -8615,7 +8615,7 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
stream_data.base.id = get_stream_id(worker, 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 {
@@ -8625,7 +8625,7 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
stream_data.base.id = get_stream_id(worker, 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;
@@ -8634,12 +8634,12 @@ static inline 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
@@ -9401,7 +9401,6 @@ static void display_channel_client_on_disconnect(RedChannelClient *rcc)
red_release_pixmap_cache(dcc);
red_release_glz(dcc);
red_reset_palette_cache(dcc);
- free(dcc->send_data.stream_outbuf);
red_display_reset_compress_buf(dcc);
free(dcc->send_data.free_list.res);
red_display_destroy_streams_agents(dcc);
@@ -10772,7 +10771,6 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
{
DisplayChannel *display_channel;
DisplayChannelClient *dcc;
- size_t stream_buf_size;
if (!worker->display_channel) {
spice_warning("Display channel was not created");
@@ -10788,9 +10786,6 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
return;
}
spice_info("New display (client %p) dcc %p stream %p", client, dcc, stream);
- stream_buf_size = 32*1024;
- dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);
- dcc->send_data.stream_outbuf_size = stream_buf_size;
red_display_init_glz_data(dcc);
dcc->send_data.free_list.res =
diff --git a/server/video_encoder.h b/server/video_encoder.h
index 6816293..9f2409f 100644
--- a/server/video_encoder.h
+++ b/server/video_encoder.h
@@ -23,6 +23,38 @@
#include "red_common.h"
+
+typedef struct VideoBuffer VideoBuffer;
+#ifndef VIDEO_BUFFER_T
+# define VIDEO_BUFFER_T VideoBuffer
+#endif
+
+/* A structure containing the data for a compressed frame. See encode_frame(). */
+struct VideoBuffer {
+ /* A pointer to the compressed frame data. */
+ uint8_t *data;
+
+ /* The size of the compressed frame in bytes. */
+ uint32_t size;
+
+ /* The buffer's reference count. */
+ uint32_t ref_count;
+
+ /* Increments the video buffer's reference count and returns the new count.
+ *
+ * @buffer: The video buffer.
+ * @return: The new reference count.
+ */
+ VIDEO_BUFFER_T* (*ref)(VIDEO_BUFFER_T *buffer);
+
+ /* Decrements the video buffer's reference count and deallocates it as
+ * appropriate.
+ *
+ * @buffer: The video buffer.
+ */
+ void (*unref)(VIDEO_BUFFER_T *buffer);
+};
+
enum {
VIDEO_ENCODER_FRAME_UNSUPPORTED = -1,
VIDEO_ENCODER_FRAME_DROP,
@@ -52,11 +84,10 @@ struct VideoEncoder {
* @height: The height of the video area. This always matches src.
* @src: A rectangle specifying the area occupied by the video.
* @top_down: If true the first video line is specified by src.top.
- * @outbuf: The buffer for the compressed frame. This must either be
- * NULL or point to a buffer allocated by malloc since it may be
- * reallocated, if its size is too small.
- * @outbuf_size: The size of the outbuf buffer.
- * @data_size: The size of the compressed frame.
+ * @buffer: A pointer to a VideoBuffer structure containing the
+ * compressed frame if successful. This buffer should be
+ * unref()-ed as soon as no longer needed, optimally before the
+ * next call to encode_frame().
* @return:
* VIDEO_ENCODER_FRAME_ENCODE_DONE if successful.
* VIDEO_ENCODER_FRAME_UNSUPPORTED if the frame cannot be encoded.
@@ -65,8 +96,8 @@ struct VideoEncoder {
*/
int (*encode_frame)(VIDEO_ENCODER_T *encoder, const SpiceBitmap *bitmap,
int width, int height, const SpiceRect *src,
- int top_down, uint32_t frame_mm_time,uint8_t **outbuf,
- size_t *outbuf_size, int *data_size);
+ int top_down, uint32_t frame_mm_time,
+ VIDEO_BUFFER_T** buffer);
/*
* Bit rate control methods.
--
2.1.4
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel