[FFmpeg-devel] [PATCH] move av_parse_color() to libavcore

[Aurelien Jacobs]

On Wed, Nov 17, 2010 at 01:16:35AM +0100, Michael Niedermayer wrote:
> On Mon, Nov 15, 2010 at 02:29:40PM +0100, Aurelien Jacobs wrote:
> > On Mon, Nov 15, 2010 at 02:17:50PM +0100, Michael Niedermayer wrote:
> > > On Sun, Nov 14, 2010 at 11:22:36PM +0100, Aurelien Jacobs wrote:
> > > > On Sat, Oct 23, 2010 at 08:28:06PM +0200, Michael Niedermayer wrote:
> > > > > On Sat, Oct 23, 2010 at 06:27:59PM +0200, Aurelien Jacobs wrote:
> > > > > > On Sat, Oct 23, 2010 at 06:25:59PM +0200, Aurelien Jacobs wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > I need to use the av_parse_color() function in the upcomming SubRip
> > > > > > > decoder (especially to decode those html named colors).
> > > > > > > So the attached patch move this function from libavfilter to libavcore.
> > > > > > > To simplify review, this patch don't contain removal of
> > > > > > > libavfilter/parseutil.* but I will do the svn rm while committing.
> > > > > > 
> > > > > > Hum... Here is the patch.
> > > > > > 
> > > > > > Aurel
> > > > > >  libavcore/parseutils.c   |  272 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  libavcore/parseutils.h   |   19 +++
> > > > > >  libavfilter/Makefile     |    1 
> > > > > >  libavfilter/vf_drawbox.c |    2 
> > > > > >  libavfilter/vf_frei0r.c  |    2 
> > > > > >  libavfilter/vf_pad.c     |    1 
> > > > > >  6 files changed, 293 insertions(+), 4 deletions(-)
> > > > > > 62f8c66e68b5cffb991d1ba90cbfad4fe2cd8f53  parse_color.diff
> > > > > > Index: libavcore/parseutils.h
> > > > > > ===================================================================
> > > > > > --- libavcore/parseutils.h	(revision 25528)
> > > > > > +++ libavcore/parseutils.h	(working copy)
> > > > > > @@ -50,4 +50,23 @@
> > > > > >   */
> > > > > >  int av_parse_video_rate(AVRational *rate, const char *str);
> > > > > >  
> > > > > > +/**
> > > > > > + * 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,
> > > > > > + * possibly followed by "@" and a string representing the alpha
> > > > > > + * component.
> > > > > > + * The alpha component may be a string composed by "0x" followed by an
> > > > > > + * hexadecimal number or a base-10 number between 0 and 255, or a
> > > > > > + * decimal number between 0.0 and 1.0, which represents the opacity
> > > > > > + * value (0/0x00/0.0 means completely transparent, 255/0xff/1.0
> > > > > > + * completely opaque).
> > > > > 
> > > > > I think the double meaning of 1 should be fixed 1==1.0
> > > > 
> > > > Now that this issue is fixed and that av_parse_color() is improved to
> > > > be more useful, here is an updated patch moving it to libavcore.
> > > 
> > > > Again, for readability, this patch don't contain the
> > > >   svn rm libavfilter/parseutils.*
> > > 
> > > how does this improve readability?
> > > with moved code there are 2 things i need to check
> > > 1. what is moved
> > > 2. what is changed in the moved code
> > > 
> > > For 2. a diff of the moved code would help but i can also just look at the
> > > moved code side by side to spot differences. If its not included in the
> > > diff though then thats another step of added work, and if it wasnt a complete
> > > rm then i couldnt even do it as i wouldnt know if the hunks are actually removed
> > > in the change if they are ommited from the patch
> > 
> > Well, I honestly don't think adding the svn rm here really help review,
> > but if you prefer having it, here it is.
> > 
> 
> > For 2. there nothing changed in the moved code except 2 trivial details:
> >  - libavutil/opt.h is not included anymore in parseutils.h (it is not
> >    needed anyway)
> >  - the "int i" declaration in the main() TEST function is moved inside
> >    of the block instead of outside
> > Except this, the code is just a straight copy/paste.
> 
> you know that, but i dont and its my job to check what is changed instead of
> assuming nothing of importance has been changed ...
> 
> 
> > 
> > Aurel
> >  doc/APIchanges            |    3 
> >  libavcore/avcore.h        |    4 
> >  libavcore/parseutils.c    |  285 ++++++++++++++++++++++++++++++++++++++++
> >  libavcore/parseutils.h    |   22 +++
> >  libavfilter/Makefile      |    1 
> >  libavfilter/graphparser.c |    1 
> >  libavfilter/parseutils.c  |  323 ----------------------------------------------
> >  libavfilter/parseutils.h  |   52 -------
> >  libavfilter/vf_drawbox.c  |    2 
> >  libavfilter/vf_frei0r.c   |    2 
> >  libavfilter/vf_pad.c      |    1 
> >  11 files changed, 314 insertions(+), 382 deletions(-)
> > aa6426110566e90ac90557391db392445f5e76e0  move_parse_color.diff
> 
> lgtm

Applied.

Aurel