[FFmpeg-devel] [PATCH] fix build with --disable-encoders

Michael Niedermayer michaelni
Sun Feb 24 15:33:48 CET 2008


On Sun, Feb 24, 2008 at 02:44:09PM +0100, Diego Biurrun wrote:
> On Sat, Feb 23, 2008 at 09:44:05PM +0100, Michael Niedermayer wrote:
> > On Sat, Feb 23, 2008 at 06:32:28PM +0000, M?ns Rullg?rd wrote:
> > [...]
> > > I really don't understand why Michael doesn't like placing stuff
> > > inside #ifdef CONFIG_ENCODERS.
> > 
> > Because its extra maintaince work and leads to hard to find bugs.
> > Just look in this thread,
> > it was about some code which ended up under that #ifdef which then
> > broke compilation with --disable-encoders. The first proposed fix (which
> > needed reviewing time) would have fixed it by disabling some png
> > decoding optimizations. If i would have missed that (possible, iam just
> > a human ...) --disable-encoders would have caused the decoders to be
> > mysteriously slower.
> 
> I'm with Mans on this one, one more #ifdef in dsputil_mmx.c will not
> hurt, but the warning about unused code is useful.

Unused code doesnt cause hard to debug bugs, these ifdefs do. Furthermore
they are wrong, the code is not needed for encoding in general but just
for some specific encoders.
With the ifdefs in place i would have to check in every submitted patch that
each hunk is under the proper ifdefs (which obviously isnt clear from the
patch itself but requires reading through the source). Also as this thread
demonstrated the ifdefs cause bugs and correcting these bugs isnt trivial,
the first suggestion would have introduced more bugs.
Iam the one who has to do the extra work, neither you nor mans have
volunteered. Thus i think until either of you volunteers to do the extra
work i think you should not demand that i do.

Also if the ifdefs stay they should at minimum each enclose exactly 1
function, not cover random subsets of a file.


[...]


> 
> It might be good to split off encoding-related stuff from that file but
> this question is orthogonal to adding one more #ifdef.

No, its not orthogonal, ifdef CONFIG_ENCODERS is specific to the case of mixed
encoder and decoder code, if its split the encoder ifdefs become unneeded.


> 
> > Without #ifdefs none of these problems would have occured. The warnings
> > can as well be suppressed by attribute unused or -wno-whatever.
> 
> That would have to be a lot of attributes..

We also have a lot of static and void infront of functions. So far noone
complained about it though, and iam pretty sure you could reduce these with
some preprocessor tricks as well ;)

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Thouse who are best at talking, realize last or never when they are wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080224/52ef80ee/attachment.pgp>



More information about the ffmpeg-devel mailing list