[Spice-devel] [PATCH] Add some documentation to image-encoders header

[Jonathon Jongsma]

On Mon, 2016-06-20 at 10:09 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/image-encoders.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/server/image-encoders.h b/server/image-encoders.h
> index e07c036..230dd7c 100644
> --- a/server/image-encoders.h
> +++ b/server/image-encoders.h
> @@ -45,15 +45,49 @@ void image_encoder_shared_stat_print(const
> ImageEncoderSharedData *shared_data);
>  
>  void image_encoders_init(ImageEncoders *enc, ImageEncoderSharedData
> *shared_data);
>  void image_encoders_free(ImageEncoders *enc);
> +
> +/**
> + * Free Glz images which are no more retained.
> + * Caller should prevent encoding using the dictionary during the operation.
> + */


It would be a bit more natural to say "no longer retained" or "not retained
anymore". But that leads to the question about what it means to be "retained".
Perhaps add a "see glz_image_retention_detach_drawables() for more information
about retaining images"?

By the way, the .c file already has a similar but slightly different comment for
this function: 

/*
 * Remove from the global lz dictionary some glz_drawables that have no
reference to
 * Drawable (their qxl drawables are released too).
 * NOTE - the caller should prevent encoding using the dictionary during the
operation
 */

What should we do about the documentation in the .c file? Remove it in favor of
the header documentation? I don't know if it's a good idea to have the
documentation in two places as it will inevitably get out-of-sync.

>  int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc);
> +
> +/**
> + * Free all Glz images build from these encoders.
> + */

It's not entirely clear to me what is meant by "build from these encoders". I
think that I might change that to:

"Free all Glz images created by these encoders".

Does that sound like the same meaning you're trying to convey? By the way, the
.c file again has slightly different documentation for this function:

/* Clear all lz drawables - enforce their removal from the global dictionary.
   NOTE - prevents encoding using the dictionary during the operation*/


>  void image_encoders_free_glz_drawables(ImageEncoders *enc);
> +
> +/**
> + * Free all Glz images marked as freed.
> + * Images are marked as freed when removed from Glz dictionary but
> + * was not possible to free as the removal was from another thread.
> + */

Minor wording suggestion: 

"Images are marked as freed when they were removed from the Glz dictionary but
could not be freed since the removal was done from another thread."

>  void image_encoders_free_glz_drawables_to_free(ImageEncoders* enc);
> +
>  gboolean image_encoders_glz_create(ImageEncoders *enc, uint8_t id);
>  void image_encoders_glz_get_restore_data(ImageEncoders *enc,
>                                           uint8_t *out_id,
> GlzEncDictRestoreData *out_data);
> +
> +/**
> + * Disable Glz encoding on a given encoder.
> + */
>  gboolean image_encoders_glz_encode_lock(ImageEncoders *enc);
> +
> +/**
> + * Enable again Glz encoding on a given encoder.

"Enable again" -> "Re-enable"

> + */
>  void image_encoders_glz_encode_unlock(ImageEncoders *enc);
> +
> +/**
> + * Free all Glz images associated with a retention structure.
> + * Caller should prevent encoding using the dictionary during the operation.
> + */
>  void glz_retention_free_drawables(GlzImageRetention *ret);
> +
> +/**
> + * Mark all Glz images associated with a retention structure
> + * as not retained.
> + */
>  void glz_retention_detach_drawables(GlzImageRetention *ret);
>  
>  #define RED_COMPRESS_BUF_SIZE (1024 * 64)
> @@ -132,6 +166,12 @@ typedef struct {
>      EncoderData data;
>  } GlzData;
>  
> +/**
> + * This structure is used to associate Glz images to be freed.
> + * Can be used to notify that attached images should be freed
> + * or are no more associated and could be freed later if
> + * needed.
> + */

This comment is a bit unclear to me as well. 

The first sentence implies to me that it's just a structure that holds images
that need to be freed. But it actually holds all images, not just ones that need
to be freed, right? And how is the structure used to "notify"?

>  struct GlzImageRetention {
>      Ring ring;
>  };
> @@ -204,6 +244,14 @@ int image_encoders_compress_jpeg(ImageEncoders *enc,
> SpiceImage *dest,
>                                   SpiceBitmap *src, compress_send_data_t*
> o_comp_data);
>  int image_encoders_compress_lz4(ImageEncoders *enc, SpiceImage *dest,
>                                  SpiceBitmap *src, compress_send_data_t*
> o_comp_data);
> +
> +/**
> + * Compress an image with Glz.
> + * Glz use a shared dictionary which needs to access images stored

"use" -> "uses"

> + * in RedDrawables even after the compression took place.
> + * For this reason these drawables are marked in a GlzImageRetention
> + * structure.

"marked" seems like a strange verb here. What about "stored"? "retained"?

> + */
>  int image_encoders_compress_glz(ImageEncoders *enc,
>                                  SpiceImage *dest, SpiceBitmap *src,
>                                  RedDrawable *red_drawable,

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