[Spice-devel] [PATCH v5 02/20] server: Refactor the Spice server video encoding so alternative implementations can be added.

[Francois Gouget]

On Mon, 21 Sep 2015, Christophe Fergeau wrote:
[...]
> > +typedef struct MJpegEncoder MJpegEncoder;
> > +#define VIDEO_ENCODER_T MJpegEncoder
> 
> Still not a big fan of this #define magic

Ok. I removed it. I attached the new VIDEO_ENCODER_T-less patch so you 
can validate the new approach.


> > +            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?

I rechecked this and it does not actually seem to be necessary. So I 
removed it.


> > @@ -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.

Which part concerns you?

-- 
Francois Gouget <fgouget at codeweavers.com>              
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02-refactor.diff
Type: text/x-diff
Size: 41108 bytes
Desc: 02-refactor.diff
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150928/34931273/attachment-0001.diff>