Re: r14484 - in trunk/libavcodec: audioconvert.c audioconvert.h

On Thu, Jul 31, 2008 at 05:00:40PM -0400, The Wanderer wrote:
> Michael Niedermayer wrote:
> 
> > On Thu, Jul 31, 2008 at 07:47:05PM +0200, Michael Niedermayer wrote:
> > 
> >> On Thu, Jul 31, 2008 at 01:11:14PM -0400, compn wrote:
> 
> >>> is this policy documented somewhere?
> >> 
> >> it has not even been discussed or agreed upon yet ...
> >> 
> >> Besides maybe we can make the converter independant of avcodec.h
> >> Which seems better either way ...
> > 
> > and just to repeat my view on the headers inclusion IMHO headers
> > should NOT include other headers, this nicely avoids such problems
> > ...
> 
> It also leads to the case where including one header leads to needing to
> include another header, which might in turn need another header, et

which is not available and compilation fails.
The user now who just wants to use the part has to hack the headers instead
of adding a 5 line enum before the header that needs it.
Or maybe the functionality is available but in a different header ...

Or the application breaks randomly until the user realized that audioconvert.h
included acodec.h that included common.h that included assert.h and one of
the headers defined NDEBUG thus disabling asserts. and rendering the
user own #include <assert.h> worthless (this one actually happened to me

Re: r14484 - in trunk/libavcodec: audioconvert.c audioconvert.h

On Fri, 2008-08-01 at 03:27 +0200, Michael Niedermayer wrote:
> On Thu, Jul 31, 2008 at 05:00:40PM -0400, The Wanderer wrote:
> > It also leads to the case where including one header leads to needing to
> > include another header, which might in turn need another header, et
> 
> which is not available and compilation fails.

So you think trial and error is a good way to set the dependencies? In
such an inclusion system it is pretty much the only way. And it doesn't
work if some indirect dependencies are conditional or change later.

> Or the application breaks randomly until the user realized that audioconvert.h
> included acodec.h that included common.h that included assert.h and one of
> the headers defined NDEBUG thus disabling asserts. and rendering the
> user own #include <assert.h> worthless (this one actually happened to me
> once with assert and some headers i dont remember ...)

I think defining NDEBUG in a normal header is a bug. That doesn't say
much about the inclusion style in general.

> Some more concrete examples
> avcodec.h uses int64_t, if it would #include inttypes.h it would be between
> unuseable to a PITA in any environment that does not have inttypes.h.

It does already include inttypes.h indirectly through libavutil headers.

> libavcodec may be compiled with gcc + glibc in a gnu environment. but
> it might be used in VC++ or another obscure environment that lacks these
> headers.

r14494 - trunk/libavcodec/libmp3lame.c

Author: benoit
Date: Fri Aug  1 09:23:29 2008
New Revision: 14494

Log:
Use compression level to set mp3lame quality option.
Patch by Nicolas George nicolas george normalesup org
Original thread:
[PATCH] libmp3lame: set noise shaping & psychoacoustic algorithms quality
Date: 07/31/2008 03:53 PM

Modified:
   trunk/libavcodec/libmp3lame.c

