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

[Vladimir Voroshilov]

2007/10/4, Aurelien Jacobs <aurel at gnuage.org>:
> Vladimir Voroshilov wrote:
>
> > 2007/10/4, Aurelien Jacobs <aurel at gnuage.org>:
> > > Vladimir Voroshilov wrote:
> > >
> > > > 2007/10/4, Aurelien Jacobs <aurel at gnuage.org>:
> > > > > Vladimir Voroshilov wrote:
> > > > >
> > > > > > 2007/10/3, Vitor Sessak <vitor1001 at gmail.com>:
> > > > > > > Hi
> > > > > > >
> > > > > > > Vladimir Voroshilov wrote:
> > > > > > > > Hi, All
> > > > > > > >
> > > > > > > > Attached simle patches fixes clicks in audio when playing AMV files.
> > > > > > >
> > > > > > > Cool. Is it still not loud enough?
> > > > > >
> > > > > > Hm. It is enough loud for me now.
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > Index: libavcodec/adpcm.c
> > > > > > > > ===================================================================
> > > > > > > > --- libavcodec/adpcm.c        (revision 10652)
> > > > > > > > +++ libavcodec/adpcm.c        (working copy)
> > > > > > > > @@ -1184,7 +1184,7 @@
> > > > > > > >          break;
> > > > > > > >      case CODEC_ID_ADPCM_IMA_AMV:
> > > > > > > >      case CODEC_ID_ADPCM_IMA_SMJPEG:
> > > > > > > > -        c->status[0].predictor = *src;
> > > > > > > > +        c->status[0].predictor = (int16_t)AV_RL16(src);
> > > > > > > >          src += 2;
> > > > > > >
> > > > > > > c->status[0].predictor = bytestream_get_le16(&src);
> > > > > >
> > > > > > I've made simple (imho) solution which also comply with other code style.
> > > > >
> > > > > This new patch is not good. It could segfault on arch which requires strict
> > > > > alignment if src is not properly aligned.
> > > > > The solution proposed by Vitor is the right one, and it's simpler (it saves
> > > > > you the src += 2).
> > > >
> > > > Thanks. Didn't know that.
> > > >
> > > > > BTW: I wonder if step_index shouldn't be read as an int16_t as well ?
> > > >
> > > > According to multimedia wiki it should.
> > > > This also also shows why fourth byte is always zero (index<=88)
> > > > Included in patch.
> > > >
> > > > 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).
>
> OK.
>
> > > Except those remarks, patch is OK. I think you can apply it yourself.
> >
> > Do i have write access :-O ?
>
> AFAIR, you have write access to mplayer, so you should also have write
> access to ffmpeg (just don't abuse it, keep posting patches here and
> only commit the one which gets approved).

But i still need to get Michael's approvement before commit, right?

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