On Wed, Oct 14, 2015 at 05:31:38PM +0200, Francois Gouget wrote: > Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx> > --- > server/Makefile.am | 2 +- > server/mjpeg_encoder.c | 85 +++++++++++++++----------- > server/red_worker.c | 86 +++++++++++++------------- > server/video_encoder.h | 162 +++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 257 insertions(+), 78 deletions(-) > create mode 100644 server/video_encoder.h > > > Changes since the previous version: > * Split off what I could to reduce the size of this patch, Fwiw, I would have split the mechanical s/mjpeg/video/ out of this patch to only keep the introduction of the VideoEncoder base class here (but no need to redo that at this point!) Looks good, ACK, a bunch of cosmetic issues below > * Got rid of the VIDEO_ENCODER_T magic. > * Removed a change that seems unnecessary in red_stop_stream(). > > > diff --git a/server/Makefile.am b/server/Makefile.am > index fad1cbc..9bed41f 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 \ > @@ -121,6 +120,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 1c3ecff..c0fb7c6 100644 > --- a/server/mjpeg_encoder.c > +++ b/server/mjpeg_encoder.c > @@ -20,7 +20,8 @@ > #endif > > #include "red_common.h" > -#include "mjpeg_encoder.h" > +#include "video_encoder.h" > + > #include <jerror.h> > #include <jpeglib.h> > #include <inttypes.h> > @@ -153,7 +154,8 @@ typedef struct MJpegEncoderRateControl { > uint64_t warmup_start_time; > } MJpegEncoderRateControl; > > -struct MJpegEncoder { > +typedef struct MJpegEncoder { > + VideoEncoder base; > uint8_t *row; > uint32_t row_size; > int first_frame; > @@ -165,14 +167,14 @@ struct MJpegEncoder { > void (*pixel_converter)(void *src, uint8_t *dest); > > MJpegEncoderRateControl rate_control; > - MJpegEncoderRateControlCbs cbs; > + VideoEncoderRateControlCbs cbs; > void *cbs_opaque; > > /* stats */ > uint64_t starting_bit_rate; > uint64_t avg_quality; > uint32_t num_frames; > -}; > +} MJpegEncoder; > > static void mjpeg_encoder_process_server_drops(MJpegEncoder *encoder); > static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size, > @@ -184,8 +186,9 @@ static inline int rate_control_is_active(MJpegEncoder* encoder) > return encoder->cbs.get_roundtrip_ms != NULL; > } > > -void mjpeg_encoder_destroy(MJpegEncoder *encoder) > +static void mjpeg_encoder_destroy(VideoEncoder *video_encoder) > { > + MJpegEncoder *encoder = (MJpegEncoder*)video_encoder; > jpeg_destroy_compress(&encoder->cinfo); > free(encoder->row); > free(encoder); > @@ -727,7 +730,7 @@ static int mjpeg_encoder_start_frame(MJpegEncoder *encoder, > 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); > @@ -775,14 +778,14 @@ static int mjpeg_encoder_start_frame(MJpegEncoder *encoder, > 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); > @@ -802,7 +805,7 @@ static int mjpeg_encoder_start_frame(MJpegEncoder *encoder, > > encoder->num_frames++; > encoder->avg_quality += quality; > - return MJPEG_ENCODER_FRAME_ENCODE_DONE; > + return VIDEO_ENCODER_FRAME_ENCODE_DONE; > } > > static int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder, > @@ -926,27 +929,30 @@ static int encode_frame(MJpegEncoder *encoder, const SpiceRect *src, > return TRUE; > } > > -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 int mjpeg_encoder_encode_frame(VideoEncoder *video_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) > { > + MJpegEncoder *encoder = (MJpegEncoder*)video_encoder; > + > int ret = mjpeg_encoder_start_frame(encoder, bitmap->format, > - width, height, outbuf, outbuf_size, > - frame_mm_time); > - if (ret != MJPEG_ENCODER_FRAME_ENCODE_DONE) { > + width, height, outbuf, outbuf_size, > + frame_mm_time); > + if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) { > return ret; > } > > if (!encode_frame(encoder, src, bitmap, top_down)) { > - return MJPEG_ENCODER_FRAME_UNSUPPORTED; > + return VIDEO_ENCODER_FRAME_UNSUPPORTED; > } > > *data_size = mjpeg_encoder_end_frame(encoder); > > - return MJPEG_ENCODER_FRAME_ENCODE_DONE; > + return VIDEO_ENCODER_FRAME_ENCODE_DONE; > } > > > @@ -1179,14 +1185,15 @@ 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(VideoEncoder *video_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) > { > + MJpegEncoder *encoder = (MJpegEncoder*)video_encoder; > MJpegEncoderRateControl *rate_control = &encoder->rate_control; > MJpegEncoderClientState *client_state = &rate_control->client_state; > uint64_t avg_enc_size = 0; > @@ -1287,8 +1294,9 @@ 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(VideoEncoder *video_encoder) > { > + MJpegEncoder *encoder = (MJpegEncoder*)video_encoder; > encoder->rate_control.server_state.num_frames_dropped++; > mjpeg_encoder_process_server_drops(encoder); > } > @@ -1325,27 +1333,36 @@ 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(VideoEncoder *video_encoder) > { > + MJpegEncoder *encoder = (MJpegEncoder*)video_encoder; > return encoder->rate_control.byte_rate * 8; > } > > -void mjpeg_encoder_get_stats(MJpegEncoder *encoder, MJpegEncoderStats *stats) > +static void mjpeg_encoder_get_stats(VideoEncoder *video_encoder, > + VideoEncoderStats *stats) > { > + MJpegEncoder *encoder = (MJpegEncoder*)video_encoder; > 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->cur_bit_rate = mjpeg_encoder_get_bit_rate(video_encoder); > stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames; > } > > -MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate, > - MJpegEncoderRateControlCbs *cbs, > +VideoEncoder *mjpeg_encoder_new(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; > @@ -1369,5 +1386,5 @@ MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate, > encoder->cinfo.err = jpeg_std_error(&encoder->jerr); > jpeg_create_compress(&encoder->cinfo); > > - return encoder; > + return (VideoEncoder*)encoder; > } > diff --git a/server/red_worker.c b/server/red_worker.c > index 68485a5..4f28e98 100644 > --- a/server/red_worker.c > +++ b/server/red_worker.c > @@ -71,7 +71,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" > @@ -485,7 +485,7 @@ typedef struct StreamAgent { > PipeItem destroy_item; > Stream *stream; > uint64_t last_send_time; > - MJpegEncoder *mjpeg_encoder; > + VideoEncoder *video_encoder; > DisplayChannelClient *dcc; > > int frames; > @@ -724,7 +724,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; > }; > @@ -2635,10 +2635,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" " > @@ -2681,8 +2681,8 @@ 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_agent->video_encoder && 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", > @@ -2989,7 +2989,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; > @@ -3010,7 +3010,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; > > @@ -3033,7 +3033,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) { > @@ -3046,9 +3046,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; > } > } > > @@ -3084,18 +3084,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 = mjpeg_encoder_new(initial_bit_rate, &video_cbs, agent); > } else { > - agent->mjpeg_encoder = mjpeg_encoder_new(0, NULL, NULL); > + agent->video_encoder = mjpeg_encoder_new(0, NULL, NULL); > } > red_channel_client_pipe_add(&dcc->common.base, &agent->create_item); > > @@ -3181,7 +3181,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); > } > > @@ -3193,9 +3193,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; > } > } > } > @@ -3322,7 +3322,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; > } > @@ -3331,8 +3331,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; > } > @@ -3345,7 +3345,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) { > @@ -8507,7 +8507,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 > @@ -8523,25 +8523,25 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc, > reds_get_mm_time(); > > outbuf_size = dcc->send_data.stream_outbuf_size; > - ret = mjpeg_encoder_encode_frame(agent->mjpeg_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); > + 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); > + Alignemnt of the parameters is off here > 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_DONE: > + case VIDEO_ENCODER_FRAME_ENCODE_DONE: > break; > default: > - spice_error("bad return value (%d) from mjpeg_encoder_encode_frame", ret); > + spice_error("bad return value (%d) from VideoEncoder::encode_frame", ret); > return FALSE; > } > dcc->send_data.stream_outbuf_size = outbuf_size; > @@ -10168,7 +10168,7 @@ static int display_channel_handle_stream_report(DisplayChannelClient *dcc, > return FALSE; > } > stream_agent = &dcc->stream_agents[stream_report->stream_id]; > - if (!stream_agent->mjpeg_encoder) { > + if (!stream_agent->video_encoder) { > spice_info("stream_report: no encoder for stream id %u." > "Probably the stream has been destroyed", stream_report->stream_id); > return TRUE; > @@ -10179,7 +10179,7 @@ static int display_channel_handle_stream_report(DisplayChannelClient *dcc, > stream_agent->report_id, stream_report->unique_id); > return TRUE; > } > - mjpeg_encoder_client_stream_report(stream_agent->mjpeg_encoder, > + stream_agent->video_encoder->client_stream_report(stream_agent->video_encoder, > stream_report->num_frames, > stream_report->num_drops, > stream_report->start_frame_mm_time, Aligment is off here too. > diff --git a/server/video_encoder.h b/server/video_encoder.h > new file mode 100644 > index 0000000..1640ed7 > --- /dev/null > +++ b/server/video_encoder.h > @@ -0,0 +1,162 @@ > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ > +/* > + Copyright (C) 2009 Red Hat, Inc. > + Copyright (C) 2015 Jeremy White > + Copyright (C) 2015 Francois Gouget > + > + 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 <<A HREF="http://www.gnu.org/licenses/">http://www.gnu.org/licenses/</A>>. Should just be <http://www.gnu.org/licences/>, not <<A HREF=..... > +*/ > + > +#ifndef _H_video_encoder > +#define _H_video_encoder Upper-case "video_encoder" Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel