[Spice-devel] [PATCH spice-server 08/28] mjpeg_encoder: move the control over frame drops to mjpeg_encoder

[Alon Levy]

On Tue, Feb 26, 2013 at 01:03:54PM -0500, Yonit Halperin wrote:

ACK with one comment.

> ---
>  server/mjpeg_encoder.c | 20 +++++++++-----------
>  server/mjpeg_encoder.h | 21 ++++++++++++++-------
>  server/red_worker.c    | 21 ++++++++++++++++-----
>  3 files changed, 39 insertions(+), 23 deletions(-)
> 
> diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
> index 70b6338..c124286 100644
> --- a/server/mjpeg_encoder.c
> +++ b/server/mjpeg_encoder.c
> @@ -641,9 +641,15 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
>          MJpegEncoderRateControl *rate_control = &encoder->rate_control;
>          struct timespec time;
>          uint64_t now;
> +        uint64_t interval;
>  
>          clock_gettime(CLOCK_MONOTONIC, &time);
>          now = ((uint64_t) time.tv_sec) * 1000000000 + time.tv_nsec;
> +        interval = (now - rate_control->bit_rate_info.last_frame_time);
> +
> +        if (interval < (1000*1000*1000) / rate_control->fps) {
> +            return MJPEG_ENCODER_FRAME_DROP;
> +        }
>  
>          mjpeg_encoder_adjust_params_to_bit_rate(encoder);
>  
> @@ -690,14 +696,14 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
>          break;
>      default:
>          spice_warning("unsupported format %d", format);
> -        return FALSE;
> +        return MJPEG_ENCODER_FRAME_UNSUPPORTED;
>      }
>  
>      if (encoder->pixel_converter != NULL) {
>          unsigned int stride = width * 3;
>          /* check for integer overflow */
>          if (stride < width) {
> -            return FALSE;
> +            return MJPEG_ENCODER_FRAME_UNSUPPORTED;
>          }
>          if (encoder->row_size < stride) {
>              encoder->row = spice_realloc(encoder->row, stride);
> @@ -715,7 +721,7 @@ int mjpeg_encoder_start_frame(MJpegEncoder *encoder, SpiceBitmapFmt format,
>      jpeg_set_quality(&encoder->cinfo, quality, TRUE);
>      jpeg_start_compress(&encoder->cinfo, encoder->first_frame);
>  
> -    return TRUE;
> +    return MJPEG_ENCODER_FRAME_ENCODE_START;
>  }
>  
>  int mjpeg_encoder_encode_scanline(MJpegEncoder *encoder, uint8_t *src_pixels,
> @@ -773,14 +779,6 @@ size_t mjpeg_encoder_end_frame(MJpegEncoder *encoder)
>      return encoder->rate_control.last_enc_size;
>  }
>  
> -uint32_t mjpeg_encoder_get_fps(MJpegEncoder *encoder)
> -{
> -    if (!encoder->rate_control_is_active) {
> -        spice_warning("bit rate control is not active");
> -    }
> -    return encoder->rate_control.fps;
> -}
> -
>  static void mjpeg_encoder_quality_eval_stop(MJpegEncoder *encoder)
>  {
>      MJpegEncoderRateControl *rate_control = &encoder->rate_control;
> diff --git a/server/mjpeg_encoder.h b/server/mjpeg_encoder.h
> index f9ae43c..0ee2e96 100644
> --- a/server/mjpeg_encoder.h
> +++ b/server/mjpeg_encoder.h
> @@ -21,6 +21,12 @@
>  
>  #include "red_common.h"
>  
> +enum {
> +    MJPEG_ENCODER_FRAME_UNSUPPORTED = -1,
> +    MJPEG_ENCODER_FRAME_DROP,
> +    MJPEG_ENCODER_FRAME_ENCODE_START,
> +};
> +
>  typedef struct MJpegEncoder MJpegEncoder;
>  
>  /*
> @@ -44,8 +50,15 @@ 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
> + * 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,
> @@ -60,12 +73,6 @@ size_t mjpeg_encoder_end_frame(MJpegEncoder *encoder);
>   */
>  
>  /*
> - * The recommended output frame rate (per second) for the
> - * current available bit rate.
> - */
> -uint32_t mjpeg_encoder_get_fps(MJpegEncoder *encoder);
> -
> -/*
>   * 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.
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 568b8e5..a390629 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8352,6 +8352,7 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
>      RedWorker *worker = dcc->common.worker;
>      int n;
>      int width, height;
> +    int ret;
>  
>      if (!stream) {
>          spice_assert(drawable->sized_stream);
> @@ -8389,13 +8390,23 @@ static inline int red_marshall_stream_data(RedChannelClient *rcc,
>      }
>  
>      outbuf_size = dcc->send_data.stream_outbuf_size;
> -    if (!mjpeg_encoder_start_frame(agent->mjpeg_encoder, image->u.bitmap.format,
> -                                   width, height,
> -                                   &dcc->send_data.stream_outbuf,
> -                                   &outbuf_size,
> -                                   drawable->red_drawable->mm_time)) {
> +    ret = mjpeg_encoder_start_frame(agent->mjpeg_encoder, image->u.bitmap.format,
> +                                    width, height,
> +                                    &dcc->send_data.stream_outbuf,
> +                                    &outbuf_size,
> +                                    drawable->red_drawable->mm_time);
> +    switch (ret) {
> +    case MJPEG_ENCODER_FRAME_DROP:
> +        spice_warning("mjpeg rate control is not supported yet");
> +        return TRUE;
> +    case MJPEG_ENCODER_FRAME_UNSUPPORTED:
>          return FALSE;
> +    case MJPEG_ENCODER_FRAME_ENCODE_START:
> +        break;
> +    default:
> +        spice_error("bad return value (%d) from mjpeg_encoder_start_frame", ret);

Why don't we return here?

>      }
> +
>      if (!encode_frame(dcc, &drawable->red_drawable->u.copy.src_area,
>                        &image->u.bitmap, stream)) {
>          return FALSE;
> -- 
> 1.8.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel