[FFmpeg-devel] [PATCH] Move H.264 DSP functions from dsputil.c to h264dsp.c

[Panagiotis Issaris]

Hi,

Panagiotis Issaris schreef:
> Hi Aurelien,
> 
> Aurelien Jacobs schreef:
>> On Thu, 26 Jul 2007 18:43:08 +0200
>> Panagiotis Issaris <takis.issaris at uhasselt.be> wrote:
>>
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> Hi,
>>>
>>> The attached patch moves the H.264 DSP functions from dsputil.c to
>>> h264dsp.c. Regression tests succeed with this patch applied.
>>>
>>>  Makefile  |    2
>>>  dsputil.c |  320 ---------------------------------------------
>>>  h264dsp.c |  321 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 325 insertions(+), 318 deletions(-)
>>>
>>>
>>> I've tested the impact on the current SVN revision (9802) using various
>>> scenarios.
>>>
>>> First I checked how the patch altered a minimally configured ffmpeg's
>>> codesize:
>>>
>>> ./configure --disable-decoders --disable-demuxers --disable-parsers
>>> - --disable-protocols --disable-muxers --disable-encoders --disable-vhook
>>> - --disable-network --disable-zlib --disable-mmx
>>>
>>> Without this patch:
>>>  323522       0    4484  328006   50146 libavcodec/dsputil.o
>>>  592586     904    8920  602410   9312a ffmpeg
>>> - -rw-r--r-- 1 takis takis 888916 2007-07-26 17:37 libavcodec/dsputil.o
>>> - -rwxr-xr-x 1 takis takis 598076 2007-07-26 17:37 ffmpeg
>>>  4191 libavcodec/dsputil.c
>>>    81 libavcodec/h264dsp.c
>>>
>>>
>>> With this patch:
>>>    text    data     bss     dec     hex filename
>>>  297858       0    4484  302342   49d06 libavcodec/dsputil.o
>>>  566922     904    8920  576746   8ccea ffmpeg
>>> - -rwxr-xr-x 1 takis takis 571388 2007-07-26 17:34 ffmpeg
>>> - -rw-r--r-- 1 takis takis 807848 2007-07-26 17:34 libavcodec/dsputil.o
>>>  3877 libavcodec/dsputil.c
>>>   402 libavcodec/h264dsp.c
>>>
>>> Save 25664 bytes for ffmpeg in text segment,
>>> 26688 bytes in ffmpeg executable size.
>>> dsputil.o shrinks 25664 bytes.
>> Nice result :-)
> 
> Thanks :)
> 
> I'd also created a patch moving out the H.264 QPEL functions which had a
> much nicer impact, but unfortunately it is rather messy.
> 
> 
>> Few remarks:
>>
>>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>>> index e1685fe..9b05f62 100644
>>> --- a/libavcodec/Makefile
>>> +++ b/libavcodec/Makefile
>>>
>>> [...]
>>>
>>> @@ -2558,10 +2394,10 @@ void ff_put_vc1_mspel_mc00_c(uint8_t *dst, uint8_t *src, int stride, int rnd) {
>>>  }
>>>  #endif /* CONFIG_VC1_DECODER||CONFIG_WMV3_DECODER */
>>>  
>>> -#if defined(CONFIG_H264_ENCODER)
>>> +#if defined(CONFIG_H264_DECODER) || defined(CONFIG_H264_ENCODER)
>>>  /* H264 specific */
>>>  void ff_h264dsp_init(DSPContext* c, AVCodecContext *avctx);
>>> -#endif /* CONFIG_H264_ENCODER */
>>> +#endif /* CONFIG_H264_DECODER || CONFIG_H264_ENCODER */
>> Simply remove the #ifdef here. It does no good at all.
> 
> Fixed. I've removed the comment too as it seems rather obvious by the
> functionname that it is H264 specific and as I was the one who added it
> in the first place I'd think nobody would disagree.
> 
> 
>>> diff --git a/libavcodec/h264dsp.c b/libavcodec/h264dsp.c
>>> index 4f18afa..6a5dcbb 100644
>>> --- a/libavcodec/h264dsp.c
>>> +++ b/libavcodec/h264dsp.c
>>> @@ -28,6 +28,8 @@
>>>  
>>>  #include "dsputil.h"
>>>  
>>> +#if defined(CONFIG_H264_ENCODER)
>>> +
>>>  extern const uint8_t ff_div6[52];
>>>  extern const uint8_t ff_rem6[52];
>>>  
>>> @@ -73,9 +75,328 @@ static void h264_dct_c(DCTELEM block[4][4])
>>>      H264_DCT_PART2(2);
>>>      H264_DCT_PART2(3);
>>>  }
>>> +#endif /* CONFIG_H264_ENCODER */
>> Maybe spliting this part of the code into a h264dspenc.c file would be
>> even better (but that can be done later).
> 
> Yep, I'll look into that later.
> 
> 
>>>  void ff_h264dsp_init(DSPContext* c, AVCodecContext *avctx)
>>>  {
>>> +#if defined(CONFIG_H264_ENCODER)
>>>      c->h264_dct = h264_dct_c;
>>> +#endif
>> Here you could use if (ENABLE_H264_ENCODER).
> 
> Unfortunately, this won't work, as the H.264 encoder isn't in Subversion
> yet.
> 
> 
>> Except those remarks, the patch looks fine to me.
> 
> Thanks!

Ouch, I just noticed that with this patch I had accidentally moved lots
of the H.264 decoder DSP code in a file with a different header,
resulting in incorrect "Copyright by ..." lines.

Is it okay to just merge the "Copyright by ..." lines? Or would it be
better to split the h264dsp.c file right away in a h264dspenc.c and
h264dspdec.c as Aurelien suggested?

With friendly regards,
Takis
--
vCard: http://issaris.org/pi.vcf
PGP key: http://issaris.org/pi.key