[FFmpeg-devel] [PATCH] split off h264_mb.h from h264.h
Wed Apr 14 11:49:34 CEST 2010
On Wed, Apr 14, 2010 at 03:15:54AM +0100, M?ns Rullg?rd wrote:
> Michael Niedermayer <michaelni at gmx.at> writes:
> > On Wed, Apr 14, 2010 at 02:19:10AM +0100, M?ns Rullg?rd wrote:
> >> Diego Biurrun <diego at biurrun.de> writes:
> >> > On Wed, Apr 14, 2010 at 02:42:24AM +0200, Michael Niedermayer wrote:
> >> >> On Wed, Apr 14, 2010 at 01:05:40AM +0200, Diego Biurrun wrote:
> >> >> > This patch splits off a separate header file for decode_mb_skip() and
> >> >> > friends from h264.h. I consider it a sensible idea in general and it
> >> >> > eliminates more of those pesky 'defined but not used' warnings.
> >> >> >
> >> >> > Since this just moves static functions like my previous patch I assume
> >> >> > it is similarly safe. OK to apply?
> >> >> >
> >> >> > h264.h | 541 -------------------------------------------
> >> >> > h264_cabac.c | 1
> >> >> > h264_cavlc.c | 1
> >> >> > h264_mb.h | 735 -----------------------------------------------------------
> >> >>
> >> >> iam against this patch. The placement of code into files becomes random
> >> >> with it and merely guided by avoidance of silly warnings
> >> >> these warnings should be disabled!
> >> Over my dead body.
> > I can mark the functions as av_unused if you prefer that
> No, that will not make it any better. Why do you place huge functions
> in a header file? If you intend for them to be inlined, they should
> be marked as such. If not, they belong in a C file with only a
> prototype in the header.
An efficient implmentaton of a h264 decoder uses some kind of templating.
That means for example that large parts of the decoder are compiled
several times, each time for a different case (like progressive, like
MBAFF like PAFF like CABAC like CAVLC, like for >8bit per
there can be a generic "support everything" case as fallback to reduce the
currently we only split on cabac / cavlc. But either way such spliting leads
to large functions being compiled multiple times and it is unlikely that
all functions all of a sudden would benefit from being marked as inline.
> >> > I beg to differ. h264.h is huge at 1300 lines of code and it's just a
> >> > grab bag of many things H.264-related with no particular order or
> >> > structure. Splitting the macroblock code off from it is not making
> >> > things more random and is not just guided by warning avoidance.
> >> In this case, the warnings are working exactly as intended. Unused
> >> functions being visible to the compiler are a sign of bad structure.
> > the patch doesnt improve the structure.
> > And we have hundreads of unused static inline functions in headers, and
> Yes, *inline*. These are small functions meant to be inlined. There
> is a reason gcc doesn't warn about unused inline functions.
> > any solution to that would make the code worse.
> > split headers based on 1 function per header or #ifdef around every function
> Now you're just being deliberately obtuse again. Please stop.
there may be solutions for this single function here but once real templating
is used in the decoder there will be non inline functions that are unused in
some of the cases. So short of gcc not being smart enough to realize they are
unused there will be warnings. I see only the solution of disabling the
warnings or adding huge amounts of ifdefs. If you feel thats obtuse iam sorry.
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel