[FFmpeg-devel] [PATCH] Fix non-rounding up to next 16-bit aligned bug in IFF decoder

[Sebastian Vater]

Michael Niedermayer a ?crit :
> On Wed, Apr 28, 2010 at 02:23:07PM +0200, Sebastian Vater wrote:
>   
>> Sebastian Vater a ?crit :
>>     
>>> I have fixed the wrong IFF decoding issue in the IFF decoder.
>>>
>>> The reason is that the IFF docs say that each line in the BODY chunk has
>>> it's width rounded up to next 16-bit boundary, such that each new line
>>> begins on a word boundary (address divisible by 2).
>>>
>>> Please review and apply.
>>>
>>> I will do the heavy optimization stuff now based on this.
>>>
>>>   
>>>       
>> Heavy optimization for decodeplane8 done. Patch attached.
>>
>> Please note that I used a different IFF file for benchmarking this (one
>> which was displayed incorrectly before fix non-rounding on word boundary
>> patch applied).
>>
>> This image also has a larger width and thus the decodeplane8 function is
>> called more often per line (should yield more accurate results).
>>
>> Benchmarks for fix non-rounding on word boundary patch:
>> basty at cdgs-basty:~/src/ffmpeg/build$ ./ffplay ../patches/MRLake.iff
>> FFplay version git-svn-r22986, Copyright (c) 2003-2010 the FFmpeg developers
>>   built on Apr 28 2010 13:22:35 with gcc 4.2.4 (Ubuntu 4.2.4-1ubuntu4)
>>   configuration:
>>   libavutil     50.14. 0 / 50.14. 0
>>   libavcodec    52.66. 0 / 52.66. 0
>>   libavformat   52.61. 0 / 52.61. 0
>>   libavdevice   52. 2. 0 / 52. 2. 0
>>   libswscale     0.10. 0 /  0.10. 0
>> [IFF @ 0x8b32790]Estimating duration from bitrate, this may be inaccurate
>> Input #0, IFF, from '../patches/MRLake.iff':
>>   Duration: N/A, bitrate: N/A
>>     Stream #0.0: Video: iff_byterun1, pal8, 737x595, PAR 17:20 DAR
>> 737:700, 90k tbr, 90k tbn, 90k tbc
>> 57150 dezicycles in decodeplane8, 1 runs, 0 skips
>> 55335 dezicycles in decodeplane8, 2 runs, 0 skips
>> 54387 dezicycles in decodeplane8, 4 runs, 0 skips
>> 53866 dezicycles in decodeplane8, 8 runs, 0 skips
>> 53598 dezicycles in decodeplane8, 16 runs, 0 skips
>> 53414 dezicycles in decodeplane8, 32 runs, 0 skips
>> 53297 dezicycles in decodeplane8, 64 runs, 0 skips
>> 53249 dezicycles in decodeplane8, 127 runs, 1 skips
>> 53216 dezicycles in decodeplane8, 255 runs, 1 skips
>> 53201 dezicycles in decodeplane8, 511 runs, 1 skips
>> 53194 dezicycles in decodeplane8, 1023 runs, 1 skips
>> 53224 dezicycles in decodeplane8, 2046 runs, 2 skips
>> 53429 dezicycles in decodeplane8, 4091 runs, 5 skipsB sq=    0B f=0/0
>>    0.51 A-V:  0.000 s:0.0 aq=    0KB vq=    0KB sq=    0B f=0/0   0/0
>>
>> Benchmarks with heavy optimization based on fix non-rounding to word
>> boundary patch:
>> basty at cdgs-basty:~/src/ffmpeg/build$ ./ffplay ../patches/MRLake.iff
>> FFplay version git-svn-r22986, Copyright (c) 2003-2010 the FFmpeg developers
>>   built on Apr 28 2010 13:22:35 with gcc 4.2.4 (Ubuntu 4.2.4-1ubuntu4)
>>   configuration:
>>   libavutil     50.14. 0 / 50.14. 0
>>   libavcodec    52.66. 0 / 52.66. 0
>>   libavformat   52.61. 0 / 52.61. 0
>>   libavdevice   52. 2. 0 / 52. 2. 0
>>   libswscale     0.10. 0 /  0.10. 0
>> [IFF @ 0x8b32790]Estimating duration from bitrate, this may be inaccurate
>> Input #0, IFF, from '../patches/MRLake.iff':
>>   Duration: N/A, bitrate: N/A
>>     Stream #0.0: Video: iff_byterun1, pal8, 737x595, PAR 17:20 DAR
>> 737:700, 90k tbr, 90k tbn, 90k tbc
>> 24900 dezicycles in decodeplane8, 1 runs, 0 skips
>> 22215 dezicycles in decodeplane8, 2 runs, 0 skips
>> 19937 dezicycles in decodeplane8, 4 runs, 0 skips
>> 17513 dezicycles in decodeplane8, 8 runs, 0 skips
>> 16066 dezicycles in decodeplane8, 16 runs, 0 skips
>> 15322 dezicycles in decodeplane8, 32 runs, 0 skips
>> 14925 dezicycles in decodeplane8, 64 runs, 0 skips
>> 14719 dezicycles in decodeplane8, 128 runs, 0 skips
>> 14596 dezicycles in decodeplane8, 256 runs, 0 skips
>> 14543 dezicycles in decodeplane8, 512 runs, 0 skips
>> 14516 dezicycles in decodeplane8, 1024 runs, 0 skips
>> 14503 dezicycles in decodeplane8, 2046 runs, 2 skips
>> 14492 dezicycles in decodeplane8, 4093 runs, 3 skips
>>    0.59 A-V:  0.000 s:0.0 aq=    0KB vq=    0KB sq=    0B f=0/0   0/0
>>
>> -- 
>>
>> Best regards,
>>                    :-) Basty/CDGS (-:
>>
>>     
>
>   
>>  iff.c |   37 ++++++++++++++++++++++++++++++++++---
>>  1 file changed, 34 insertions(+), 3 deletions(-)
>> c47530e98c6fc28fa943f51b8b5d14b64cafa5c5  iff-decoder-fix-heavy-dp8.patch
>>     
>
> just wanted to say iam still fine with this patch once its tested on
> little & big & the other devels are ok with it too
>
> [...]
>   

Bad news here...

I just got a report from the guy who did sent the IFF files which were
decoded wrong and he said that it doesn't work on big-endian.

I assume that the tables have to be swapped around. Hence, what's the
ffmpeg prefered way of checking if it's big or little endian (I just
will then do sth. like):
#ifdef BIG_ENDIAN
  // declare table for BE here
#else
  // declare table for LE here

So please remove the patch regarding this optimization from git!

-- 

Best regards,
                   :-) Basty/CDGS (-: