On Thu, Aug 27, 2015 at 09:00:42PM +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 <fgouget@xxxxxxxxxxxxxxx>
> ---
>
> Changes since take 4:
> - Fixed the reported formatting issues.
>
>
> server/Makefile.am | 2 +-
> server/mjpeg_encoder.c | 230 +++++++++++++++++++++++++++++++++++--------------
> server/mjpeg_encoder.h | 114 ------------------------
> server/red_worker.c | 173 ++++++++++++-------------------------
> server/video_encoder.h | 165 +++++++++++++++++++++++++++++++++++
> 5 files changed, 384 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 f1da60b..d78e164 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -83,7 +83,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 \
> @@ -119,6 +118,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 4b803a9..d064fd2 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
Still not a big fan of this #define magic
> +#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)(void *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) * 1000000000 + 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)
> +static void mjpeg_encoder_destroy(MJpegEncoder *encoder)
> {
> jpeg_destroy_compress(&encoder->cinfo);
> free(encoder->row);
> free(encoder);
> }
>
> -uint8_t mjpeg_encoder_get_bytes_per_pixel(MJpegEncoder *encoder)
> +static uint8_t mjpeg_encoder_get_bytes_per_pixel(MJpegEncoder *encoder)
> {
> return encoder->bytes_per_pixel;
> }
> @@ -732,10 +697,11 @@ static void mjpeg_encoder_adjust_fps(MJpegEncoder *encoder, uint64_t now)
> }
> }
>
> -int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
> - int width, int height,
> - uint8_t **dest, size_t *dest_len,
> - uint32_t frame_mm_time)
> +static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
> + SpiceBitmapFmt format,
> + int width, int height,
> + uint8_t **dest, size_t *dest_len,
> + uint32_t frame_mm_time)
> {
> uint32_t quality;
>
> @@ -755,7 +721,7 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
> interval = (now - rate_control->bit_rate_info.last_frame_time);
>
> if (interval < (1000*1000*1000) / rate_control->adjusted_fps) {
> - return MJPEG_ENCODER_FRAME_DROP;
> + return VIDEO_ENCODER_FRAME_DROP;
> }
>
> mjpeg_encoder_adjust_params_to_bit_rate(encoder);
> @@ -803,14 +769,14 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
> break;
> default:
> spice_debug("unsupported format %d", format);
> - return MJPEG_ENCODER_FRAME_UNSUPPORTED;
> + return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> }
>
> if (encoder->pixel_converter != NULL) {
> unsigned int stride = width * 3;
> /* check for integer overflow */
> if (stride < width) {
> - return MJPEG_ENCODER_FRAME_UNSUPPORTED;
> + return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> }
> if (encoder->row_size < stride) {
> encoder->row = spice_realloc(encoder->row, stride);
> @@ -830,11 +796,12 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
>
> encoder->num_frames++;
> encoder->avg_quality += quality;
> - return MJPEG_ENCODER_FRAME_ENCODE_START;
> + return VIDEO_ENCODER_FRAME_ENCODE_DONE;
> }
>
> -int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder, uint8_t *src_pixels,
> - size_t image_width)
> +static int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder,
> + uint8_t *src_pixels,
> + size_t image_width)
> {
> unsigned int scanlines_written;
> uint8_t *row;
> @@ -861,7 +828,7 @@ int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder, uint8_t *src_pixels,
> return scanlines_written;
> }
>
> -size_t mjpeg_encoder_end_frame(MJpegEncoder *encoder)
> +static size_t mjpeg_encoder_end_frame(MJpegEncoder *encoder)
> {
> mem_destination_mgr *dest = (mem_destination_mgr *) encoder->cinfo.dest;
> MJpegEncoderRateControl *rate_control = &encoder->rate_control;
> @@ -890,6 +857,98 @@ size_t mjpeg_encoder_end_frame(MJpegEncoder *encoder)
> return encoder->rate_control.last_enc_size;
> }
>
> +static inline uint8_t *get_image_line(SpiceChunks *chunks, size_t *offset,
> + int *chunk_nr, int stride)
> +{
> + uint8_t *ret;
> + SpiceChunk *chunk;
> +
> + chunk = &chunks->chunk[*chunk_nr];
> +
> + if (*offset == chunk->len) {
> + if (*chunk_nr == chunks->num_chunks - 1) {
> + return NULL; /* Last chunk */
> + }
> + *offset = 0;
> + (*chunk_nr)++;
> + chunk = &chunks->chunk[*chunk_nr];
> + }
> +
> + if (chunk->len - *offset < stride) {
> + spice_warning("bad chunk alignment");
> + return NULL;
> + }
> + ret = chunk->data + *offset;
> + *offset += stride;
> + return ret;
> +}
> +
> +
> +
> +static int encode_mjpeg_frame(MJpegEncoder *encoder, const SpiceRect *src,
> + const SpiceBitmap *image, int top_down)
> +{
> + SpiceChunks *chunks;
> + uint32_t image_stride;
> + size_t offset;
> + int i, chunk;
> +
> + chunks = image->data;
> + offset = 0;
> + chunk = 0;
> + image_stride = image->stride;
> +
> + const int skip_lines = top_down ? src->top : image->y - (src->bottom - 0);
> + for (i = 0; i < skip_lines; i++) {
> + get_image_line(chunks, &offset, &chunk, image_stride);
> + }
> +
> + const unsigned int stream_height = src->bottom - src->top;
> + const unsigned int stream_width = src->right - src->left;
> +
> + for (i = 0; i < stream_height; i++) {
> + uint8_t *src_line = (uint8_t *)get_image_line(chunks, &offset, &chunk, image_stride);
> +
> + if (!src_line) {
> + return FALSE;
> + }
> +
> + src_line += src->left * mjpeg_encoder_get_bytes_per_pixel(encoder);
> + if (mjpeg_encoder_encode_scanline(encoder, src_line, stream_width) == 0) {
> + return FALSE;
> + }
> + }
> +
> + return TRUE;
> +}
> +
> +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)
> +{
> + 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;
> + }
> +
> + *data_size = mjpeg_encoder_end_frame(encoder);
> +
> + return VIDEO_ENCODER_FRAME_ENCODE_DONE;
> +}
> +
> +
> static void mjpeg_encoder_quality_eval_stop(MJpegEncoder *encoder)
> {
> MJpegEncoderRateControl *rate_control = &encoder->rate_control;
> @@ -1060,6 +1119,7 @@ static void mjpeg_encoder_increase_bit_rate(MJpegEncoder *encoder)
> rate_control->quality_id,
> rate_control->fps);
> }
> +
> static void mjpeg_encoder_handle_positive_client_stream_report(MJpegEncoder *encoder,
> uint32_t report_start_frame_mm_time)
> {
> @@ -1118,13 +1178,13 @@ static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
> #define MJPEG_VIDEO_VS_AUDIO_LATENCY_FACTOR 1.25
> #define MJPEG_VIDEO_DELAY_TH -15
>
> -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_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)
> {
> MJpegEncoderRateControl *rate_control = &encoder->rate_control;
> MJpegEncoderClientState *client_state = &rate_control->client_state;
> @@ -1226,7 +1286,7 @@ void mjpeg_encoder_client_stream_report(MJpegEncoder *encoder,
> }
> }
>
> -void mjpeg_encoder_notify_server_frame_drop(MJpegEncoder *encoder)
> +static void mjpeg_encoder_notify_server_frame_drop(MJpegEncoder *encoder)
> {
> encoder->rate_control.server_state.num_frames_dropped++;
> mjpeg_encoder_process_server_drops(encoder);
> @@ -1264,15 +1324,55 @@ static void mjpeg_encoder_process_server_drops(MJpegEncoder *encoder)
> server_state->num_frames_dropped = 0;
> }
>
> -uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder *encoder)
> +static uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder *encoder)
> {
> return encoder->rate_control.byte_rate * 8;
> }
>
> -void mjpeg_encoder_get_stats(MJpegEncoder *encoder, MJpegEncoderStats *stats)
> +static void mjpeg_encoder_get_stats(MJpegEncoder *encoder, VideoEncoderStats *stats)
> {
> spice_assert(encoder != NULL && stats != NULL);
> stats->starting_bit_rate = encoder->starting_bit_rate;
> stats->cur_bit_rate = mjpeg_encoder_get_bit_rate(encoder);
> stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;
> }
> +
> +MJpegEncoder *create_mjpeg_encoder(uint64_t starting_bit_rate,
> + VideoEncoderRateControlCbs *cbs,
> + void *cbs_opaque)
> +{
> + spice_assert(!cbs || (cbs && cbs->get_roundtrip_ms && cbs->get_source_fps));
> +
> + MJpegEncoder *encoder = spice_new0(MJpegEncoder, 1);
> +
> + encoder->base.destroy = &mjpeg_encoder_destroy;
> + encoder->base.encode_frame = &mjpeg_encoder_encode_frame;
> + encoder->base.client_stream_report = &mjpeg_encoder_client_stream_report;
> + encoder->base.notify_server_frame_drop = &mjpeg_encoder_notify_server_frame_drop;
> + encoder->base.get_bit_rate = &mjpeg_encoder_get_bit_rate;
> + encoder->base.get_stats = &mjpeg_encoder_get_stats;
> + encoder->first_frame = TRUE;
> + encoder->rate_control.byte_rate = starting_bit_rate / 8;
> + encoder->starting_bit_rate = starting_bit_rate;
> +
> + if (cbs) {
> + struct timespec time;
> +
> + clock_gettime(CLOCK_MONOTONIC, &time);
> + encoder->cbs = *cbs;
> + encoder->cbs_opaque = cbs_opaque;
> + mjpeg_encoder_reset_quality(encoder, MJPEG_QUALITY_SAMPLE_NUM / 2, 5, 0);
> + encoder->rate_control.during_quality_eval = TRUE;
> + encoder->rate_control.quality_eval_data.type = MJPEG_QUALITY_EVAL_TYPE_SET;
> + encoder->rate_control.quality_eval_data.reason = MJPEG_QUALITY_EVAL_REASON_RATE_CHANGE;
> + encoder->rate_control.warmup_start_time = ((uint64_t) time.tv_sec) * 1000000000 + time.tv_nsec;
> + } else {
> + encoder->cbs.get_roundtrip_ms = NULL;
> + mjpeg_encoder_reset_quality(encoder, MJPEG_LEGACY_STATIC_QUALITY_ID, MJPEG_MAX_FPS, 0);
> + }
> +
> + encoder->cinfo.err = jpeg_std_error(&encoder->jerr);
> + jpeg_create_compress(&encoder->cinfo);
> +
> + return encoder;
> +}
> diff --git a/server/mjpeg_encoder.h b/server/mjpeg_encoder.h
> deleted file mode 100644
> index d584b92..0000000
> --- a/server/mjpeg_encoder.h
> +++ /dev/null
> @@ -1,114 +0,0 @@
> -/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> -/*
> - Copyright (C) 2009 Red Hat, Inc.
> -
> - This library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - This library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with this library; if not, see <http://www.gnu.org/licenses/>.
> -*/
> -
> -#ifndef _H_MJPEG_ENCODER
> -#define _H_MJPEG_ENCODER
> -
> -#include "red_common.h"
> -
> -enum {
> - MJPEG_ENCODER_FRAME_UNSUPPORTED = -1,
> - MJPEG_ENCODER_FRAME_DROP,
> - MJPEG_ENCODER_FRAME_ENCODE_START,
> -};
> -
> -typedef struct MJpegEncoder MJpegEncoder;
> -
> -/*
> - * Callbacks required for controling and adjusting
> - * the stream bit rate:
> - * get_roundtrip_ms: roundtrip time in milliseconds
> - * get_source_fps: the input frame rate (#frames per second), i.e.,
> - * the rate of frames arriving from the guest to spice-server,
> - * before any drops.
> - */
> -typedef struct MJpegEncoderRateControlCbs {
> - uint32_t (*get_roundtrip_ms)(void *opaque);
> - uint32_t (*get_source_fps)(void *opaque);
> - void (*update_client_playback_delay)(void *opaque, uint32_t delay_ms);
> -} MJpegEncoderRateControlCbs;
> -
> -typedef struct MJpegEncoderStats {
> - uint64_t starting_bit_rate;
> - uint64_t cur_bit_rate;
> - double avg_quality;
> -} MJpegEncoderStats;
> -
> -MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
> - MJpegEncoderRateControlCbs *cbs, void *opaque);
> -void mjpeg_encoder_destroy(MJpegEncoder *encoder);
> -
> -uint8_t mjpeg_encoder_get_bytes_per_pixel(MJpegEncoder *encoder);
> -
> -/*
> - * dest must be either NULL or allocated by malloc, since it might be freed
> - * during the encoding, if its size is too small.
> - *
> - * return:
> - * MJPEG_ENCODER_FRAME_UNSUPPORTED : frame cannot be encoded
> - * MJPEG_ENCODER_FRAME_DROP : frame should be dropped. This value can only be returned
> - * if mjpeg rate control is active.
> - * MJPEG_ENCODER_FRAME_ENCODE_START: frame encoding started. Continue with
> - * mjpeg_encoder_encode_scanline.
> - */
> -int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
> - int width, int height,
> - uint8_t **dest, size_t *dest_len,
> - uint32_t frame_mm_time);
> -int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder, uint8_t *src_pixels,
> - size_t image_width);
> -size_t mjpeg_encoder_end_frame(MJpegEncoder *encoder);
> -
> -/*
> - * bit rate control
> - */
> -
> -/*
> - * Data that should be periodically obtained from the client. The report contains:
> - * num_frames : the number of frames that reached the client during the time
> - * the report is referring to.
> - * num_drops : the part of the above frames that was dropped by the client due to
> - * late arrival time.
> - * start_frame_mm_time: the mm_time of the first frame included in the report
> - * end_frame_mm_time : the mm_time of the last_frame included in the report
> - * end_frame_delay : (end_frame_mm_time - client_mm_time)
> - * audio delay : the latency of the audio playback.
> - * If there is no audio playback, set it to MAX_UINT.
> - *
> - */
> -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);
> -
> -/*
> - * Notify the encoder each time a frame is dropped due to pipe
> - * congestion.
> - * We can deduce the client state by the frame dropping rate in the server.
> - * Monitoring the frame drops can help in fine tuning the playback parameters
> - * when the client reports are delayed.
> - */
> -void mjpeg_encoder_notify_server_frame_drop(MJpegEncoder *encoder);
> -
> -uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder *encoder);
> -void mjpeg_encoder_get_stats(MJpegEncoder *encoder, MJpegEncoderStats *stats);
> -
> -#endif
> diff --git a/server/red_worker.c b/server/red_worker.c
> index eeffdd0..a5548f0 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -70,7 +70,7 @@
> #include "glz_encoder.h"
> #include "stat.h"
> #include "reds.h"
> -#include "mjpeg_encoder.h"
> +#include "video_encoder.h"
> #include "red_memslots.h"
> #include "red_parse_qxl.h"
> #include "red_record_qxl.h"
> @@ -484,7 +484,7 @@ typedef struct StreamAgent {
> PipeItem destroy_item;
> Stream *stream;
> uint64_t last_send_time;
> - MJpegEncoder *mjpeg_encoder;
> + VideoEncoder *video_encoder;
> DisplayChannelClient *dcc;
>
> int frames;
> @@ -723,7 +723,7 @@ struct DisplayChannelClient {
> QRegion surface_client_lossy_region[NUM_SURFACES];
>
> StreamAgent stream_agents[NUM_STREAMS];
> - int use_mjpeg_encoder_rate_control;
> + int use_video_encoder_rate_control;
> uint32_t streams_max_latency;
> uint64_t streams_max_bit_rate;
> };
> @@ -2645,10 +2645,10 @@ static void red_print_stream_stats(DisplayChannelClient *dcc, StreamAgent *agent
> #ifdef STREAM_STATS
> StreamStats *stats = &agent->stats;
> double passed_mm_time = (stats->end - stats->start) / 1000.0;
> - MJpegEncoderStats encoder_stats = {0};
> + VideoEncoderStats encoder_stats = {0};
>
> - if (agent->mjpeg_encoder) {
> - mjpeg_encoder_get_stats(agent->mjpeg_encoder, &encoder_stats);
> + if (agent->video_encoder) {
> + agent->video_encoder->get_stats(agent->video_encoder, &encoder_stats);
> }
>
> spice_debug("stream=%"PRIdPTR" dim=(%dx%d) #in-frames=%"PRIu64" #in-avg-fps=%.2f #out-frames=%"PRIu64" "
> @@ -2691,18 +2691,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);
> }
> - stream->refs++;
> - red_channel_client_pipe_add(&dcc->common.base, &stream_agent->destroy_item);
This bit got moved inside the if (stream_agent->video_encoder &&
dcc->use_mjpeg_encoder_rate_control) block rather than being done
unconditionnally?
> red_print_stream_stats(dcc, stream_agent);
> }
> worker->streams_size_total -= stream->width * stream->height;
> @@ -2999,7 +3000,7 @@ static uint64_t red_stream_get_initial_bit_rate(DisplayChannelClient *dcc,
> stream->width * stream->height) / dcc->common.worker->streams_size_total;
> }
>
> -static uint32_t red_stream_mjpeg_encoder_get_roundtrip(void *opaque)
> +static uint32_t red_stream_video_encoder_get_roundtrip(void *opaque)
> {
> StreamAgent *agent = opaque;
> int roundtrip;
> @@ -3020,7 +3021,7 @@ static uint32_t red_stream_mjpeg_encoder_get_roundtrip(void *opaque)
> return roundtrip;
> }
>
> -static uint32_t red_stream_mjpeg_encoder_get_source_fps(void *opaque)
> +static uint32_t red_stream_video_encoder_get_source_fps(void *opaque)
> {
> StreamAgent *agent = opaque;
>
> @@ -3043,7 +3044,7 @@ static void red_display_update_streams_max_latency(DisplayChannelClient *dcc, St
> }
> for (i = 0; i < NUM_STREAMS; i++) {
> StreamAgent *other_agent = &dcc->stream_agents[i];
> - if (other_agent == remove_agent || !other_agent->mjpeg_encoder) {
> + if (other_agent == remove_agent || !other_agent->video_encoder) {
> continue;
> }
> if (other_agent->client_required_latency > new_max_latency) {
> @@ -3056,9 +3057,9 @@ static void red_display_update_streams_max_latency(DisplayChannelClient *dcc, St
> static void red_display_stream_agent_stop(DisplayChannelClient *dcc, StreamAgent *agent)
> {
> red_display_update_streams_max_latency(dcc, agent);
> - if (agent->mjpeg_encoder) {
> - mjpeg_encoder_destroy(agent->mjpeg_encoder);
> - agent->mjpeg_encoder = NULL;
> + if (agent->video_encoder) {
> + agent->video_encoder->destroy(agent->video_encoder);
> + agent->video_encoder = NULL;
> }
> }
>
> @@ -3094,18 +3095,18 @@ static void red_display_create_stream(DisplayChannelClient *dcc, Stream *stream)
> agent->fps = MAX_FPS;
> agent->dcc = dcc;
>
> - if (dcc->use_mjpeg_encoder_rate_control) {
> - MJpegEncoderRateControlCbs mjpeg_cbs;
> + if (dcc->use_video_encoder_rate_control) {
> + VideoEncoderRateControlCbs video_cbs;
> uint64_t initial_bit_rate;
>
> - mjpeg_cbs.get_roundtrip_ms = red_stream_mjpeg_encoder_get_roundtrip;
> - mjpeg_cbs.get_source_fps = red_stream_mjpeg_encoder_get_source_fps;
> - mjpeg_cbs.update_client_playback_delay = red_stream_update_client_playback_latency;
> + video_cbs.get_roundtrip_ms = red_stream_video_encoder_get_roundtrip;
> + video_cbs.get_source_fps = red_stream_video_encoder_get_source_fps;
> + video_cbs.update_client_playback_delay = red_stream_update_client_playback_latency;
>
> initial_bit_rate = red_stream_get_initial_bit_rate(dcc, stream);
> - agent->mjpeg_encoder = mjpeg_encoder_new(initial_bit_rate, &mjpeg_cbs, agent);
> + agent->video_encoder = create_mjpeg_encoder(initial_bit_rate, &video_cbs, agent);
> } else {
> - agent->mjpeg_encoder = mjpeg_encoder_new(0, NULL, NULL);
> + agent->video_encoder = create_mjpeg_encoder(0, NULL, NULL);
> }
> red_channel_client_pipe_add(&dcc->common.base, &agent->create_item);
>
> @@ -3191,7 +3192,7 @@ static void red_display_client_init_streams(DisplayChannelClient *dcc)
> red_channel_pipe_item_init(channel, &agent->create_item, PIPE_ITEM_TYPE_STREAM_CREATE);
> red_channel_pipe_item_init(channel, &agent->destroy_item, PIPE_ITEM_TYPE_STREAM_DESTROY);
> }
> - dcc->use_mjpeg_encoder_rate_control =
> + dcc->use_video_encoder_rate_control =
> red_channel_client_test_remote_cap(&dcc->common.base, SPICE_DISPLAY_CAP_STREAM_REPORT);
> }
>
> @@ -3203,9 +3204,9 @@ static void red_display_destroy_streams_agents(DisplayChannelClient *dcc)
> StreamAgent *agent = &dcc->stream_agents[i];
> region_destroy(&agent->vis_region);
> region_destroy(&agent->clip);
> - if (agent->mjpeg_encoder) {
> - mjpeg_encoder_destroy(agent->mjpeg_encoder);
> - agent->mjpeg_encoder = NULL;
> + if (agent->video_encoder) {
> + agent->video_encoder->destroy(agent->video_encoder);
> + agent->video_encoder = NULL;
> }
> }
> }
> @@ -3332,7 +3333,7 @@ static inline void pre_stream_item_swap(RedWorker *worker, Stream *stream, Drawa
> dcc = dpi->dcc;
> agent = &dcc->stream_agents[index];
>
> - if (!dcc->use_mjpeg_encoder_rate_control &&
> + if (!dcc->use_video_encoder_rate_control &&
> !dcc->common.is_low_bandwidth) {
> continue;
> }
> @@ -3341,8 +3342,8 @@ static inline void pre_stream_item_swap(RedWorker *worker, Stream *stream, Drawa
> #ifdef STREAM_STATS
> agent->stats.num_drops_pipe++;
> #endif
> - if (dcc->use_mjpeg_encoder_rate_control) {
> - mjpeg_encoder_notify_server_frame_drop(agent->mjpeg_encoder);
> + if (dcc->use_video_encoder_rate_control) {
> + agent->video_encoder->notify_server_frame_drop(agent->video_encoder);
> } else {
> ++agent->drops;
> }
> @@ -3355,7 +3356,7 @@ static inline void pre_stream_item_swap(RedWorker *worker, Stream *stream, Drawa
>
> agent = &dcc->stream_agents[index];
>
> - if (dcc->use_mjpeg_encoder_rate_control) {
> + if (dcc->use_video_encoder_rate_control) {
> continue;
> }
> if (agent->frames / agent->fps < FPS_TEST_INTERVAL) {
> @@ -8473,70 +8474,6 @@ static inline void display_begin_send_message(RedChannelClient *rcc)
> red_channel_client_begin_send_message(rcc);
> }
>
> -static inline uint8_t *red_get_image_line(SpiceChunks *chunks, size_t *offset,
> - int *chunk_nr, int stride)
> -{
> - uint8_t *ret;
> - SpiceChunk *chunk;
> -
> - chunk = &chunks->chunk[*chunk_nr];
> -
> - if (*offset == chunk->len) {
> - if (*chunk_nr == chunks->num_chunks - 1) {
> - return NULL; /* Last chunk */
> - }
> - *offset = 0;
> - (*chunk_nr)++;
> - chunk = &chunks->chunk[*chunk_nr];
> - }
> -
> - if (chunk->len - *offset < stride) {
> - spice_warning("bad chunk alignment");
> - return NULL;
> - }
> - ret = chunk->data + *offset;
> - *offset += stride;
> - return ret;
> -}
> -
> -static int encode_frame(DisplayChannelClient *dcc, const SpiceRect *src,
> - const SpiceBitmap *image, Stream *stream)
> -{
> - SpiceChunks *chunks;
> - uint32_t image_stride;
> - size_t offset;
> - int i, chunk;
> - StreamAgent *agent = &dcc->stream_agents[stream - dcc->common.worker->streams_buf];
> -
> - chunks = image->data;
> - offset = 0;
> - chunk = 0;
> - image_stride = image->stride;
> -
> - const int skip_lines = stream->top_down ? src->top : image->y - (src->bottom - 0);
> - for (i = 0; i < skip_lines; i++) {
> - red_get_image_line(chunks, &offset, &chunk, image_stride);
> - }
> -
> - const unsigned int stream_height = src->bottom - src->top;
> - const unsigned int stream_width = src->right - src->left;
> -
> - for (i = 0; i < stream_height; i++) {
> - uint8_t *src_line =
> - (uint8_t *)red_get_image_line(chunks, &offset, &chunk, image_stride);
> -
> - if (!src_line) {
> - return FALSE;
> - }
> -
> - src_line += src->left * mjpeg_encoder_get_bytes_per_pixel(agent->mjpeg_encoder);
> - if (mjpeg_encoder_encode_scanline(agent->mjpeg_encoder, src_line, stream_width) == 0)
> - return FALSE;
> - }
> -
> - return TRUE;
> -}
> -
> static inline int red_marshall_stream_data(RedChannelClient *rcc,
> SpiceMarshaller *base_marshaller, Drawable *drawable)
> {
> @@ -8581,7 +8518,7 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
> uint64_t time_now = red_now();
> size_t outbuf_size;
>
> - if (!dcc->use_mjpeg_encoder_rate_control) {
> + if (!dcc->use_video_encoder_rate_control) {
> if (time_now - agent->last_send_time < (1000 * 1000 * 1000) / agent->fps) {
> agent->frames--;
> #ifdef STREAM_STATS
> @@ -8595,33 +8532,29 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
> 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 = mjpeg_encoder_start_frame(agent->mjpeg_encoder, image->u.bitmap.format,
> - width, height,
> - &dcc->send_data.stream_outbuf,
> - &outbuf_size,
> - frame_mm_time);
> + 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);
> +
> switch (ret) {
> - case MJPEG_ENCODER_FRAME_DROP:
> - spice_assert(dcc->use_mjpeg_encoder_rate_control);
> + case VIDEO_ENCODER_FRAME_DROP:
> + spice_assert(dcc->use_video_encoder_rate_control);
> #ifdef STREAM_STATS
> agent->stats.num_drops_fps++;
> #endif
> return TRUE;
> - case MJPEG_ENCODER_FRAME_UNSUPPORTED:
> + case VIDEO_ENCODER_FRAME_UNSUPPORTED:
> return FALSE;
> - case MJPEG_ENCODER_FRAME_ENCODE_START:
> + case VIDEO_ENCODER_FRAME_ENCODE_DONE:
> break;
> default:
> - spice_error("bad return value (%d) from mjpeg_encoder_start_frame", ret);
> - return FALSE;
> - }
> -
> - if (!encode_frame(dcc, &drawable->red_drawable->u.copy.src_area,
> - &image->u.bitmap, stream)) {
> + spice_error("bad return value (%d) from VideoEncoder::encode_frame", ret);
> return FALSE;
> }
> - n = mjpeg_encoder_end_frame(agent->mjpeg_encoder);
> dcc->send_data.stream_outbuf_size = outbuf_size;
>
> if (!drawable->sized_stream) {
This hunk is also more than just renaming/moving code around, I would
have added a commit doing just this change before the big code
movement/renaming that this commit does, as otherwise it's quite hidden
in all these changes.
Looks good apart from these comments,
Christophe
Attachment:
pgpfvNNc6Fdfb.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel