[FFmpeg-devel] av_parse_color() improvement

[Aurelien Jacobs]

On Sat, Nov 13, 2010 at 12:30:23AM +0100, Michael Niedermayer wrote:
> On Sat, Nov 13, 2010 at 12:22:32AM +0100, Aurelien Jacobs wrote:
> > On Sat, Nov 13, 2010 at 12:13:15AM +0100, Michael Niedermayer wrote:
> > > On Fri, Nov 12, 2010 at 11:53:10PM +0100, Aurelien Jacobs wrote:
> > > > On Fri, Nov 12, 2010 at 07:38:40PM +0100, Michael Niedermayer wrote:
> > > > > On Thu, Nov 11, 2010 at 02:29:27AM +0100, Aurelien Jacobs wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Attached patch makes the av_parse_color() a bit more versatile.
> > > > > > It allows parsing of html formated hex color (ie. starting with a #
> > > > > > instead of 0x), including its loose form where the # is missing.
> > > > > > This makes quite some sens as the function is already able to parse
> > > > > > html named colors.
> > > > > > This is required to be able easily use this function to parse colors
> > > > > > out of SubRip files.
> > > > > > 
> > > > > > Aurel
> > > > > >  parseutils.c |   22 +++++++++++++++++-----
> > > > > >  parseutils.h |    2 +-
> > > > > >  2 files changed, 18 insertions(+), 6 deletions(-)
> > > > > > 4ae48624cebdcb2a693371dd00f51f6cff086711  parse_color.diff
> > > > > > Index: libavfilter/parseutils.h
> > > > > > ===================================================================
> > > > > > --- libavfilter/parseutils.h	(revision 25719)
> > > > > > +++ libavfilter/parseutils.h	(working copy)
> > > > > > @@ -31,7 +31,7 @@
> > > > > >   * Put the RGBA values that correspond to color_string in rgba_color.
> > > > > >   *
> > > > > >   * @param color_string a string specifying a color. It can be the name of
> > > > > > - * a color (case insensitive match) or a 0xRRGGBB[AA] sequence,
> > > > > > + * a color (case insensitive match) or a [0x|#]RRGGBB[AA] sequence,
> > > > > >   * possibly followed by "@" and a string representing the alpha
> > > > > >   * component.
> > > > > >   * The alpha component may be a string composed by "0x" followed by an
> > > > > > Index: libavfilter/parseutils.c
> > > > > > ===================================================================
> > > > > > --- libavfilter/parseutils.c	(revision 25719)
> > > > > > +++ libavfilter/parseutils.c	(working copy)
> > > > > > @@ -187,9 +187,17 @@
> > > > > >  {
> > > > > >      char *tail, color_string2[128];
> > > > > >      const ColorEntry *entry;
> > > > > > -    av_strlcpy(color_string2, color_string, sizeof(color_string2));
> > > > > > +    int len, hex_offset = 0;
> > > > > > +
> > > > > > +    if (color_string[0] == '#')
> > > > > > +        hex_offset = 1;
> > > > > > +    else if (!strncmp(color_string, "0x", 2))
> > > > > > +        hex_offset = 2;
> > > > > > +
> > > > > > +    av_strlcpy(color_string2, color_string + hex_offset, sizeof(color_string2));
> > > > > >      if ((tail = strchr(color_string2, ALPHA_SEP)))
> > > > > >          *tail++ = 0;
> > > > > > +    len = strlen(color_string2);
> > > > > >      rgba_color[3] = 255;
> > > > > >  
> > > > > >      if (!strcasecmp(color_string2, "random") || !strcasecmp(color_string2, "bikeshed")) {
> > > > > > @@ -198,16 +206,16 @@
> > > > > >          rgba_color[1] = rgba >> 16;
> > > > > >          rgba_color[2] = rgba >> 8;
> > > > > >          rgba_color[3] = rgba;
> > > > > > -    } else if (!strncmp(color_string2, "0x", 2)) {
> > > > > > +    } else if (hex_offset ||
> > > > > 
> > > > > > +               strspn(color_string2, "0123456789ABCDEFabcdef") == len) {
> > > > > 
> > > > > why is that needed?
> > > > > i thought things failing this would fail some other tests too
> > > > 
> > > > yes and no...
> > > > The code has 3 branches looking like this:
> > > >     if("random") {
> > > >         color = random();
> > > >     } else if(has_hex_prefix()) {
> > > >         color = parse_as_hex_number();
> > > >     } else {
> > > >         color = parse_as_color_name();
> > > >     }
> > > > 
> > > > Right now, the has_hex_prefix() branch will only be entered if the
> > > > string start with 0x.
> > > > I need this function to also be able to parse hex numbers without a
> > > > prefix. Right now, such a string fall back into the
> > > > parse_as_color_name() case and just fail.
> > > > 
> > > > I see to solution to solve this:
> > > > 1) Always try to parse as a color name first, and if it fail, try to
> > > > parse as a hex number. But this would slow down parsing hex numbers by
> > > > first trying to match them with the entries of the color name table.
> > > 
> > > have you checked that is slower than strspn() ?
> > 
> > No, I didn't benchmark this.
> > And I don't think it is really worth to benchmark it.
> > In actual use case, this function should never account for a significant
> > part of decode time.
> > 
> > > also you could try to parse as hex first and if it fails fallback to the
> > > names
> > 
> > This could be possible but it wouldn't be easy to provide as specific
> > error messages in case of parsing failure.
> > 
> > > its also not a real issue if you leave the strspn() there if you prefer
> > 
> > I will give myself a day or two to think about it, but I may indeed
> > prefer to keep the strspn().
> 
> if so the patch otherwise looks fine expcet a {} nitpick between if/else

Applied with this nitpick.

Aurel