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

[Vladimir Voroshilov]

2007/10/4, M?ns Rullg?rd <mans at mansr.com>:
> 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.

Code around uses int16_t too.
So i'll commit this trivial fix in few hours if Michael will not say against.

-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719