[FFmpeg-devel] [PATCH 1/3] Implement parse_preset_line() and use it in ffmpeg.c and ffserver.c.

[Stefano Sabatini]

On date Tuesday 2010-11-09 02:49:18 +0100, Michael Niedermayer encoded:
> On Mon, Nov 08, 2010 at 01:18:15PM +0100, Stefano Sabatini wrote:
> > On date Sunday 2010-11-07 16:14:21 +0100, Michael Niedermayer encoded:
> > > On Sun, Nov 07, 2010 at 10:48:38AM +0100, Stefano Sabatini wrote:
> > > > On date Sunday 2010-11-07 01:06:36 +0100, Michael Niedermayer encoded:
> > > > > On Sat, Nov 06, 2010 at 03:02:39PM +0100, Stefano Sabatini wrote:
> > > > > > ---
> > > > > >  cmdutils.c |   24 ++++++++++++++++++++++++
> > > > > >  cmdutils.h |   15 +++++++++++++++
> > > > > >  ffmpeg.c   |   14 ++++----------
> > > > > >  ffserver.c |   12 ++----------
> > > > > 
> > > > > >  4 files changed, 45 insertions(+), 20 deletions(-)
> > > > > 
> > > > > if this is code factorization its making the code >2x larger instead of smaller
> > > > 
> > > > It's because of the docs and the function header overhead, also sscanf
> > > 
> > > that makes 15 lines, the patch is still worsening it by 50%
> > > 
> > > 
> > > > doesn't allow to write in a string with given variable size, so I need
> > > > to copy the string back to the user provided buffers.
> > > > 
> > > > But the point of the patch is not to reduce the line count, but
> > > > avoiding code duplication (see also patch #2).
> > > 
> > > I understand that, it still doesnt look all that much like a improvment to me
> > 
> > bug_count/2, also I like to bury implementation details away from the
> > application, that's why I like to move code from ff* tools to the lib
> > or to the cmdutils.c file, as this reduces the exposed complexity when
> > reading the application code.
> > 
> > When this is (maybe) increasing the overall complexity, it is still
> > reducing the exposed complexity in the application code.
> 
> one hides a trivial thing behind a supposedly more trivial API
> i dont think adding layers on layers is a good idea in this case
> especially as you put a custom layer over 2 ANSI C sscanf(), later every
> programmer will understand but noone will knoe what your custom function
> does without finding and opening the file where that function resides

So patch dropped, but it would still make sense if the complexity of
the parsing code increases.
-- 
FFmpeg = Fancy Fantastic Multipurpose Portable Enlightened Guru