[FFmpeg-devel] [PATCH 1/4] Remove unnecessary mem.h inclusions

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2021-02-04 12:06:15)
>> Anton Khirnov:
>>> Quoting Andreas Rheinhardt (2021-02-04 01:05:05)
>>>> Thanks for the report. I have only checked for whether the relevant
>>>> translation unit uses any of the alloc/free functions (because
>>>> DECLARE_ALIGNED is already provided by mem_internal.h), but I have not
>>>> taken into account stuff that is included by the headers included by
>>>> mem.h. In this case, one needs string.h (which is included in
>>>> libavutil/common.h which in turn is included in avutil.h which is
>>>> included in mem.h). Will look over everything again.
>>>>
>>>> - Andreas
>>>>
>>>> PS: It actually seems that the only thing provided by avutil.h that
>>>> mem.h uses is size_t. Do we allow to remove included headers from
>>>> installed headers at major version bumps?
>>>
>>> I would say yes, IIRC it has happened before.
>>> Certainly nobody should depend on our headers to provide any random
>>> system headers.
>>>
>> Removing avutil.h affects not only system headers, but which of our
>> headers are included as it includes
>> #include "common.h"
>> #include "error.h"
>> #include "rational.h"
>> #include "version.h"
>> #include "macros.h"
>> #include "mathematics.h"
>> #include "log.h"
>> #include "pixfmt.h"
>> (And removing avutil.h will also remove avutil.h, of course.)
>>
>> Other installed headers include too much, too: audio_fifo.h includes
>> avutil.h and fifo.h, but needs only attributes.h; eval.h unnecessarily
>> includes avutil.h; display.h includes common.h without needing it.
>> fifo.h includes avutil.h and attributes.h, but needs only stddef.h.
>> Several other headers include version.h despite the relevant FF_API
>> check for which it has been included no longer existing. (If we don't
>> remove version.h, sooner or later all headers will contain it.)
> 
> I don't think we have an official project policy on this, but I would
> be in favor of something like:
> 
>     All installed headers are only guaranteed to provide those
>     identifiers that are explicitly declared in them. Users must not
>     rely on an installed header #include'ing any other specific headers,
>     as those can change at any time.
> 
> +exceptions for av*.h, but ideally I'd remove those too in the long
> run - people should just include what they need and avoid monsterheaders
> 
>>
>> And while I played around a bit with this night, I found something odd:
>> libavutil/error.h is broken: It relies on MKTAG without providing the
>> header for it. MKTAG is in common.h which also includes mem.h which uses
>> an AVERROR code in an inline function. So the simple solution of
>> including common.h at the beginning of error.h doesn't work, as
>> AVERROR(EINVAL) will still be undefined in said function. I see two
>> other solutions: Move MKTAG (and probably a bit other stuff) to a new
>> header and include that in error.h; include common.h, but not at the
>> beginning of error.h, but only after the definition of AVERROR. Given
>> that I dislike monster headers like common.h, I prefer the first approach.
>> (I already mentioned this on IRC, but those who only follow the ML
>> should be informed, too.)
> 
> I am in favor of moving MKTAG if you can think of a good place for it.
> (also it should be properly namespaced, but not necessarily now)
> 
macros.h seems like a good place for it; in fact, it seems like a better
place for all the macros from common.h. common.h would then be reduced
to misc inline math functions and inclusions of other headers.
common.h already includes macros.h, so this would not be disruptive.
Furthermore, the hardcoded tables code currently has to define
FF_ARRAY_ELEMS itself, because common.h includes config.h (when building
the libraries itself), and by moving this stuff to macros.h one can
simply use macros.h.
If no one objects, I'll prepare patches for this.

- Andreas