Re: [Spice-devel] [PATCH spice 3/11] server: Refactor the Spice server video encoding so alternative implementations can be added.
On Tue, 19 May 2015, Marc-André Lureau wrote:
[...]
+struct VideoEncoder {
+/* Releases the video encoder's resources */
+void (*destroy)(VIDEO_ENCODER_T *encoder);
+
+/* Compresses the specified src image area into the outbuf buffer.
+ *
+ * @encoder: The video encoder.
+ * @bitmap:The Spice screen.
+ * @width: The width of the Spice screen.
+ * @height:The heigth of the Spice screen.
It's not the screen, it's the video area. It's not clear to me what
would happen if the src size doesn't match the widthheight.
Ah right. I'll fix the comment. I was confused about that but after
looking again I must say I'm still unsure as to why we get both src and
width+height.
My understanding is that this has been introduced to deal with the
YouTube progress bar:
https://bugzilla.redhat.com/show_bug.cgi?id=813826
To sum up, the size of the video area detected by the Spice server
changes when the progress bar pops up or down. As a result some of the
'video frames' it gets don't match the size it detected when it created
the video stream.
* It used to be that this caused such frames to go through the regular
update mechanism instead of through the video stream. This is still
the case with the spicec client for instance and results in the video
seeming to jump back and forth because the video stream is delayed
due to buffering while the regular path is not.
Looking at the impact on src height, either the size of the video
frame, aka src, matches the stream size and then so do width and
height (lines 9 10 below), or it does not and then we don't go
through the video encoder (line 6 below).
* Now clients can advertise support for changing stream sizes with the
SIZED_STREAM capability. This does not totally fix the back and forth
issue but restricts it to the progress bar area as it comes in and
out of the video frame.
In that case width height are either computed from the actual video
frame size, aka src, (lines 3 4 below), or default to the initial
stream size when the video frame size matches (lines 9 10 below).
From red_marshall_stream_data():
1 if (drawable-sized_stream) {
2 if (red_channel_client_test_remote_cap(rcc,
SPICE_DISPLAY_CAP_SIZED_STREAM)) {
3 width = src_rect-right - src_rect-left;
4 height = src_rect-bottom - src_rect-top;
5 } else {
6 return FALSE;
7 }
8 } else {
9 width = stream-width;
10 height = stream-height;
11 }
So in the end width and height *always* match src. So they're redundant,
unless some larger purpose is planned for them.
Now must say having a continuously changing stream size is quite
annoying in the context of GStreamer:
- At a minimum it forces a reconfiguration of the pipeline which,
depending on the encoder, may require rebuilding it. Either cases
seem to cause a reset of the GStreamer's encoder bitrate control
algorithm which tends to cause it to overshoot and may cause frame
drops.
- Ultimately the GStreamer video encoder should support inter-frame
compression (with VP8 or H264 for instance) and a changing stream
size will also cause some disruption in this context.
Things can still work as is so there's not hurry but maybe a better
solution can be devised in time.
For instance it may be better to increase the stream size as the video
frame size increases but then peg it so it does not shrink again. This
would solve the remaining back and forth issue with the YouTube progress
bar, and limit the number of resizes the video encoders have to go
through. It supposes that regular frame updates know to ignore changes
happening inside the stream area even if they don't go through the
regular 'video frame' update path but I have no idea if that's easy to
do.
Alternatively one could split oversize video frames in two: the stream
area which would go through the video encoder, and the remainder which
would go through the regular path. This would also solve the remaining
back and forth issue with the YouTube progress bar and would even be
compatible with non SIZED_STREAM capable clients.
--
Francois Gouget fgou...@codeweavers.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice 3/11] server: Refactor the Spice server video encoding so alternative implementations can be added.
looks good
On Wed, May 13, 2015 at 10:27 PM, Francois Gouget fgou...@codeweavers.com
wrote:
This patch simply replaces the mjpeg_encoder_xxx() call points with a
more object oriented design.
---
server/Makefile.am | 2 +-
server/mjpeg_encoder.c | 134 +---
server/mjpeg_encoder.h | 114 --
server/red_worker.c| 156
++-
server/video_encoder.h | 162
+
5 files changed, 331 insertions(+), 237 deletions(-)
delete mode 100644 server/mjpeg_encoder.h
create mode 100644 server/video_encoder.h
diff --git a/server/Makefile.am b/server/Makefile.am
index 89a375c..36b6d45 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -81,7 +81,6 @@ libspice_server_la_SOURCES = \
main_channel.c \
main_channel.h \
mjpeg_encoder.c \
- mjpeg_encoder.h \
red_bitmap_utils.h \
red_channel.c \
red_channel.h \
@@ -115,6 +114,7 @@ libspice_server_la_SOURCES =\
spicevmc.c \
spice_timer_queue.c \
spice_timer_queue.h \
+ video_encoder.h \
zlib_encoder.c \
zlib_encoder.h \
spice_bitmap_utils.h\
diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index 9a41ef3..d734520 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -20,7 +20,11 @@
#endif
#include red_common.h
-#include mjpeg_encoder.h
+
+typedef struct MJpegEncoder MJpegEncoder;
+#define VIDEO_ENCODER_T MJpegEncoder
+#include video_encoder.h
+
#include jerror.h
#include jpeglib.h
#include inttypes.h
@@ -154,6 +158,7 @@ typedef struct MJpegEncoderRateControl {
} MJpegEncoderRateControl;
struct MJpegEncoder {
+VideoEncoder base;
uint8_t *row;
uint32_t row_size;
int first_frame;
@@ -165,7 +170,7 @@ struct MJpegEncoder {
void (*pixel_converter)(uint8_t *src, uint8_t *dest);
MJpegEncoderRateControl rate_control;
-MJpegEncoderRateControlCbs cbs;
+VideoEncoderRateControlCbs cbs;
void *cbs_opaque;
/* stats */
@@ -174,6 +179,22 @@ struct MJpegEncoder {
uint32_t num_frames;
};
+static void mjpeg_encoder_destroy(MJpegEncoder *encoder);
+static int mjpeg_encoder_encode_frame(MJpegEncoder *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);
+static void mjpeg_encoder_client_stream_report(MJpegEncoder *encoder,
+uint32_t num_frames,
+uint32_t num_drops,
+uint32_t start_frame_mm_time,
+uint32_t end_frame_mm_time,
+int32_t end_frame_delay,
+uint32_t audio_delay);
+static void mjpeg_encoder_notify_server_frame_drop(MJpegEncoder *encoder);
+static uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder *encoder);
+static void mjpeg_encoder_get_stats(MJpegEncoder *encoder,
VideoEncoderStats *stats);
+
static inline void mjpeg_encoder_reset_quality(MJpegEncoder *encoder,
int quality_id,
uint32_t fps,
@@ -189,8 +210,9 @@ static inline int rate_control_is_active(MJpegEncoder*
encoder)
return encoder-cbs.get_roundtrip_ms != NULL;
}
-MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
-MJpegEncoderRateControlCbs *cbs, void
*opaque)
+MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
+ VideoEncoderRateControlCbs *cbs,
+ void *cbs_opaque)
{
MJpegEncoder *enc;
@@ -198,6 +220,12 @@ MJpegEncoder *mjpeg_encoder_new(uint64_t
starting_bit_rate,
enc = spice_new0(MJpegEncoder, 1);
+enc-base.destroy = mjpeg_encoder_destroy;
+enc-base.encode_frame = mjpeg_encoder_encode_frame;
+enc-base.client_stream_report = mjpeg_encoder_client_stream_report;
+enc-base.notify_server_frame_drop =
mjpeg_encoder_notify_server_frame_drop;
+enc-base.get_bit_rate = mjpeg_encoder_get_bit_rate;
+enc-base.get_stats = mjpeg_encoder_get_stats;
enc-first_frame = TRUE;
enc-rate_control.byte_rate = starting_bit_rate / 8;
