[FFmpeg-soc] [Patch] Maxis EA XA decoder - GSoC Task

[Michael Niedermayer]

On Tue, Apr 08, 2008 at 04:43:25PM +0200, Robert Marston wrote:
[...]
> >
> > >     int tag;
> > >     int channels;
> > >     uint32_t sampleRate;
> >
> > redundant
> >
> 
> Removed tag.

The others are redundant as well


> 
> 
> >
> >
> > [...]
> > > static int xa_read_header(AVFormatContext *s,
> > >                           AVFormatParameters *ap)
> > > {
> > >     MaxisXADemuxContext *xa = s->priv_data;
> > >     AVStream *st;
> > >
> > >     /* Extract Header Information from xa file */
> > >     if (!xa_extract_header_info(s))
> > >         return AVERROR(EIO);
> > >
> > >     /*Set up the XA Audio Decoder*/
> > >     st = av_new_stream(s, 0);
> > >     if (!st)
> > >         return AVERROR(ENOMEM);
> >
> > >     av_set_pts_info(st, 33, 1, xa->sampleRate);
> >
> > i doubt 33 is correct
> 
> 
> I am not entirely sure what the wrap bits is and what its value should be, I
> copied that from the original ADPCM EA in  electronicarts.c

Its the number of bits in the pts, mpeg stores them in 33 bits people
copied that all over the place without undestanding it ...


[...]
> >
> >
> > >     if (ret != packet_size) {
> > >         av_log(s, AV_LOG_WARNING, "Size mismatch");
> > >         return AVERROR(EIO);
> > >     }
> >
> > memleak
> >
> 
> Added a av_free for pkt here. The test files seem to have an extra few
> samples but according to the description the samples are contained in blocks
> of 14 bytes for each channel and so I am not sure why there are these extra
> samples in the file.

Probably because the source did not contain a multiple of 28 samples, it
would be nice if tha last incomplete packet would be decoded as well.
Assuming iam guessing correct that it is actual audio at all instead of
junk.


[...]
>  typedef struct ADPCMContext {
>      int channel; /* for stereo MOVs, decode left, then decode right, then tell it's decoded */
>      ADPCMChannelStatus status[6];
> +    int32_t mxa_current_left_sample; /*Needed to keep track of left and right samples for Maxis EA XA */
> +    int32_t mxa_previous_left_sample;
> +    int32_t mxa_current_right_sample;
> +    int32_t mxa_previous_right_sample;
>  } ADPCMContext;
>  
>  /* XXX: implement encoding */

unneeded


[...]
> @@ -1235,6 +1246,53 @@
>              }
>          }
>          break;
> +    case CODEC_ID_ADPCM_EA_MAXIS_XA:
> +        channel = avctx->channels;
> +
> +        if(channel > 2) {
> +            av_log(avctx, AV_LOG_ERROR, "Only 1 or 2 channels supported");
> +            return -1;
> +        }
> +

> +        coeff1l = ea_adpcm_table[(*src >> 4) & 0x0F];
> +        coeff2l = ea_adpcm_table[((*src >> 4) & 0x0F) + 4];
> +        shift_left = (*src & 0x0F) + 8;
> +        src++;
> +
> +        if(st) {
> +            coeff1r = ea_adpcm_table[(*src >> 4) & 0x0F];
> +            coeff2r = ea_adpcm_table[((*src >> 4) & 0x0F) + 4];
> +            shift_right = (*src & 0x0F) + 8;
> +            src++;
> +        }

duplicate


> +
> +        for (count1 = 0; count1 < 14 ; count1++) {
> +            for(count2 = 4; count2 >= 0; count2-=4) { /* Pairwise samples LL RR (st) or LL LL (mono) */

> +                next_left_sample = ((((*src >> count2) & 0x0F) << 0x1C) >> shift_left);
> +                next_left_sample = (next_left_sample +
> +                    (c->mxa_current_left_sample * coeff1l) +
> +                    (c->mxa_previous_left_sample * coeff2l) + 0x80) >> 8;
> +                c->mxa_previous_left_sample = c->mxa_current_left_sample;
> +                c->mxa_current_left_sample = av_clip_int16(next_left_sample);
> +                *samples++ = (unsigned short)c->mxa_current_left_sample;
> +
> +                if(st) {
> +                    src++;
> +                    next_right_sample = ((((*src >> count2) & 0x0F) << 0x1C) >> shift_right);
> +                    next_right_sample = (next_right_sample +
> +                        (c->mxa_current_right_sample * coeff1r) +
> +                        (c->mxa_previous_right_sample * coeff2r) + 0x80) >> 8;
> +                    c->mxa_previous_right_sample = c->mxa_current_right_sample;
> +                    c->mxa_current_right_sample = av_clip_int16(next_right_sample);
> +                    *samples++ = (unsigned short)c->mxa_current_right_sample;
> +                    src--;
> +                }

duplicate


[...]
> +typedef struct MaxisXADemuxContext {
> +    int channels;
> +    uint32_t sampleRate;
> +    int bits;

> +    int audio_stream_index;

unneeded


[...]
> +static int xa_extract_header_info(AVFormatContext *s)
> +{
> +    MaxisXADemuxContext *xa = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +
> +    url_fskip(pb, 10); /* Skip the XA ID, data size and tag parameters */
> +    xa->channels = get_le16(pb);
> +    xa->sampleRate = get_le32(pb);
> +    url_fskip(pb, 6); /* Skip the avgByteRate and Align parameters, since can be calculated */
> +    xa->bits = get_le16(pb);
> +
> +    av_log(s, AV_LOG_DEBUG, "Channels = %d Sample Rate = %d, Bits = %d\n", xa->channels, xa->sampleRate, xa->bits);
> +
> +    return 1;
> +}

spliting this out in its own function makes no sense


> +static int xa_read_header(AVFormatContext *s,
> +               AVFormatParameters *ap)
> +{
> +    MaxisXADemuxContext *xa = s->priv_data;
> +    AVStream *st;
> +
> +    /* Extract Header Information from xa file */
> +    if (!xa_extract_header_info(s))
> +        return AVERROR(EIO);
> +
> +    /*Set up the XA Audio Decoder*/
> +    st = av_new_stream(s, 0);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +    av_set_pts_info(st, 33, 1, xa->sampleRate);
> +    st->codec->codec_type      = CODEC_TYPE_AUDIO;
> +    st->codec->codec_id        = CODEC_ID_ADPCM_EA_MAXIS_XA;
> +    st->codec->bits_per_sample = xa->bits;

> +    st->codec->block_align     = (xa->bits/8) * xa->channels;

This is wrong.


[...]
> +    packet_size = 15*xa->channels;/* 1 byte header and 14 bytes worth of samples * number channels per block */
> +    if (av_get_packet(pb, pkt, packet_size) != packet_size) {
> +        av_log(s, AV_LOG_WARNING, "Size mismatch when reading data from file\n");
> +        av_free_packet(pkt);
> +        return AVERROR(EIO);
> +    }

This is wrong as well


> +    else {
> +        pkt->stream_index = xa->audio_stream_index;
> +        pkt->pts = 90000;
> +        pkt->pts *= xa->audio_frame_counter;
> +        pkt->pts /= xa->sampleRate;
> +        xa->audio_frame_counter += (14 * xa->channels);  /* 14 Samples per channel  */

to quote the documentation
"int64_t pts;                            ///< presentation time stamp in time_base units"

This is not what you set it to


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080408/18a83e86/attachment.pgp>