[Spice-devel] [PATCH 05/30] Encapsulate quic information in a new EncodersData structure

Jonathon Jongsma jjongsma at redhat.com
Wed Jun 8 20:32:17 UTC 2016


On Tue, 2016-06-07 at 11:17 +0100, Frediano Ziglio wrote:
> Start putting all encoding code into dcc-encoders.c.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/dcc-encoders.c | 27 ++++++++++++++-------------
>  server/dcc-encoders.h |  6 ++++++
>  server/dcc.c          | 14 +++++++-------
>  server/dcc.h          |  5 +++--
>  4 files changed, 30 insertions(+), 22 deletions(-)
> 
> diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> index 2bf68d0..7d317d5 100644
> --- a/server/dcc-encoders.c
> +++ b/server/dcc-encoders.c
> @@ -291,19 +291,19 @@ static int zlib_usr_more_input(ZlibEncoderUsrContext
> *usr, uint8_t** input)
>      return buf_size;
>  }
>  
> -static void dcc_init_quic(DisplayChannelClient *dcc)
> +static void dcc_init_quic(EncodersData *enc)

If these are no longer accepting a dcc argument, it seems to me that they're no
longer DisplayChannelClient, so it doesn't really make sense to name them dcc_*.
Something like encoders_data_init_quic() would seem more appropriate.

