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

[Christophe Fergeau]

On Mon, Sep 28, 2015 at 09:36:02PM +0200, Francois Gouget wrote:
> On Mon, 21 Sep 2015, Christophe Fergeau wrote:
> > > @@ -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?

I'm not really concerned, but this patch is changing about 600 lines of
code, most of it is straightforward renaming/code movement, but this
hunk is imo a more involved rework as this changes from
start_frame/encode_frame/end_frame to a single encode_frame() helper.
In other words, while most of this patch can be reviewed in 'auto' mode
by comparing code before/after, this specific change requires more
thinking/careful review, so I would have moved it to a commit of its
own.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150929/6e5ac7a7/attachment.sig>