[FFmpeg-devel] [PATCH] fixes and optimization for ff_find_start_code()

[Michael Niedermayer]

On Thu, Feb 26, 2009 at 03:56:48PM +0100, Matthias Dahl wrote:
> Hi.
> 
> The following patch fixes bugs in ff_find_start_code() and
> optimizes it.
> 
>  (1) In certain cases, ff_find_start_code() could report false
>      positives, depending on the contents of start_code when the
>      the function gets called. 
> 
>      Cause: The for-loop operates on start_code without clearing
>             it. It simply shiftes the contents and checks if a
>             start code has been found- thus, shifting the old
>             contents.
> 
>             Example: start_code = 0x00000001 at call time
>             The second for-loop iteration will always return
>             a false positive.
> 
>      Looking through the ffmpeg code, this is not always taken into
>      account. Sometimes ff_find_start_code() gets called repeatedly
>      with old contents which could trigger a false positive randomly.
> 
>  (2) ff_find_start_code() always changes start_code without providing
>      a way to know if this is old contents, a found start_code or some
>      random data.
> 
> This patch fixes (1) and (2) and greatly simplifies the function
> along the way. The new behaviour won't cause any regressions and has
> been tested extensively:
> 
>  (1) start_code is still always modified
>  
>  (2) start_code contains either a valid start code or 0xFFFFFFFF
>      which can be used to shortcut certain evaluations (subject for
>      a different patch though)
> 
> The simplified version seems to be easier optimizable for gcc 4.3.x
> as with -O3 and some artificial benchmarks I made, the new version
> is approx. 33% faster in all cases. A oprofile in mythtv revealed
> also a speed up.
> 
> Hope this patch is okay. If there are any questions or remarks, please let me 
> know. Thanks for taking a look...
[...]
> 
> Index: mythtv/libs/libavcodec/mpegvideo.c
> ===================================================================
> --- trunk/libavcodec/mpegvideo.c	(revision 17538)
> +++ trunk/libavcodec/mpegvideo.c	(working copy)
> @@ -76,34 +76,23 @@
>  };
>  
>  
> -const uint8_t *ff_find_start_code(const uint8_t * restrict p, const uint8_t *end, uint32_t * restrict state){
> -    int i;
> -
> -    assert(p<=end);
> -    if(p>=end)
> -        return end;
> -
> -    for(i=0; i<3; i++){
> -        uint32_t tmp= *state << 8;
> -        *state= tmp + *(p++);
> -        if(tmp == 0x100 || p==end)
> -            return p;
> -    }
> +const uint8_t *ff_find_start_code(const uint8_t * restrict p, const uint8_t *end, uint32_t * restrict state)
> +{

this mixes cosmetic and functional changes and thus is unreviewable


> +    *state = 0xFFFFFFFF;

this though looks very wrong

[..]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20090226/17b41582/attachment.pgp>