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

[Tomas Härdin]

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