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

[Måns Rullgård]

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.

> 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.

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.  You'll have to give up on this one.

-- 
M?ns Rullg?rd
mans at mansr.com