[FFmpeg-devel] [PATCH] Move H.264 DSP functions from dsputil.c to h264dsp.c
Panagiotis Issaris
takis.issaris
Fri Jul 27 18:04:18 CEST 2007
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
More information about the ffmpeg-devel
mailing list