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

[Aurelien Jacobs]

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/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?

You need the approval of the maintainer of the code you are touching.
(Michael being the maintainer of everything not explicitly maintained
by someone else).
But here, the patch is trivial enough that I think I can OK it.
So go, apply it.
(don't worry, if Michael disagree, we will know soon enough)

Aurel