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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101117/9b354d58/attachment.pgp>