[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 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"

I know you don't care about language standards.  However, *compilers*
care, and it's them that ultimately decide what we can and cannot do.

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

No standard specifies that those symbols should be defined.  Therefor
they are non-standard.  They work with gcc and gcc-emulation compilers
only.

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

I'm not disputing that.

> The code is used (mplayer is one example)
> The code works very well (mplayer is the proof)

MPlayer sets up all the necessary definitions for it to work.  Others
are of course free to include their own ffmpeg copy just like MPlayer
does.

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

No, we cannot.  Are you suggesting we install the configure script and
rerun it every time the header is used?  (Not that _that_ is possible
either.)

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

Its fault, not mine.

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

You're the one putting your ideas of what you'd like above actual reality.

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