[FFmpeg-devel] [PATCH] Fix inclusion in pixdesc.h of the non-public header intreadwrite.h

[Michael Niedermayer]

On Mon, Feb 15, 2010 at 10:47:15PM +0000, M?ns Rullg?rd wrote:
> Michael Niedermayer <michaelni at gmx.at> writes:
> 
> > On Mon, Feb 15, 2010 at 07:24:57PM +0000, M?ns Rullg?rd wrote:
> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >> 
> >> > On Mon, Feb 15, 2010 at 06:37:19PM +0000, M?ns Rullg?rd wrote:
> >> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >> >> 
> >> >> > On Mon, Feb 15, 2010 at 01:00:42PM +0000, M?ns Rullg?rd wrote:
> >> >> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >> >> >> 
> >> >> >> > On Sun, Feb 14, 2010 at 11:39:35PM +0100, Stefano Sabatini wrote:
> >> >> >> >> Hi all,
> >> >> >> >> 
> >> >> >> >> intreadwrite.h is not public so we should not include it in a public
> >> >> >> >> header, this also fixes a bunch of warnings during compilation.
> >> >> >> >> 
> >> >> >> >> Also read_line() and write_line() are just meant for
> >> >> >> >> testing/debugging/pedagogical purposes, so the fact that they're not
> >> >> >> >> defined inline shouldn't be relevant.
> >> >> >> >
> >> >> >> > no, read/write_line() are an essential and important part of
> >> >> >> > pixdescs They where intended as fallback for very rarely used
> >> >> >> > convertions in swscale having a specific optimized converter
> >> >> >> > for each pixel format is overkill especially on CONFIG_SMALL
> >> >> >> > targets
> >> >> >> 
> >> >> >> Having them inline in the public header doesn't exactly fit with that
> >> >> >> description, so I think the patch is fine.  No functionality is being
> >> >> >> removed.
> >> >> >
> >> >> > well, if its important to you then iam ok with moving them to the c file
> >> >> >
> >> >> > but i still do want intreadwrite public ...
> >> >> 
> >> >> That's a different debate.
> >> >> 
> >> >> > it is usefull and used
> >> >> 
> >> >> That's impossible.
> >> >
> >> > in mplayer trunk
> >> > grep 'AV_R[BL][0-9]' *.{c,h} libmp*/*.{c,h} |wc
> >> >     108     564    7751
> >> 
> >> For the billionth time: intreadwrite.h is IMPOSSIBLE TO WRITE
> >> PORTABLY.  MPlayer abuse is totally irrelevant.
> >
> > It is possible to implement with plain C and this is portable
> > within normal platforms (iam ignoring the char is 24bits systems)
> > we arent supporting these anyway.
> 
> How big a cluebat do I need to use to pound it into your head that
> this code *REQUIRES* inline asm etc to be fast.  IT CANNOT BE DONE IN
> PLAIN C.

But if it fails to detect the compiler it falls back to plain C,
it does so now as well.


> 
> > About compiler specific code like unaligned reads and all that,
> > there is #ifdef __GNUC__ the rest can be handled by plain C
> >
> > about asm there is
> > ifdef __i386__ inside #ifdef __GNUC__
> 
> Are you out of your mind?  Have you looked at the conditions used
> there?  None of the tests you just mentioned are even remotely
> standard.

i dont care at all if they are "standard" the only question is if they
work and id say they should. Besides i see nothing non standard on them
ive seen checks based on them many years ago in code ...


> 
> The intreadwrite.h code is extremely sensitive to compiler specifics
> due to the way it abuses the C standard.  It has unaligned accesses,
> aliasing violations, etc.  It is impossible to predict what an unknown
> compiler might do with it.  

unknown compilers would fall back the the "slow" plain C code, this is the
best that can be done for them either way.


> You'll have to give up on this one.

I see no reason why i should, nor do i see why my giving up on this
would make any difference at all
The code is usefull
The code is used (mplayer is one example)
The code works very well (mplayer is the proof)
We can make it work even better by checking the used compiler and cpu
for the cases that the user compiles lib & app with different compilers
or exchanged his cpu in the meantime.
Nothing you can do will prevent mplayer and others from using this code
I have in the past said that the header should be properly installed,
its a matter of time until distros will apply patches for this as more
packages start depending on it. (mplayer needs it already)
Our choice is just between trying to make it work for most (that are
99.9999%) of the cases or trying to be one of the FOSS philosophers who
put their ideas of what is right and wrong above actual useability and
their users.

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

Democracy is the form of government in which you can choose your dictator
-------------- 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/20100216/a3d5f824/attachment.pgp>