Modified: trunk/libavcodec/libmp3lame.c
==============================================================================
--- trunk/libavcodec/libmp3lame.c	(original)
+++ trunk/libavcodec/libmp3lame.c	Fri Aug  1 09:23:29 2008
 <at>  <at>  -50,8 +50,11  <at>  <at>  static av_cold int MP3lame_encode_init(A
     lame_set_in_samplerate(s->gfp, avctx->sample_rate);
     lame_set_out_samplerate(s->gfp, avctx->sample_rate);
     lame_set_num_channels(s->gfp, avctx->channels);
-    /* lame 3.91 dies on quality != 5 */
-    lame_set_quality(s->gfp, 5);
+    if(avctx->compression_level == FF_COMPRESSION_DEFAULT) {
+        lame_set_quality(s->gfp, 5);
+    } else {
+        lame_set_quality(s->gfp, avctx->compression_level);
+    }
     /* lame 3.91 doesn't work in mono */

Re: r14484 - in trunk/libavcodec: audioconvert.c audioconvert.h

Michael Niedermayer <michaelni <at> gmx.at> writes:

> The only reason why its not causing a problem is plain because its not
> done, our headers luckily do in general not include all the headers they
> somehow depend on.

Actually, *all* our headers #include their dependencies.  That's what
"make checkheaders" makes sure.

Mike, could you add "make checkheaders" to the FATE tests, so we
notice any breakage immediately?

--

-- 
Måns Rullgård
mans <at> mansr.com

Re: r14484 - in trunk/libavcodec: audioconvert.c audioconvert.h

Michael Niedermayer wrote:

> On Thu, Jul 31, 2008 at 05:00:40PM -0400, The Wanderer wrote:
> 
>> Michael Niedermayer wrote:

>>> and just to repeat my view on the headers inclusion IMHO headers
>>> should NOT include other headers, this nicely avoids such
>>> problems
>> 
>> It also leads to the case where including one header leads to
>> needing to include another header, which might in turn need another
>> header, et
> 
> which is not available and compilation fails.

But if that other header is not available, compilation would fail
anyway, because the original header needs something which is not
defined.

> The user now who just wants to use the part has to hack the headers
> instead of adding a 5 line enum before the header that needs it.

And how is the user to know what to add? In the case of the enum it's
mildly obvious, but exactly how that enum is defined is not obvious, and
finding out how it is defined would require tracking down the header
which defines it - which leads to the same problem as before.

> Or maybe the functionality is available but in a different header ...

r14495 - trunk/libavcodec/audioconvert.c

Author: pross
Date: Fri Aug  1 13:26:22 2008
New Revision: 14495

Log:
Revert r14484 hunk that deleted the 'include avcodec.h' statement.

Modified:
   trunk/libavcodec/audioconvert.c

Modified: trunk/libavcodec/audioconvert.c
==============================================================================
--- trunk/libavcodec/audioconvert.c	(original)
+++ trunk/libavcodec/audioconvert.c	Fri Aug  1 13:26:22 2008
 <at>  <at>  -25,6 +25,7  <at>  <at> 
  *  <at> author Michael Niedermayer <michaelni <at> gmx.at>
  */

+#include "avcodec.h"
 #include "audioconvert.h"

 typedef struct SampleFmtInfo {

Re: r14484 - in trunk/libavcodec: audioconvert.c audioconvert.h

On Fri, Aug 01, 2008 at 05:20:33AM +0300, Uoti Urpala wrote:
> On Fri, 2008-08-01 at 03:27 +0200, Michael Niedermayer wrote:
> > On Thu, Jul 31, 2008 at 05:00:40PM -0400, The Wanderer wrote:
[...]
> > Some more concrete examples
> > avcodec.h uses int64_t, if it would #include inttypes.h it would be between
> > unuseable to a PITA in any environment that does not have inttypes.h.
> 
> It does already include inttypes.h indirectly through libavutil headers.

interresting, how much time did i took you to find that out? How much do you
think it might cause a average developer who debugs a problem related to it?

> 
> > libavcodec may be compiled with gcc + glibc in a gnu environment. but
> > it might be used in VC++ or another obscure environment that lacks these
> > headers.
> 
> If you create such workaround definitions it's not much more work to
> place them in a header named "inttypes.h".

as long as there is no inttypes.h already that maybe just misses half of the
things.

> 
> > The rule of including headers in headers is a absolute pain for some use
> > cases to deal with. The advantage of this is purely "because its the right
> > way".
> 
> There was a thread about this on mplayer-dev-eng not too long ago that

Re: r14484 - in trunk/libavcodec: audioconvert.c audioconvert.h

On Fri, Aug 01, 2008 at 09:39:16AM +0100, Måns Rullgård wrote:
> Michael Niedermayer <michaelni <at> gmx.at> writes:
> 
> > The only reason why its not causing a problem is plain because its not
> > done, our headers luckily do in general not include all the headers they
> > somehow depend on.
> 
> Actually, *all* our headers #include their dependencies.  That's what
> "make checkheaders" makes sure.

avcodec.h does not include inttypes or stdint directly but use int64_t
and iam sure it would be easier to find headers missing dependencies
than ones listing all.

[...]
--

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway

_______________________________________________
ffmpeg-cvslog mailing list
ffmpeg-cvslog <at> mplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-cvslog

Re: r14484 - in trunk/libavcodec: audioconvert.c audioconvert.h

Michael Niedermayer wrote:
> On Fri, Aug 01, 2008 at 09:39:16AM +0100, Måns Rullgård wrote:
>> Michael Niedermayer <michaelni <at> gmx.at> writes:
>>
>> > The only reason why its not causing a problem is plain because its not
>> > done, our headers luckily do in general not include all the headers they
>> > somehow depend on.
>>
>> Actually, *all* our headers #include their dependencies.  That's what
>> "make checkheaders" makes sure.
>
> avcodec.h does not include inttypes or stdint directly but use int64_t
> and iam sure it would be easier to find headers missing dependencies
> than ones listing all.

avcodec.h includes common.h, which does include stdint.h.  As I said,
common.h is a special case.  Its purpose is to avoid duplicating
code that would have to be in practically every file otherwise, and
this includes including stdint.h.

Now please stop trolling, and get back to coding, which you do very well.
Quite frankly though, I doubt you have the experience necessary to for
an informed opinion on organisational matters like these.  That a certain
method can, if enough force is applied, achieve a working end result,
does not in any way whatsoever make it a good method.

--

-- 
Måns Rullgård
mans <at> mansr.com

Re: r14484 - in trunk/libavcodec: audioconvert.c audioconvert.h

On Fri, Aug 01, 2008 at 06:58:42AM -0400, The Wanderer wrote:
> Michael Niedermayer wrote:
> 
> > On Thu, Jul 31, 2008 at 05:00:40PM -0400, The Wanderer wrote:
> > 
> >> Michael Niedermayer wrote:
[...]
> > The advantage of this is purely "because its the right way".
> 
> And just why is it that it is the right way?

You said "If a header needs something then it should provide that thing,
either by defining it directly or (more practically) by including
another header which already defines it. A header which fails to do so
is, IMO, broken."

I felt that this was an argument based on "because it should be done
that way"

[...]
> >> cetera. If a header needs something then it should provide that
> >> thing, either by defining it directly or (more practically) by
> >> including another header which already defines it. A header which
> >> fails to do so is, IMO, broken.
> > 
> > Do you have a argument beyond "is broken" vs. "is the right way"?
> 
> Perhaps a few, but Uoti has made a few which seem sensible to me
> (although also at least one which does not). The fact that Uoti appears
> to agree with me is enough to lead me to doubt my own position, but I'm