[FFmpeg-devel] [PATCH] split off h264_mb.h from h264.h

[Michael Niedermayer]

On Wed, Apr 14, 2010 at 11:59:31AM +0200, Jean-Daniel Dupas wrote:
> 
> Le 14 avr. 2010 ? 11:49, Michael Niedermayer a ?crit :
> 
> > 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
> > sample...)
> > there can be a generic "support everything" case as fallback to reduce the
> > number ...
> > 
> > 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.
> 
> libswscale is template based too, but it does not require big static unused function in headers file.
> Wouldn't be possible to do something similar here ? 

swscale is different, it uses templating for optimizations for that maybe
there are no unused functions.
if one uses templating for optimizing code to specific subsets of features
then this inevitably leads to unused code

thats besides the point that swscale is not a terribly good example of
pretty code. It surely works and works well and is maintainable by my
(being the author of the code) but it can surely be restructured toward
being prettier.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100414/28d7bc1b/attachment.pgp>