Re: [Spice-devel] [PATCH spice 01/13] server: Refactor the Spice server video encoding so alternative implementations can be added. (take 4)
This patch simply replaces the mjpeg_encoder_xxx() call points with a
more object oriented
design.
Signed-off-by: Francois Gouget fgou...@codeweavers.com
---
On Fri, 31 Jul 2015, Victor Toso wrote:
[...]
+static inline uint8_t *get_image_line(SpiceChunks *chunks, size_t *offset,
+ int *chunk_nr, int stride)
Lining up with opening parenthesis
Done.
+static int encode_mjpeg_frame(MJpegEncoder *encoder, const SpiceRect *src,
+const SpiceBitmap *image, int top_down)
Lining up with opening parenthesis
Done.
+for (i = 0; i skip_lines; i++) {
+get_image_line(chunks, offset, chunk, image_stride);
Decrease one level of identation
Done.
+if (!src_line) {
+return FALSE;
+}
I would remove the brances of above if to keep equal to the if bellow
+
+src_line += src-left * mjpeg_encoder_get_bytes_per_pixel(encoder);
+if (mjpeg_encoder_encode_scanline(encoder, src_line, stream_width)
== 0)
+return FALSE;
+}
Actually I introduced the if below too and it violates the coding
standard that says if statements should always have braces. So I've done
a pass through all my patches and it seems this file contained the only
3 incorrect if statements.
@@ -2693,18 +2693,19 @@ static void red_stop_stream(RedWorker *worker,
Stream *stream)
region_clear(stream_agent-vis_region);
region_clear(stream_agent-clip);
spice_assert(!pipe_item_is_linked(stream_agent-destroy_item));
-if (stream_agent-mjpeg_encoder
dcc-use_mjpeg_encoder_rate_control) {
-uint64_t stream_bit_rate =
mjpeg_encoder_get_bit_rate(stream_agent-mjpeg_encoder);
-
-if (stream_bit_rate dcc-streams_max_bit_rate) {
-spice_debug(old max-bit-rate=%.2f new=%.2f,
-dcc-streams_max_bit_rate / 8.0 / 1024.0 / 1024.0,
-stream_bit_rate / 8.0 / 1024.0 / 1024.0);
-dcc-streams_max_bit_rate = stream_bit_rate;
+if (stream_agent-video_encoder) {
+if (dcc-use_video_encoder_rate_control) {
+uint64_t stream_bit_rate =
stream_agent-video_encoder-get_bit_rate(stream_agent-video_encoder);
+if (stream_bit_rate dcc-streams_max_bit_rate) {
+spice_debug(old max-bit-rate=%.2f new=%.2f,
+dcc-streams_max_bit_rate / 8.0 / 1024.0 / 1024.0,
+stream_bit_rate / 8.0 / 1024.0 / 1024.0);
+dcc-streams_max_bit_rate = stream_bit_rate;
+}
}
+stream-refs++;
+red_channel_client_pipe_add(dcc-common.base,
stream_agent-destroy_item);
Here you actually changed the logic a bit, moving the strem ref and
red_channel_client_pipe_add into the if. It does seem correct but I was
wondering if stream_agent-mjpeg_encoder could be NULL before...
I have not been able to determine that.
mjpeg_encoder is set in the following locations:
* red_display_create_stream()
This is where mjpeg_encoder is initialized and in the old code this
could not fail. In future patches this may fail.
* red_display_stream_agent_stop()
* red_display_destroy_streams_agents()
Checks whether it is non-NULL and then resets it to NULL.
And mjpeg_encoder is checked against NULL in the following places.
* red_print_stream_stats()
* red_stop_stream()
* red_display_update_streams_max_latency()
But red_marshall_stream_data() assumes it is not NULL.
So clearly mjpeg_encoder is normally not NULL in red_stop_stream() and
red_print_stream_stats() otherwise they would be unable to collect the
bit rate and stream statistics. But I could not rule it out
with certainty.
Let me know if you'd prefer this chunk to go in separately.
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..48042ba
Re: [Spice-devel] [PATCH spice 01/13] server: Refactor the Spice server video encoding so alternative implementations can be added. (take 4)
Hello,
Few comments/suggestions below,
On Tue, Jul 21, 2015 at 08:00:40PM +0200, Francois Gouget wrote:
This patch simply replaces the mjpeg_encoder_xxx() call points with a more
object oriented design.
Signed-off-by: Francois Gouget fgou...@codeweavers.com
---
Changes since take 3:
- A NULL video_encoder means that the stream creation failed so
red_stop_stream() no longer sends messages to destroy it in
that case. This avoids getting error messages in spice-gtk.
- Removed some forward declarations and made more functions static.
- Tweaked some video_encoder.h comments.
Changes since take 2:
- No change.
Changes since take 1:
- I fixed the width/height comments and they now state that they always
match src.
server/Makefile.am | 2 +-
server/mjpeg_encoder.c | 227
+++--
server/mjpeg_encoder.h | 114 -
server/red_worker.c| 173 -
server/video_encoder.h | 165 +++
5 files changed, 381 insertions(+), 300 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..48042ba 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,11 +179,6 @@ struct MJpegEncoder {
uint32_t num_frames;
};
-static inline void mjpeg_encoder_reset_quality(MJpegEncoder *encoder,
- int quality_id,
- uint32_t fps,
- uint64_t frame_enc_size);
-static uint32_t get_max_fps(uint64_t frame_size, uint64_t bytes_per_sec);
static void mjpeg_encoder_process_server_drops(MJpegEncoder *encoder);
static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
uint64_t byte_rate,
@@ -189,49 +189,14 @@ 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 *enc;
-
-spice_assert(!cbs || (cbs cbs-get_roundtrip_ms
cbs-get_source_fps));
-
-enc = spice_new0(MJpegEncoder, 1);
-
-enc-first_frame = TRUE;
-enc-rate_control.byte_rate = starting_bit_rate / 8;
-enc-starting_bit_rate = starting_bit_rate;
-
-if (cbs) {
-struct timespec time;
-
-clock_gettime(CLOCK_MONOTONIC, time);
-enc-cbs = *cbs;
-enc-cbs_opaque = opaque;
-mjpeg_encoder_reset_quality(enc, MJPEG_QUALITY_SAMPLE_NUM / 2, 5, 0);
-enc-rate_control.during_quality_eval = TRUE;
-enc-rate_control.quality_eval_data.type =
MJPEG_QUALITY_EVAL_TYPE_SET;
-enc-rate_control.quality_eval_data.reason =
MJPEG_QUALITY_EVAL_REASON_RATE_CHANGE;
-enc-rate_control.warmup_start_time = ((uint64_t) time.tv_sec) *
10 + time.tv_nsec;
-} else {
-enc-cbs.get_roundtrip_ms = NULL;
-mjpeg_encoder_reset_quality(enc,
[Spice-devel] [PATCH spice 01/13] server: Refactor the Spice server video encoding so alternative implementations can be added. (take 4)
This patch simply replaces the mjpeg_encoder_xxx() call points with a more
object oriented design.
Signed-off-by: Francois Gouget fgou...@codeweavers.com
---
Changes since take 3:
- A NULL video_encoder means that the stream creation failed so
red_stop_stream() no longer sends messages to destroy it in
that case. This avoids getting error messages in spice-gtk.
- Removed some forward declarations and made more functions static.
- Tweaked some video_encoder.h comments.
Changes since take 2:
- No change.
Changes since take 1:
- I fixed the width/height comments and they now state that they always
match src.
server/Makefile.am | 2 +-
server/mjpeg_encoder.c | 227 +++--
server/mjpeg_encoder.h | 114 -
server/red_worker.c| 173 -
server/video_encoder.h | 165 +++
5 files changed, 381 insertions(+), 300 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..48042ba 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,11 +179,6 @@ struct MJpegEncoder {
uint32_t num_frames;
};
-static inline void mjpeg_encoder_reset_quality(MJpegEncoder *encoder,
- int quality_id,
- uint32_t fps,
- uint64_t frame_enc_size);
-static uint32_t get_max_fps(uint64_t frame_size, uint64_t bytes_per_sec);
static void mjpeg_encoder_process_server_drops(MJpegEncoder *encoder);
static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
uint64_t byte_rate,
@@ -189,49 +189,14 @@ 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 *enc;
-
-spice_assert(!cbs || (cbs cbs-get_roundtrip_ms
cbs-get_source_fps));
-
-enc = spice_new0(MJpegEncoder, 1);
-
-enc-first_frame = TRUE;
-enc-rate_control.byte_rate = starting_bit_rate / 8;
-enc-starting_bit_rate = starting_bit_rate;
-
-if (cbs) {
-struct timespec time;
-
-clock_gettime(CLOCK_MONOTONIC, time);
-enc-cbs = *cbs;
-enc-cbs_opaque = opaque;
-mjpeg_encoder_reset_quality(enc, MJPEG_QUALITY_SAMPLE_NUM / 2, 5, 0);
-enc-rate_control.during_quality_eval = TRUE;
-enc-rate_control.quality_eval_data.type = MJPEG_QUALITY_EVAL_TYPE_SET;
-enc-rate_control.quality_eval_data.reason =
MJPEG_QUALITY_EVAL_REASON_RATE_CHANGE;
-enc-rate_control.warmup_start_time = ((uint64_t) time.tv_sec) *
10 + time.tv_nsec;
-} else {
-enc-cbs.get_roundtrip_ms = NULL;
-mjpeg_encoder_reset_quality(enc, MJPEG_LEGACY_STATIC_QUALITY_ID,
MJPEG_MAX_FPS, 0);
-}
-
-enc-cinfo.err = jpeg_std_error(enc-jerr);
-jpeg_create_compress(enc-cinfo);
-
-return enc;
-}
-
-void mjpeg_encoder_destroy(MJpegEncoder *encoder)
