[MPlayer-dev-eng] [PATCH] factorize print_version()

[Diego Biurrun]

On Tue, Jan 13, 2009 at 12:37:06AM +0200, Uoti Urpala wrote:
> On Mon, 2009-01-12 at 13:02 +0100, Diego Biurrun wrote:
> > On Mon, Jan 12, 2009 at 03:45:26AM +0200, Uoti Urpala wrote:
> > > On Mon, 2009-01-12 at 02:01 +0100, Diego Biurrun wrote:
> > > > Here is a patch to factorize the version string etc. output in mplayer.c
> > > > and mencoder.c.
> > > 
> > > Do you have any motivation for this beyond just decreasing code
> > > duplication?
> > 
> > I stumbled across it while looking at the copyright year update patches
> > and the bugzilla entry 1378 about some information about CPU options not
> > being printed.  Yes, the motivation is to decrease the code duplication
> > and to avoid bugs like 1378 in the future.
> > 
> > > I don't really like it - I think MPlayer code quality is
> > > the most important thing, and from that viewpoint this way of splitting
> > > code between mplayer.c and mpcommon.c is IMO quite arbitrary and does
> > > not give a good architecture. If MEncoder is developed further then that
> > > would be best done by deleting most of its independent code and moving
> > > the encoding functionality to work with MPlayer's frame generation. If
> > > that is done it doesn't matter much what extra crap mencoder.c had.
> > 
> > I don't think MEncoder should be left to rot even more.  The way of code
> > splitting can be changed at any point easily.  The code duplication
> > causes issues now.
> 
> The code can be moved back later reasonably easily, but is it really
> beneficial even in the short term? MEncoder is quite rotten already. As
> long as MEncoder is not really maintained MPlayer quality is what
> matters, and IMO it's worsened by changes like this.

I think the change is worth the trouble.

It will make my life easier when I fix bug 1378 and it will make my
life easier when I update the copyright notice next year.  So I think
it's beneficial in the short term and not a big deal in the long run.

If you think print_version() should be put in a different place, I'm
all ears.  But basically anything that makes mplayer.c smaller cannot
hurt IMO.

> MEncoder has lots of duplicated functionality, much of it an inferior
> version of the MPlayer code. That's exactly why encoding should be
> implemented on top of MPlayer's frame generation if MEncoder is to be
> developed further. Splitting random pieces of MPlayer between "common"
> and "non-common" files and using those in some individual parts of
> MEncoder isn't really getting any closer to that goal.

Maybe, but at least MEncoder will get the bug fixes applied to the
common parts.

Diego