[FFmpeg-devel] [PATCH] lavu: add av_get_color_string() and use it in -colors option

[Nicolas George]

Le sextidi 26 vendémiaire, an CCXXII, Stefano Sabatini a écrit :
>  /**
> + * Get color name with index color_idx, as recognized by the
> + * av_parse_color() function.

I suggest to try to make it clearer that this functions is intended to
enumerate the hardcoded color names known to the library, as opposed to, for
example, convert a color into a user-readable string.

> + *
> + * @param bufp      pointer to the address where the color name string address is written

What is the point of both returning it through the return value and a
pointer?

> + * @param rgb       pointer to the address where the 3-values array containing the RGB
> + *                  components is written, may be NULL

Inconsistent naming of the argument here and in the prototype, compared to
the function definition "rgbp".

And the description is really not clear.

> + * @param color_idx the number of the color entries, starting from 0
> + * @return the color name string or NULL if color_idx is not in the array
> + */
> +const char *av_get_color_name(const char **bufp, const uint8_t **rgb, int color_idx);
> +
> +/**
>   * Parse timestr and return in *time a corresponding number of
>   * microseconds.
>   *

May I suggest:

av_get_known_color_name(unsigned id, const uint8_t **rgbp);

Get the name of a color from the internal table of hard-coded named colors.

[in]  id:   index of the requested color, starting from 0
[out] rgbp: if not NULL, will point to a 3-elements array with the color
            value in RGB

OTOH, since it only concerns hard-coded colors, I believe it very unlikely
more than 8-bits will be required. But if you want more, than I suggest to
go directly to 32-bits.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131021/cfe92bcd/attachment.asc>