[FFmpeg-devel] [PATCH 3/3] Add ADPCM IMA CRYO APC encoder

[Tomas Härdin]

ons 2018-12-12 klockan 00:05 +0100 skrev Michael Niedermayer:
> On Mon, Dec 10, 2018 at 12:32:51PM +0100, Tomas Härdin wrote:
> > 
> >  Changelog                      |    1 +
> >  libavcodec/adpcmenc.c          |   33 +++++++++++++++++++++++++++++++++
> >  libavcodec/allcodecs.c         |    1 +
> >  libavcodec/version.h           |    4 ++--
> >  tests/fate/acodec.mak          |    2 ++
> >  tests/ref/acodec/adpcm-ima_apc |    4 ++++
> >  6 files changed, 43 insertions(+), 2 deletions(-)
> > e86974218c35b93a077f5a38bcccb56ee3b36ca5  0003-Add-ADPCM-IMA-CRYO-APC-encoder.patch
> > From 32cc6a96f80b5406e8327d912c8fc38812e6a664 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjoppen at acc.umu.se>
> > Date: Fri, 23 Nov 2018 15:15:02 +0100
> > Subject: [PATCH 3/3] Add ADPCM IMA CRYO APC encoder
> > 
> > No trellis quantization yet
> > ---
> >  Changelog                      |  1 +
> >  libavcodec/adpcmenc.c          | 33 +++++++++++++++++++++++++++++++++
> >  libavcodec/allcodecs.c         |  1 +
> >  libavcodec/version.h           |  4 ++--
> >  tests/fate/acodec.mak          |  2 ++
> >  tests/ref/acodec/adpcm-ima_apc |  4 ++++
> >  6 files changed, 43 insertions(+), 2 deletions(-)
> >  create mode 100644 tests/ref/acodec/adpcm-ima_apc
> > 
> > diff --git a/Changelog b/Changelog
> > index f678feed65..e6ae0c1187 100644
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -11,6 +11,7 @@ version <next>:
> >  - dhav demuxer
> >  - PCM-DVD encoder
> >  - CRYO APC muxer
> > +- ADPCM IMA CRYO APC encoder
> >  
> >  
> >  version 4.1:
> > diff --git a/libavcodec/adpcmenc.c b/libavcodec/adpcmenc.c
> > index 668939c778..0d757d5b46 100644
> > --- a/libavcodec/adpcmenc.c
> > +++ b/libavcodec/adpcmenc.c
> > @@ -54,6 +54,7 @@ typedef struct ADPCMEncodeContext {
> >      TrellisNode *node_buf;
> >      TrellisNode **nodep_buf;
> >      uint8_t *trellis_hash;
> > +    int extradata_updated;
> >  } ADPCMEncodeContext;
> >  
> >  #define FREEZE_INTERVAL 128
> > @@ -124,6 +125,15 @@ static av_cold int adpcm_encode_init(AVCodecContext *avctx)
> >              bytestream_put_le16(&extradata, ff_adpcm_AdaptCoeff2[i] * 4);
> >          }
> >          break;
> > +    case AV_CODEC_ID_ADPCM_IMA_APC:
> > +        if (avctx->trellis) {
> > +            av_log(avctx, AV_LOG_ERROR, "trellis encoding not implemented for CRYO APC\n");
> > +            return AVERROR_PATCHWELCOME;
> > +        }
> > +        //extradata will be output in adpcm_encode_frame()
> > +        avctx->frame_size  = BLKSIZE * 2 / avctx->channels;
> > +        avctx->block_align = BLKSIZE;
> > +        break;
> >      case AV_CODEC_ID_ADPCM_YAMAHA:
> >          avctx->frame_size  = BLKSIZE * 2 / avctx->channels;
> >          avctx->block_align = BLKSIZE;
> > @@ -491,6 +501,28 @@ static int adpcm_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
> >      dst = avpkt->data;
> >  
> >      switch(avctx->codec->id) {
> > +    case AV_CODEC_ID_ADPCM_IMA_APC:
> > +        //initialize predictors using initial samples
> > +        if (!c->extradata_updated) {
> > +            uint8_t *side_data = av_packet_new_side_data(
> > +                avpkt, AV_PKT_DATA_NEW_EXTRADATA, 8);
> > +
> > +            if (!side_data) {
> > +                return AVERROR(ENOMEM);
> > +            }
> > +
> > +            for (ch = 0; ch < avctx->channels; ch++) {
> > +                c->status[ch].prev_sample = samples[ch];
> > +                bytestream_put_le32(&side_data, c->status[ch].prev_sample);
> > +            }
> > +            c->extradata_updated = 1;
> > +        }
> 
> This looks like something went wrong with how IMA_APC was implemented
> 
> the first samples are not a global header. extradata is a global header

For APC they are global. They are used to "warm up" the IMA ADPCM
decoder. Compare this to some of the other IMA variants like IMA_WAV
which have some bytes for initial samples in every packet.

> If its done as its implemented then extradata will not be available before
> the first packet and it will not work with many muxers
> in fact the muxer submitted here needs to special case the late occurance
> of extradata.
> I suspect the related code would be simpler if the data currently passed
> through extradata would be passed as part of the first packet (not counting
> code for compatibility with the old way of handling it)

You mean just prepending the APC header to the first packet? That
sounds a bit iffy. Another way could be to have the muxer delay writing
the header until apc_write_packet(), at which point the muxer can
demand some form of extradata be present (either from demuxer or
encoder).

There's no way to set extradata during codec init here - IMA APC needs
at least two samples for that. It's possible to set it to all zeroes,
but that would lead to clicking. Keep in mind IMA_APC effectively has a
block_align of 1, so extradata can't really be part of "packet" data.

I am not aware of any format besides .apc that supports IMA_APC, so
that point is a bit moot.

I considered modifying avformat_find_stream_info() to wait for at least
one packet before concluding there is no extradata, but that felt
wrong.

> another aspect of this is seeking. Seeking back to the begin has to reset
> the initial sample stuff. This would occur naturally if its in the first packet
> as is i think this doesnt work as extradata is not reused after init. That 
> issue is about the demuxer/decoder though but its also connected via the way
> extradata is used

You can't really seek anywhere except to t=0, if you want a bitexact
decode. Seeking will still kind of work because the ADPCM decoder
clamps samples. You'll get clicks, but there's not really much to do
about that.

One way to make seeking to t=0 OK is to output
AV_PKT_DATA_NEW_EXTRADATA for the very first packet always. Doing such
things in demuxers seems unusual, only flvdec and nutdec do at the
moment.

/Tomas