>  {
> -    dcc->quic_data.usr.error = quic_usr_error;
> -    dcc->quic_data.usr.warn = quic_usr_warn;
> -    dcc->quic_data.usr.info = quic_usr_warn;
> -    dcc->quic_data.usr.malloc = quic_usr_malloc;
> -    dcc->quic_data.usr.free = quic_usr_free;
> -    dcc->quic_data.usr.more_space = quic_usr_more_space;
> -    dcc->quic_data.usr.more_lines = quic_usr_more_lines;
> +    enc->quic_data.usr.error = quic_usr_error;
> +    enc->quic_data.usr.warn = quic_usr_warn;
> +    enc->quic_data.usr.info = quic_usr_warn;
> +    enc->quic_data.usr.malloc = quic_usr_malloc;
> +    enc->quic_data.usr.free = quic_usr_free;
> +    enc->quic_data.usr.more_space = quic_usr_more_space;
> +    enc->quic_data.usr.more_lines = quic_usr_more_lines;
>  
> -    dcc->quic = quic_create(&dcc->quic_data.usr);
> +    enc->quic = quic_create(&enc->quic_data.usr);
>  
> -    if (!dcc->quic) {
> +    if (!enc->quic) {
>          spice_critical("create quic failed");
>      }
>  }
> @@ -401,7 +401,7 @@ static void dcc_init_zlib(DisplayChannelClient *dcc)
>  void dcc_encoders_init(DisplayChannelClient *dcc)
>  {
>      dcc_init_glz_data(dcc);
> -    dcc_init_quic(dcc);
> +    dcc_init_quic(&dcc->encoders);
>      dcc_init_lz(dcc);
>      dcc_init_jpeg(dcc);
>  #ifdef USE_LZ4
> @@ -415,8 +415,9 @@ void dcc_encoders_init(DisplayChannelClient *dcc)
>  
>  void dcc_encoders_free(DisplayChannelClient *dcc)
>  {
> -    quic_destroy(dcc->quic);
> -    dcc->quic = NULL;
> +    EncodersData *enc = &dcc->encoders;
> +    quic_destroy(enc->quic);
> +    enc->quic = NULL;
>      lz_destroy(dcc->lz);
>      dcc->lz = NULL;
>      jpeg_encoder_destroy(dcc->jpeg);
> diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> index 95790b0..6bed548 100644
> --- a/server/dcc-encoders.h
> +++ b/server/dcc-encoders.h
> @@ -34,6 +34,7 @@
>  typedef struct RedCompressBuf RedCompressBuf;
>  typedef struct GlzDrawableInstanceItem GlzDrawableInstanceItem;
>  typedef struct RedGlzDrawable RedGlzDrawable;
> +typedef struct EncodersData EncodersData;
>  
>  void             dcc_encoders_init                           (DisplayChannelC
> lient *dcc);
>  void             dcc_encoders_free                           (DisplayChannelC
> lient *dcc);
> @@ -161,6 +162,11 @@ struct RedGlzDrawable {
>      DisplayChannelClient *dcc;
>  };
>  
> +struct EncodersData {
> +    QuicData quic_data;
> +    QuicContext *quic;
> +};
> +

Will this struct eventually contain other encoder-related data? It seems weird
to have the name be EncodersData (plural encoders) when it only contains data
for a single encoder (quic).

>  #define RED_RELEASE_BUNCH_SIZE 64
>  
>  #endif /* DCC_ENCODERS_H_ */
> diff --git a/server/dcc.c b/server/dcc.c
> index a88f8e6..27e668c 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -1026,15 +1026,15 @@ static int dcc_compress_image_lz4(DisplayChannelClient
> *dcc, SpiceImage *dest,
>  }
>  #endif
>  
> -static int dcc_compress_image_quic(DisplayChannelClient *dcc, SpiceImage
> *dest,
> -                                   SpiceBitmap *src, compress_send_data_t*
> o_comp_data)
> +static int dcc_compress_image_quic(EncodersData *enc, SpiceImage *dest,
> +                                   SpiceBitmap *src, compress_send_data_t*
> o_comp_data, stat_info_t *stats)

again, the dcc_* prefix here is no longer really appropriate.

>  {
> -    QuicData *quic_data = &dcc->quic_data;
> -    QuicContext *quic = dcc->quic;
> +    QuicData *quic_data = &enc->quic_data;
> +    QuicContext *quic = enc->quic;
>      volatile QuicImageType type;
>      int size, stride;
>      stat_start_time_t start_time;
> -    stat_start_time_init(&start_time, &DCC_TO_DC(dcc)->quic_stat);
> +    stat_start_time_init(&start_time, stats);

This looks like an improvment to avoid poking into the DisplayChannel from this
function, but it's unrelated to the EncodersData change. It should be done in a
separate commit.

>  
>  #ifdef COMPRESS_DEBUG
>      spice_info("QUIC compress");
> @@ -1094,7 +1094,7 @@ static int dcc_compress_image_quic(DisplayChannelClient
> *dcc, SpiceImage *dest,
>      o_comp_data->comp_buf = quic_data->data.bufs_head;
>      o_comp_data->comp_buf_size = size << 2;
>  
> -    stat_compress_add(&DCC_TO_DC(dcc)->quic_stat, start_time, src->stride *
> src->y,
> +    stat_compress_add(stats, start_time, src->stride * src->y,
>                        o_comp_data->comp_buf_size);
>      return TRUE;
>  }
> @@ -1205,7 +1205,7 @@ int dcc_compress_image(DisplayChannelClient *dcc,
>              success = dcc_compress_image_jpeg(dcc, dest, src, o_comp_data);
>              break;
>          }
> -        success = dcc_compress_image_quic(dcc, dest, src, o_comp_data);
> +        success = dcc_compress_image_quic(&dcc->encoders, dest, src,
> o_comp_data, &display_channel->quic_stat);
>          break;
>      case SPICE_IMAGE_COMPRESSION_GLZ:
>          if ((src->x * src->y) < glz_enc_dictionary_get_size(dcc->glz_dict-
> >dict)) {
> diff --git a/server/dcc.h b/server/dcc.h
> index a11d25a..0ac5f5a 100644
> --- a/server/dcc.h
> +++ b/server/dcc.h
> @@ -61,10 +61,11 @@ struct DisplayChannelClient {
>      spice_wan_compression_t jpeg_state;
>      spice_wan_compression_t zlib_glz_state;
>      int jpeg_quality;
> +
> +    EncodersData encoders;
> +
>      int zlib_level;
>  
> -    QuicData quic_data;
> -    QuicContext *quic;
>      LzData lz_data;
>      LzContext  *lz;
>      JpegData jpeg_data;

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>




More information about the Spice-devel mailing list