diff mbox

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

Message ID 1544441571.21980.19.camel@acc.umu.se
State New
Headers show

Commit Message

Tomas Härdin Dec. 10, 2018, 11:32 a.m. UTC

Comments

Michael Niedermayer Dec. 11, 2018, 11:05 p.m. UTC | #1 
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@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
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)

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

thx

[...]
Tomas Härdin Dec. 12, 2018, 12:25 p.m. UTC | #2 
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@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
Michael Niedermayer Dec. 12, 2018, 6:55 p.m. UTC | #3 
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@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

[...]
Tomas Härdin Dec. 13, 2018, 2:34 p.m. UTC | #4 
ons 2018-12-12 klockan 19:55 +0100 skrev 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:
> > > > 
> > > > @@ -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.

It is "used" for every packet in the sense that correct decode of each
packet depends on correct decode of every preceding packet, back to
packet zero, which depends on those eight bytes. It also happens to be
the way the existing IMA_APC decoder works. See adpcm_decode_init() in
adpcm.c. I suspect there are more codecs that work similarly.

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

The decoder would need to "know" then that those eight bytes are
initial IMA state. It's not really possible tell in any way than a flag
in the demuxer, and at that point we're kind of in bikeshedding
territory IMO

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

As mentioned above, this is how the existing demuxer/decoder pair
works. So copying the behavior for the encoder/muxer seemed fair.
Justin Ruggles is the author of the existing decoder.

One issue struck me though: this codec only requires even number of
samples in and really has a block_align of 1. So some inputs will be
needlessly padded I think. Is setting frame_size to some reasonable
value but leaving block_align = 1 fine?

If the input is 600 frames of stereo samples (1200 samples) then the
output should be 600 bytes plus 32 bytes header.

/Tomas
Michael Niedermayer Dec. 13, 2018, 3:01 p.m. UTC | #5 
On Thu, Dec 13, 2018 at 03:34:18PM +0100, Tomas Härdin wrote:
> ons 2018-12-12 klockan 19:55 +0100 skrev 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:
> > > > > 
> > > > > @@ -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.
> 
> It is "used" for every packet in the sense that correct decode of each
> packet depends on correct decode of every preceding packet, back to
> packet zero, which depends on those eight bytes. It also happens to be
> the way the existing IMA_APC decoder works. See adpcm_decode_init() in
> adpcm.c. I suspect there are more codecs that work similarly.

yes, every codec that has I frames and P frames. Based on what you say
here the first I frame should be in extradata. Because future frames
depend on it and that makes it global.
APC: first samples, differentially coded next samples, differentially coded next samples, ...
MPEG:first I frame, differentially coded P frame, differentially coded P frame, ...

for APC the first samples are in extradata
for MPEG the first sample (I frame) is not in extradata

Its really the same as other codecs but its handled very differently in how its
mapped to extradata and packets

Again as i said, i dont mind if its left as it is, you are working on this not
me. Iam just trying to make really sure you understand what the issue is
iam seeing here. (seemed to me you dont see how this differs from other codecs)

Thanks

[...]
diff mbox

Patch

