[Spice-devel] [PATCH v2 01/18] worker: move encoders to dcc-encoders

[Frediano Ziglio]

> 
> Hi,
> 
> I know this was merged already but just one comment bellow:
> 
> On Wed, Nov 18, 2015 at 03:42:26PM -0600, Jonathon Jongsma wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > +static SPICE_GNUC_NORETURN SPICE_GNUC_PRINTF(2, 3) void
> > +quic_usr_error(QuicUsrContext *usr, const char *fmt, ...)
> > +{
> > +    EncoderData *usr_data = &(((QuicData *)usr)->data);
> > +    va_list ap;
> 
> I would prefer some #define here as well to get the EncoderData..
> 
> > +static SPICE_GNUC_NORETURN SPICE_GNUC_PRINTF(2, 3) void
> > +lz_usr_error(LzUsrContext *usr, const char *fmt, ...)
> > +{
> > +    EncoderData *usr_data = &(((LzData *)usr)->data);
> > +    va_list ap;
> 
> As it is used a lot..
> 
> > +static SPICE_GNUC_PRINTF(2, 3) void
> > +glz_usr_error(GlzEncoderUsrContext *usr, const char *fmt, ...)
> > +{
> > +    EncoderData *usr_data = &(((GlzData *)usr)->data);
> > +    va_list ap;
> 
> +1
> 
> > +static SPICE_GNUC_PRINTF(2, 3) void
> > +quic_usr_warn(QuicUsrContext *usr, const char *fmt, ...)
> > +{
> > +    EncoderData *usr_data = &(((QuicData *)usr)->data);
> > +    va_list ap;
> 
> +1
> 
> > +static SPICE_GNUC_PRINTF(2, 3) void
> > +lz_usr_warn(LzUsrContext *usr, const char *fmt, ...)
> > +{
> > +    EncoderData *usr_data = &(((LzData *)usr)->data);
> > +    va_list ap;
> 
> +1
> 
> > +static SPICE_GNUC_PRINTF(2, 3) void
> > +glz_usr_warn(GlzEncoderUsrContext *usr, const char *fmt, ...)
> > +{
> > +    EncoderData *usr_data = &(((GlzData *)usr)->data);
> > +    va_list ap;
> 
> +1
> 
> > +static int quic_usr_more_space(QuicUsrContext *usr, uint32_t **io_ptr, int
> > rows_completed)
> > +{
> > +    EncoderData *usr_data = &(((QuicData *)usr)->data);
> > +    return encoder_usr_more_space(usr_data, (uint8_t **)io_ptr) /
> > sizeof(uint32_t);
> > +}
> > +
> > +static int lz_usr_more_space(LzUsrContext *usr, uint8_t **io_ptr)
> > +{
> > +    EncoderData *usr_data = &(((LzData *)usr)->data);
> > +    return encoder_usr_more_space(usr_data, io_ptr);
> > +}
> > +
> > +static int glz_usr_more_space(GlzEncoderUsrContext *usr, uint8_t **io_ptr)
> > +{
> > +    EncoderData *usr_data = &(((GlzData *)usr)->data);
> > +    return encoder_usr_more_space(usr_data, io_ptr);
> > +}
> > +
> > +static int jpeg_usr_more_space(JpegEncoderUsrContext *usr, uint8_t
> > **io_ptr)
> > +{
> > +    EncoderData *usr_data = &(((JpegData *)usr)->data);
> > +    return encoder_usr_more_space(usr_data, io_ptr);
> > +}
> > +
> > +#ifdef USE_LZ4
> > +static int lz4_usr_more_space(Lz4EncoderUsrContext *usr, uint8_t **io_ptr)
> > +{
> > +    EncoderData *usr_data = &(((Lz4Data *)usr)->data);
> > +    return encoder_usr_more_space(usr_data, io_ptr);
> > +}
> > +#endif
> > +
> > +static int zlib_usr_more_space(ZlibEncoderUsrContext *usr, uint8_t
> > **io_ptr)
> > +{
> > +    EncoderData *usr_data = &(((ZlibData *)usr)->data);
> > +    return encoder_usr_more_space(usr_data, io_ptr);
> > +}
> 
> +6
> 
> > +static int quic_usr_more_lines(QuicUsrContext *usr, uint8_t **lines)
> > +{
> > +    EncoderData *usr_data = &(((QuicData *)usr)->data);
> > +    return encoder_usr_more_lines(usr_data, lines);
> > +}
> > +
> > +static int lz_usr_more_lines(LzUsrContext *usr, uint8_t **lines)
> > +{
> > +    EncoderData *usr_data = &(((LzData *)usr)->data);
> > +    return encoder_usr_more_lines(usr_data, lines);
> > +}
> > +
> > +static int glz_usr_more_lines(GlzEncoderUsrContext *usr, uint8_t **lines)
> > +{
> > +    EncoderData *usr_data = &(((GlzData *)usr)->data);
> > +    return encoder_usr_more_lines(usr_data, lines);
> > +}
> > +
> > +static int jpeg_usr_more_lines(JpegEncoderUsrContext *usr, uint8_t
> > **lines)
> > +{
> > +    EncoderData *usr_data = &(((JpegData *)usr)->data);
> > +    return encoder_usr_more_lines(usr_data, lines);
> > +}
> > +
> > +#ifdef USE_LZ4
> > +static int lz4_usr_more_lines(Lz4EncoderUsrContext *usr, uint8_t **lines)
> > +{
> > +    EncoderData *usr_data = &(((Lz4Data *)usr)->data);
> > +    return encoder_usr_more_lines(usr_data, lines);
> > +}
> > +#endif
> > +
> > +static int zlib_usr_more_input(ZlibEncoderUsrContext *usr, uint8_t**
> > input)
> > +{
> > +    EncoderData *usr_data = &(((ZlibData *)usr)->data);
> > +    int buf_size;
> 
> +6
> 
> Cheers,
>   - toso

I think that being a move was not good to add other stuff.

However I agree with a patch for these.

Personally I would like to see a macro like

  #define GET_USR_DATA(type) &(SPICE_CONTAINEROF(usr, JpegData, usr)->data)

and something like

  return encoder_usr_more_space(GET_USR_DATA(JpegData), io_ptr);


Looks like now is easier to integrate patches as the bigger moves
was integrated in master.

Frediano