[FFmpeg-devel] [PATCH] fix clicks in ADPCM IMA AMV decoder

[Måns Rullgård]

Vitor Sessak <vitor1001 at gmail.com> writes:

> Vladimir Voroshilov wrote:
>> 2007/10/4, Vitor Sessak <vitor1001 at gmail.com>:
>>> Hi
>>>
>>> Vladimir Voroshilov wrote:
>>>>>>>>> Vladimir Voroshilov wrote:
>>>>>>>>>> Hi, All
>>>>>>>>>>
>>>>>> Index: libavcodec/adpcm.c
>>>>>> ===================================================================
>>>>>> --- libavcodec/adpcm.c        (revision 10652)
>>>>>> +++ libavcodec/adpcm.c        (working copy)
>>>>>> @@ -1184,10 +1184,8 @@
>>>>>>          break;
>>>>>>      case CODEC_ID_ADPCM_IMA_AMV:
>>>>>>      case CODEC_ID_ADPCM_IMA_SMJPEG:
>>>>>> -        c->status[0].predictor = *src;
>>>>>> -        src += 2;
>>>>>> -        c->status[0].step_index = *src++;
>>>>>> -        src++;  /* skip another byte before getting to the meat */
>>>>>> +        c->status[0].predictor = (signed short)bytestream_get_le16(&src);
>>>>>> +        c->status[0].step_index = bytestream_get_le16(&src);
>>>>> I suppose the signed short cast is useless. If so, please remove it.
>>>>> If you remove it, you could also align the = sign.
>>>> Signed cast is required (exactly this signed cast removes clicks in sound).
>>> Would just this do it?
>>>
>>> c->status[0].predictor = (signed) bytestream_get_le16(&src);
>> 
>> No, "(signed)" cast does not work.
>
> Ok... I promise I'll test it before suggesting next time :-)

The cast makes a difference here because bytestream_get_le16() returns
an unsigned int with the high 16 bits zero.  The cast to signed short
and the following implicit widening cast act to sign-extend the low 16
bits to 32 bits when storing the value in an int.

Casting to signed short is actually not entirely correct.  Since the
size of the value to be sign-extended is exactly 16 bits, a cast to
int16_t should be used.  We cannot know for sure that short is in fact
16 bits, even if this virtually always is the case.

-- 
M?ns Rullg?rd
mans at mansr.com