Re: [PATCH spice 01/13] server: Refactor the Spice server video encoding so alternative implementations can be added. (take 4)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



This patch simply replaces the mjpeg_encoder_xxx() call points with a 
more object oriented
design.

Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
---


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 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) * 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;
 }
@@ -731,10 +696,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;
 
@@ -754,7 +720,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);
@@ -802,14 +768,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);
@@ -829,11 +795,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;
@@ -859,7 +826,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;
@@ -888,6 +855,95 @@ 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;
@@ -1058,6 +1114,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)
 {
@@ -1116,13 +1173,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;
@@ -1224,7 +1281,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);
@@ -1262,15 +1319,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 4584b36..e96d2fd 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 "jpeg_encoder.h"
@@ -483,7 +483,7 @@ typedef struct StreamAgent {
     PipeItem destroy_item;
     Stream *stream;
     uint64_t last_send_time;
-    MJpegEncoder *mjpeg_encoder;
+    VideoEncoder *video_encoder;
     DisplayChannelClient *dcc;
 
     int frames;
@@ -722,7 +722,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;
 };
@@ -2647,10 +2647,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" "
@@ -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);
         }
-        stream->refs++;
-        red_channel_client_pipe_add(&dcc->common.base, &stream_agent->destroy_item);
         red_print_stream_stats(dcc, stream_agent);
     }
     worker->streams_size_total -= stream->width * stream->height;
@@ -3001,7 +3002,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;
@@ -3022,7 +3023,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;
 
@@ -3045,7 +3046,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) {
@@ -3058,9 +3059,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;
     }
 }
 
@@ -3096,18 +3097,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);
 
@@ -3193,7 +3194,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);
 }
 
@@ -3205,9 +3206,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;
         }
     }
 }
@@ -3334,7 +3335,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;
         }
@@ -3343,8 +3344,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;
             }
@@ -3357,7 +3358,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) {
@@ -8472,70 +8473,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)
 {
@@ -8580,7 +8517,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
@@ -8594,33 +8531,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) {
@@ -10245,7 +10178,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;
@@ -10256,7 +10189,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,
diff --git a/server/video_encoder.h b/server/video_encoder.h
new file mode 100644
index 0000000..7d96351
--- /dev/null
+++ b/server/video_encoder.h
@@ -0,0 +1,165 @@
+/* -*- 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>>.
+*/
+
+#ifndef _H_video_encoder
+#define _H_video_encoder
+
+#include "red_common.h"
+
+enum {
+    VIDEO_ENCODER_FRAME_UNSUPPORTED = -1,
+    VIDEO_ENCODER_FRAME_DROP,
+    VIDEO_ENCODER_FRAME_ENCODE_DONE,
+};
+
+typedef struct VideoEncoderStats {
+    uint64_t starting_bit_rate;
+    uint64_t cur_bit_rate;
+    double avg_quality;
+} VideoEncoderStats;
+
+typedef struct VideoEncoder VideoEncoder;
+#ifndef VIDEO_ENCODER_T
+# define VIDEO_ENCODER_T VideoEncoder
+#endif
+
+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 video area. This always matches src.
+     * @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.
+     * @return:
+     *     VIDEO_ENCODER_FRAME_ENCODE_DONE if successful.
+     *     VIDEO_ENCODER_FRAME_UNSUPPORTED if the frame cannot be encoded.
+     *     VIDEO_ENCODER_FRAME_DROP if the frame was dropped. This value can
+     *                              only happen if rate control is active.
+     */
+    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);
+
+    /*
+     * Bit rate control methods.
+     */
+
+    /* When rate control is active statistics are periodically obtained from
+     * the client and sent to the video encoder through this method.
+     *
+     * @encoder:    The video encoder.
+     * @num_frames: The number of frames that reached the client during the
+     *              time period 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: This indicates how long in advance the client received
+     *              the last frame before having to display it.
+     * @audio delay: The latency of the audio playback or MAX_UINT if it is not
+     *              tracked.
+     */
+    void (*client_stream_report)(VIDEO_ENCODER_T *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);
+
+    /* This notifies the video encoder each time a frame is dropped due to pipe
+     * congestion.
+     *
+     * Note that frames are being dropped before they are encoded and that there
+     * may be any number of encoded frames in the network queue.
+     * The client reports provide richer and typically more reactive information
+     * for fine tuning the playback parameters but this function provides a
+     * fallback when client reports are getting delayed or are not supported
+     * by the client.
+     *
+     * @encoder:    The video encoder.
+     */
+    void (*notify_server_frame_drop)(VIDEO_ENCODER_T *encoder);
+
+    /* This queries the video encoder's current bit rate.
+     *
+     * @encoder:    The video encoder.
+     * @return:     The current bit rate in bits per second.
+     */
+    uint64_t (*get_bit_rate)(VIDEO_ENCODER_T *encoder);
+
+    /* Collects video statistics.
+     *
+     * @encoder:    The video encoder.
+     * @stats:      A VideoEncoderStats structure to fill with the collected
+     *              statistics.
+     */
+    void (*get_stats)(VIDEO_ENCODER_T *encoder, VideoEncoderStats *stats);
+};
+
+
+/* When rate control is active the video encoder can use these callbacks to
+ * figure out how to adjust the stream bit rate and adjust some stream
+ * parameters.
+ */
+typedef struct VideoEncoderRateControlCbs {
+    /* Returns the stream's estimated roundtrip time in milliseconds. */
+    uint32_t (*get_roundtrip_ms)(void *opaque);
+
+    /* Returns the estimated input frame rate.
+     *
+     * This is the number of frames per second arriving from the guest to
+     * spice-server, before any drops.
+     */
+    uint32_t (*get_source_fps)(void *opaque);
+
+    /* Informs the client of the minimum playback delay.
+     *
+     * @delay_ms:   The minimum number of milliseconds required for the frames
+     *              to reach the client.
+     */
+    void (*update_client_playback_delay)(void *opaque, uint32_t delay_ms);
+} VideoEncoderRateControlCbs;
+
+
+/* Instantiates the builtin MJPEG video encoder.
+ *
+ * @starting_bit_rate: An initial estimate of the available stream bit rate .
+ * @bit_rate_control:  True if the client supports rate control.
+ * @cbs:               A set of callback methods to be used for rate control.
+ * @cbs_opaque:        A pointer to be passed to the rate control callbacks.
+ * @return:            A pointer to a structure implementing the VideoEncoder
+ *                     methods.
+ */
+VIDEO_ENCODER_T* create_mjpeg_encoder(uint64_t starting_bit_rate,
+                                      VideoEncoderRateControlCbs *cbs,
+                                      void *cbs_opaque);
+
+#endif



-- 
Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]