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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel