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

[Sebastian Vater]

M?ns Rullg?rd a ?crit :
> Sebastian Vater <cdgs.basty at googlemail.com> writes:
>
>   
>> 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.
>>>   
>>>       
>> So, since there were changes in git/svn because unsigned => signed, my
>> earlier patches regarding this won't work anymore.
>>
>> So, here's an updated version.
>>
>> -- 
>>
>> Best regards,
>>                    :-) Basty/CDGS (-:
>>
>> diff --git a/libavcodec/iff.c b/libavcodec/iff.c
>> index be13f4e..7af3d37 100644
>> --- a/libavcodec/iff.c
>> +++ b/libavcodec/iff.c
>> @@ -72,7 +72,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>          return AVERROR_INVALIDDATA;
>>      }
>>  
>> -    s->planesize = avctx->width >> 3;
>> +    s->planesize = FFALIGN(avctx->width, 16) >> 3; // Align plane size in bits to word-boundary
>>      s->planebuf = av_malloc(s->planesize + FF_INPUT_BUFFER_PADDING_SIZE);
>>      if (!s->planebuf)
>>          return AVERROR(ENOMEM);
>> @@ -99,7 +99,7 @@ static void decodeplane8(uint8_t *dst, const uint8_t *const buf, int buf_size, i
>>  {
>>      GetBitContext gb;
>>      int i;
>> -    const int b = (buf_size * 8) + bps - 1;
>> +    const int b = buf_size * 8;
>>      init_get_bits(&gb, buf, buf_size * 8);
>>      for(i = 0; i < b; i++) {
>>          dst[i] |= get_bits1(&gb) << plane;
>> @@ -118,7 +118,7 @@ static void decodeplane32(uint32_t *dst, const uint8_t *const buf, int buf_size,
>>  {
>>      GetBitContext gb;
>>      int i;
>> -    const int b = (buf_size * 8) + bps - 1;
>> +    const int b = buf_size * 8;
>>      init_get_bits(&gb, buf, buf_size * 8);
>>      for(i = 0; i < b; i++) {
>>          dst[i] |= get_bits1(&gb) << plane;
>>     
>
> If this works, I have no objections from a style or performance POV.
>   

The expression [...] + bps - 1 is clearly a bug, you see this by
watching init_get_bits which also does only initialize buf_size * 8
bits...so the actual loop would simply read bps - 1 bits too much...

-- 

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