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

[Vladimir Voroshilov]

2007/10/4, Vladimir Voroshilov <voroshil at gmail.com>:
> 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.

Fix applied.

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