From 32cc6a96f80b5406e8327d912c8fc38812e6a664 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjoppen@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;
+        }
+        for (i = 0; i < frame->nb_samples*avctx->channels/2; i++) {
+            uint8_t l = adpcm_ima_compress_sample(&c->status[0],  samples[2*i+0]);
+            uint8_t r = adpcm_ima_compress_sample(&c->status[st], samples[2*i+1]);
+            *dst++ = (l<<4) | r;
+        }
+        break;
     case AV_CODEC_ID_ADPCM_IMA_WAV:
     {
         int blocks, j;
@@ -721,6 +753,7 @@  AVCodec ff_ ## name_ ## _encoder = {                        \
 
 ADPCM_ENCODER(AV_CODEC_ID_ADPCM_IMA_QT,  adpcm_ima_qt,  sample_fmts_p, "ADPCM IMA QuickTime");
 ADPCM_ENCODER(AV_CODEC_ID_ADPCM_IMA_WAV, adpcm_ima_wav, sample_fmts_p, "ADPCM IMA WAV");
+ADPCM_ENCODER(AV_CODEC_ID_ADPCM_IMA_APC, adpcm_ima_apc, sample_fmts,   "ADPCM IMA CRYO APC");
 ADPCM_ENCODER(AV_CODEC_ID_ADPCM_MS,      adpcm_ms,      sample_fmts,   "ADPCM Microsoft");
 ADPCM_ENCODER(AV_CODEC_ID_ADPCM_SWF,     adpcm_swf,     sample_fmts,   "ADPCM Shockwave Flash");
 ADPCM_ENCODER(AV_CODEC_ID_ADPCM_YAMAHA,  adpcm_yamaha,  sample_fmts,   "ADPCM Yamaha");
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index d70646e91a..873f236531 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -586,6 +586,7 @@  extern AVCodec ff_adpcm_g726_decoder;
 extern AVCodec ff_adpcm_g726le_encoder;
 extern AVCodec ff_adpcm_g726le_decoder;
 extern AVCodec ff_adpcm_ima_amv_decoder;
+extern AVCodec ff_adpcm_ima_apc_encoder;
 extern AVCodec ff_adpcm_ima_apc_decoder;
 extern AVCodec ff_adpcm_ima_dat4_decoder;
 extern AVCodec ff_adpcm_ima_dk3_decoder;
diff --git a/libavcodec/version.h b/libavcodec/version.h
index f758093582..5677a7feba 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -28,8 +28,8 @@ 
 #include "libavutil/version.h"
 
 #define LIBAVCODEC_VERSION_MAJOR  58
-#define LIBAVCODEC_VERSION_MINOR  41
-#define LIBAVCODEC_VERSION_MICRO 101
+#define LIBAVCODEC_VERSION_MINOR  42
+#define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \
diff --git a/tests/fate/acodec.mak b/tests/fate/acodec.mak
index 80d26de0f9..69bfa6940e 100644
--- a/tests/fate/acodec.mak
+++ b/tests/fate/acodec.mak
@@ -45,6 +45,7 @@  fate-acodec-pcm-u%le: FMT = nut
 fate-acodec-pcm-f%be: FMT = au
 
 FATE_ACODEC_ADPCM-$(call ENCDEC, ADPCM_ADX,     ADX)  += adx
+FATE_ACODEC_ADPCM-$(call ENCDEC, ADPCM_IMA_APC, APC)  += ima_apc
 FATE_ACODEC_ADPCM-$(call ENCDEC, ADPCM_IMA_QT,  AIFF) += ima_qt
 FATE_ACODEC_ADPCM-$(call ENCDEC, ADPCM_IMA_WAV, WAV)  += ima_wav
 FATE_ACODEC_ADPCM-$(call ENCDEC, ADPCM_MS,      WAV)  += ms
@@ -58,6 +59,7 @@  fate-acodec-adpcm: $(FATE_ACODEC_ADPCM)
 fate-acodec-adpcm-%: CODEC = adpcm_$(@:fate-acodec-adpcm-%=%)
 
 fate-acodec-adpcm-adx:     FMT = adx
+fate-acodec-adpcm-ima_apc: FMT = apc
 fate-acodec-adpcm-ima_qt:  FMT = aiff
 fate-acodec-adpcm-ima_wav: FMT = wav
 fate-acodec-adpcm-ms:      FMT = wav
diff --git a/tests/ref/acodec/adpcm-ima_apc b/tests/ref/acodec/adpcm-ima_apc
new file mode 100644
index 0000000000..f168734c78
--- /dev/null
+++ b/tests/ref/acodec/adpcm-ima_apc
@@ -0,0 +1,4 @@ 
+45aca515c679bb0c315df766432d5630 *tests/data/fate/acodec-adpcm-ima_apc.apc
+265248 tests/data/fate/acodec-adpcm-ima_apc.apc
+03fc41cf61b7a160359147cd6363562a *tests/data/fate/acodec-adpcm-ima_apc.out.wav
+stddev:  904.04 PSNR: 37.21 MAXDIFF:34026 bytes:  1058400/  1060864
-- 
2.11.0