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

[Michael Niedermayer]

On Wed, Dec 12, 2018 at 01:25:15PM +0100, Tomas Härdin wrote:
> 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 the APC data was global, then it should be used for each packet.
that is initialize from extradata on each packet

That would not work, instead the data is only correct for the first packet
Which is why this data is not global IMHO.


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

yes, thats a possibility. adding the 8 bytes from extradata in front of it or
Simply cutting the first packet 12 bytes earlier would also be a possibility


[...]

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

I think the decoder - demuxer interface should follow the standard
semantics for extradata and packets even if there is currently no other
use than this pair. 

This is all just my oppinion, you are the one working on this you can
decide to implement this any way you like. iam fine with anything you
prefer, i just wanted to raise this issue as i spotted it ...

thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181212/209dfc01/attachment.sig>