| Message ID | 20200421023153.25008-6-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/8] avformat/utils: Remove superfluous headers | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Quoting Andreas Rheinhardt (2020-04-21 04:31:51) > Since 33d18982fa03feb061c8f744a4f0a9175c1f63ab (the commit that > introduced the new bsf API) allocating an enlarged buffer in case > extradata needs to be added to a packet is done via av_new_packet(), > so that libavutil/mem.h is no longer needed. > > Furthermore, remove libavutil/log.h. This function uses something > provided by it (an AVClass), yet it does so only for AVOptions, not > for logging purposes (there is no av_log() here), so only including > libavutil/opt.h seems appropriate. IMO each file should include the headers for all the definitions it uses directly. I.e. if a file uses AVClass, it should include log.h. But I suppose it does not matter much in pracice (in this case especially), so feel free to ignore me.
Anton Khirnov: > Quoting Andreas Rheinhardt (2020-04-21 04:31:51) >> Since 33d18982fa03feb061c8f744a4f0a9175c1f63ab (the commit that >> introduced the new bsf API) allocating an enlarged buffer in case >> extradata needs to be added to a packet is done via av_new_packet(), >> so that libavutil/mem.h is no longer needed. >> >> Furthermore, remove libavutil/log.h. This function uses something >> provided by it (an AVClass), yet it does so only for AVOptions, not >> for logging purposes (there is no av_log() here), so only including >> libavutil/opt.h seems appropriate. > > IMO each file should include the headers for all the definitions it uses > directly. I.e. if a file uses AVClass, it should include log.h. > My test for inclusion is more like "What API does this file use and which header provides it?" In this case: This bsf only uses the AVOption API, not the logging API. That the former is built on top of a structure that is also used by the latter (and that happens to be provided by the header for the latter) does not imply (for me) that one should include the header for the latter. Another example where our opinions will likely differ is the following: The muxing/demuxing APIs use AVPackets a lot and for this reason avformat.h includes enough so that AVPackets are directly usable for everyone including avformat.h. That's fine by me (although I'd really like to see the amount of stuff unnecessarily included (it currently includes avcodec.h) reduced), but not for you I presume. Your approach would instead include packet.h (and stdint.h) almost everywhere. Furthermore, what counts as "use" for you (and others, of course)? Right now we have a lot of dependencies among headers and lots of files include lots of unnecessary stuff (which also means that it is easy to forget to add a header when making use of it for the first time because it might already be included indirectly), because our headers include other headers to avoid forward declarations even when all they use from another header are actually incomplete types (we only use forward declarations when it is unavoidable because of cyclic dependencies or in order to hide a private struct). Should we try to cut the dependencies among headers and thereby force users to include what they use directly by using more incomplete types/forward declarations in our headers? > But I suppose it does not matter much in pracice (in this case > especially), so feel free to ignore me. > - Andreas PS: Examples where we probably agree on exist, too: I don't think that mem.h should be included basically everywhere via avcodec.h (via libavutil/avutil.h and libavutil/common.h -- there might be other paths); and cpu.h should be removed from avcodec.h, but that probably requires a public announcement and grace period first.
Quoting Andreas Rheinhardt (2020-04-21 17:28:08) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2020-04-21 04:31:51) > >> Since 33d18982fa03feb061c8f744a4f0a9175c1f63ab (the commit that > >> introduced the new bsf API) allocating an enlarged buffer in case > >> extradata needs to be added to a packet is done via av_new_packet(), > >> so that libavutil/mem.h is no longer needed. > >> > >> Furthermore, remove libavutil/log.h. This function uses something > >> provided by it (an AVClass), yet it does so only for AVOptions, not > >> for logging purposes (there is no av_log() here), so only including > >> libavutil/opt.h seems appropriate. > > > > IMO each file should include the headers for all the definitions it uses > > directly. I.e. if a file uses AVClass, it should include log.h. > > > My test for inclusion is more like "What API does this file use and > which header provides it?" In this case: This bsf only uses the AVOption > API, not the logging API. That the former is built on top of a structure > that is also used by the latter (and that happens to be provided by the > header for the latter) does not imply (for me) that one should include > the header for the latter. > > Another example where our opinions will likely differ is the following: > The muxing/demuxing APIs use AVPackets a lot and for this reason > avformat.h includes enough so that AVPackets are directly usable for > everyone including avformat.h. That's fine by me (although I'd really > like to see the amount of stuff unnecessarily included (it currently > includes avcodec.h) reduced), but not for you I presume. Your approach > would instead include packet.h (and stdint.h) almost everywhere. Yes, I would explicitly #include "packet.h" in every place that uses AVPacket. IMO that's preferable to including it indirectly through some other header, since that just leads to giant megaheaders like avcodec.h that are just a pain to deal with in so many ways (maybe you noticed that I'm taking some preliminary steps toward splitting it). > > Furthermore, what counts as "use" for you (and others, of course)? Right > now we have a lot of dependencies among headers and lots of files > include lots of unnecessary stuff (which also means that it is easy to > forget to add a header when making use of it for the first time because > it might already be included indirectly), because our headers include > other headers to avoid forward declarations even when all they use from > another header are actually incomplete types (we only use forward > declarations when it is unavoidable because of cyclic dependencies or in > order to hide a private struct). Should we try to cut the dependencies > among headers and thereby force users to include what they use directly > by using more incomplete types/forward declarations in our headers? I wouldn't go for incomplete types, these are bound to cause extra pain. I think a reasonable direction to move in is splitting avcodec.h and avformat.h into smaller pieces and then use those smaller headers instead of including the mega-monster. > > > But I suppose it does not matter much in pracice (in this case > > especially), so feel free to ignore me. > > > - Andreas > > PS: Examples where we probably agree on exist, too: I don't think that > mem.h should be included basically everywhere via avcodec.h (via > libavutil/avutil.h and libavutil/common.h -- there might be other > paths); and cpu.h should be removed from avcodec.h, but that probably > requires a public announcement and grace period first. I think our views are mostly similar actually. While I would do some details slightly differently, those details are not very important. I still think this patchset is a step in the right direction. As far as I'm concerned, feel free to push the whole set as is.
Anton Khirnov: > > As far as I'm concerned, feel free to push the whole set as is. > I pushed the set without removing the log.h headers. - Andreas
diff --git a/libavcodec/dump_extradata_bsf.c b/libavcodec/dump_extradata_bsf.c index 0b6d404792..fbff795380 100644 --- a/libavcodec/dump_extradata_bsf.c +++ b/libavcodec/dump_extradata_bsf.c @@ -23,8 +23,6 @@ #include "avcodec.h" #include "bsf.h" -#include "libavutil/log.h" -#include "libavutil/mem.h" #include "libavutil/opt.h" enum DumpFreq {
Since 33d18982fa03feb061c8f744a4f0a9175c1f63ab (the commit that introduced the new bsf API) allocating an enlarged buffer in case extradata needs to be added to a packet is done via av_new_packet(), so that libavutil/mem.h is no longer needed. Furthermore, remove libavutil/log.h. This function uses something provided by it (an AVClass), yet it does so only for AVOptions, not for logging purposes (there is no av_log() here), so only including libavutil/opt.h seems appropriate. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/dump_extradata_bsf.c | 2 -- 1 file changed, 2 deletions(-)