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

[Robert Marston]

On Tue, Apr 8, 2008 at 4:24 AM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Tue, Apr 08, 2008 at 02:02:54AM +0200, Robert Marston wrote:
>

> > @@ -1235,6 +1246,68 @@
> >              }
> >          }
> >          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++;
> > +        }
> > +
> > +        for (count1 = 0; count1 < 14 ; count1++) {
> > +            next_left_sample = ((((*src >> 4) & 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;
> > +
> > +            next_left_sample = (((*src & 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);
> > +
> > +            if(st) {
> > +                src++;
> > +                next_right_sample = ((((*src >> 4) & 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;
> > +
> > +                *samples++ = (unsigned
> short)c->mxa_current_left_sample;
> > +
> > +                next_right_sample = (((*src & 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;
> > +            }
> > +            else {
> > +                *samples++ = (unsigned
> short)c->mxa_current_left_sample;
> > +            }
> > +            src++;
> > +        }
>
> This is full of code duplication, please remove all code duplication also
> it looks somewhat similar to the existing ADPCM_EA variants maybe some
> code
> can ba factored out or maybe it could be merged with one.
>

Removed a lot of redundancy in the code, there is still some duplication
when compared to the current EA ADPCM but to merge the two means creating
quiet a bit of confusing code and checking which format it is and offsetting
the data leads to code that is about the same in length. Can maybe look at
this again if you are really going to insist on it.


>
>
> [...]
> > typedef struct MaxisXADemuxContext {
>
> >     uint32_t xaId;
> >     uint32_t outSize;
>
> unused
>

 Removed


>
> >     int tag;
> >     int channels;
> >     uint32_t sampleRate;
>
> redundant
>

Removed tag.


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


>
>
> >     st->codec->codec_type = CODEC_TYPE_AUDIO;
> >     st->codec->codec_id = CODEC_ID_ADPCM_EA_MAXIS_XA;
> >     st->codec->codec_tag = xa->tag;
> >     st->codec->channels = xa->channels;
> >     st->codec->sample_rate = xa->sampleRate;
> >     st->codec->bits_per_sample = xa->bits;
> >     st->codec->bit_rate = xa->avgByteRate * 8;
> >     st->codec->block_align = xa->align;
>
> this can be vertically aligned like:
>
> st->codec->bits_per_sample = xa->bits;
> st->codec->bit_rate        = xa->avgByteRate * 8;
> st->codec->block_align     = xa->align;


Aligned these better.


> [...]
> > static int xa_read_packet(AVFormatContext *s,
> >                           AVPacket *pkt)
> > {
> >     MaxisXADemuxContext *xa = s->priv_data;
> >     ByteIOContext *pb = s->pb;
>
> >     int ret;
> >     unsigned int packet_size;
> >     packet_size = 15*xa->channels;/* 1 byte header and 14 byte samples *
> number channels per block */
> >     ret = av_get_packet(pb, pkt, packet_size);
>
> declaration and initialization can be merged
>

Done.


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


>
>
> >     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 += 2/xa->channels;
> >    }
>
> This looks wronge cannot see all ends.
>

Changed to what I think is more correct.

Also I attached a unified diff created with the GNU diff. If this is not
correct I am not sure what other diff program to use.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: maxis_ea_xa_format.txt
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080408/f91ce209/attachment.txt>