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