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

[Ronald S. Bultje]

Hi,

On Apr 29, 2010, at 8:15 AM, Sebastian Vater  
<cdgs.basty at googlemail.com> wrote:

> 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

No, as said on IRC, try replacing the AV_w/rn32() with w/r[bl]32().

Also think what that does and why it might fix this.

Ronald