Hello
El Miércoles, 21 de enero de 2015 17:05:00 Christophe Fergeau escribió:
> Hey,
>
> Sorry for the late review...
>
> On Thu, Jan 15, 2015 at 12:50:21PM +0100, Javier Celaya wrote:
> > This patch limits the LZ4 algorithm to RGB formats. Other formats are
> > compressed with LZ. It also sends the top_down flag and the original
> > format to the client, so that it can create the right kind of pixman
> > surface.
>
> Looking at the other compression formats, it seems they don't send a
> top down flag, but instead 'revert' the image before sending it, any
> reason this is not being done here?
It depends on the compression format. JPEG, for instance, reverts the image,
but LZ does send the top down flag (see spice-common/common/lz.c). I suppose it
is simpler, since pixman can hadle both direction in the client. The JPEG
format, on the other hand, probably forces the top-down direction.
>
> This patch breaks the wire protocol which was used in spice-gtk 0.27,
> which we usually don't do, but since what was in the release was
> crashing when being used, I'd say this is fine to change.
Yes, I thought the same...
>
> This commit could have easily been split in different ones (limit lz4 to
> RGB formats, send the original format to the client, ...
I can split it if you want.
>
> Apart from these comments, this looks good.
Thanks
Javi
>
> Christophe
>
> > ---
> >
> > server/lz4_encoder.c | 21 +++++++++++----------
> > server/lz4_encoder.h | 7 +++----
> > server/red_worker.c | 21 +++++++--------------
> > 3 files changed, 21 insertions(+), 28 deletions(-)
> >
> > diff --git a/server/lz4_encoder.c b/server/lz4_encoder.c
> > index 531ab4b..bb8e98a 100644
> > --- a/server/lz4_encoder.c
> > +++ b/server/lz4_encoder.c
> > @@ -49,22 +49,23 @@ void lz4_encoder_destroy(Lz4EncoderContext* encoder)
> >
> > free(encoder);
> >
> > }
> >
> > -int lz4_encode(Lz4EncoderContext *lz4, int height, int stride,
> > - uint8_t *io_ptr, unsigned int num_io_bytes)
> > +int lz4_encode(Lz4EncoderContext *lz4, int height, int stride, uint8_t
> > *io_ptr, + unsigned int num_io_bytes, int top_down, uint8_t
> > format)>
> > {
> >
> > Lz4Encoder *enc = (Lz4Encoder *)lz4;
> > uint8_t *lines;
> > int num_lines = 0;
> > int total_lines = 0;
> >
> > - int in_size, enc_size, out_size = 0, already_copied;
> > - int stride_abs = abs(stride);
> > + int in_size, enc_size, out_size, already_copied;
> >
> > uint8_t *in_buf, *compressed_lines;
> > uint8_t *out_buf = io_ptr;
> > LZ4_stream_t *stream = LZ4_createStream();
> >
> > - // Encode direction
> > - *(out_buf++) = stride < 0 ? 1 : 0;
> > - num_io_bytes--;
> > + // Encode direction and format
> > + *(out_buf++) = top_down ? 1 : 0;
> > + *(out_buf++) = format;
> > + out_size = 2;
> > + num_io_bytes -= 2;
> >
> > do {
> >
> > num_lines = enc->usr->more_lines(enc->usr, &lines);
> >
> > @@ -73,9 +74,9 @@ int lz4_encode(Lz4EncoderContext *lz4, int height, int
> > stride,>
> > LZ4_freeStream(stream);
> > return 0;
> >
> > }
> >
> > - in_buf = stride < 0 ? lines - (stride_abs * (num_lines - 1)) :
> > lines; - lines += stride * num_lines;
> > - in_size = stride_abs * num_lines;
> > + in_buf = lines;
> > + in_size = stride * num_lines;
> > + lines += in_size;
> >
> > compressed_lines = (uint8_t *) malloc(LZ4_compressBound(in_size)
> > + 4);
> > enc_size = LZ4_compress_continue(stream, (const char *) in_buf,
> >
> > (char *) compressed_lines + 4,
> > in_size);
> >
> > diff --git a/server/lz4_encoder.h b/server/lz4_encoder.h
> > index f3359c0..f1de12a 100644
> > --- a/server/lz4_encoder.h
> > +++ b/server/lz4_encoder.h
> > @@ -43,8 +43,7 @@ struct Lz4EncoderUsrContext {
> >
> > Lz4EncoderContext* lz4_encoder_create(Lz4EncoderUsrContext *usr);
> > void lz4_encoder_destroy(Lz4EncoderContext *encoder);
> >
> > -/* returns the total size of the encoded data. Images must be supplied
> > from the - top line to the bottom */
> > -int lz4_encode(Lz4EncoderContext *lz4, int height, int stride,
> > - uint8_t *io_ptr, unsigned int num_io_bytes);
> > +/* returns the total size of the encoded data. */
> > +int lz4_encode(Lz4EncoderContext *lz4, int height, int stride, uint8_t
> > *io_ptr, + unsigned int num_io_bytes, int top_down, uint8_t
> > format);>
> > #endif
> >
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index a18987a..16411a7 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -6422,7 +6422,6 @@ static int
> > red_lz4_compress_image(DisplayChannelClient *dcc, SpiceImage *dest,>
> > Lz4Data *lz4_data = &worker->lz4_data;
> > Lz4EncoderContext *lz4 = worker->lz4;
> > int lz4_size = 0;
> >
> > - int stride;
> >
> > #ifdef COMPRESS_STAT
> >
> > stat_time_t start_time = stat_now();
> >
> > @@ -6454,20 +6453,12 @@ static int
> > red_lz4_compress_image(DisplayChannelClient *dcc, SpiceImage *dest,>
> > lz4_data->data.u.lines_data.chunks = src->data;
> > lz4_data->data.u.lines_data.stride = src->stride;
> >
> > + lz4_data->data.u.lines_data.next = 0;
> > + lz4_data->data.u.lines_data.reverse = 0;
> >
> > lz4_data->usr.more_lines = lz4_usr_more_lines;
> >
> > -
> > - if ((src->flags & SPICE_BITMAP_FLAGS_TOP_DOWN)) {
> > - lz4_data->data.u.lines_data.next = 0;
> > - lz4_data->data.u.lines_data.reverse = 0;
> > - stride = src->stride;
> > - } else {
> > - lz4_data->data.u.lines_data.next = src->data->num_chunks - 1;
> > - lz4_data->data.u.lines_data.reverse = 1;
> > - stride = -src->stride;
> > - }
> > -
> > - lz4_size = lz4_encode(lz4, src->y, stride,
> > (uint8_t*)lz4_data->data.bufs_head->buf, -
> > sizeof(lz4_data->data.bufs_head->buf)); + lz4_size = lz4_encode(lz4,
> > src->y, src->stride, (uint8_t*)lz4_data->data.bufs_head->buf, +
> > sizeof(lz4_data->data.bufs_head->buf),
> > + src->flags & SPICE_BITMAP_FLAGS_TOP_DOWN,
> > src->format);>
> > // the compressed buffer is bigger than the original data
> > if (lz4_size > (src->y * src->stride)) {
> >
> > @@ -6678,6 +6669,7 @@ static inline int
> > red_compress_image(DisplayChannelClient *dcc,>
> > if (!glz) {
> >
> > #ifdef USE_LZ4
> >
> > if (image_compression == SPICE_IMAGE_COMPRESS_LZ4 &&
> >
> > + bitmap_fmt_is_rgb(src->format) &&
> >
> > red_channel_client_test_remote_cap(&dcc->common.base,
> >
> > SPICE_DISPLAY_CAP_LZ4_COMPRESSION)) {
> >
> > ret = red_lz4_compress_image(dcc, dest, src, o_comp_data,
> >
> > @@ -8918,6 +8910,7 @@ static void red_marshall_image(RedChannelClient
> > *rcc, SpiceMarshaller *m, ImageI>
> > } else {
> >
> > #ifdef USE_LZ4
> >
> > if (comp_mode == SPICE_IMAGE_COMPRESS_LZ4 &&
> >
> > + bitmap_fmt_is_rgb(bitmap.format) &&
> >
> > red_channel_client_test_remote_cap(&dcc->common.base,
> >
> > SPICE_DISPLAY_CAP_LZ4_COMPRESSION)) {
> >
> > comp_succeeded = red_lz4_compress_image(dcc, &red_image,
> > &bitmap,
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel