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

[Michael Niedermayer]

On Wed, Apr 09, 2008 at 04:28:50PM +0200, Robert Marston wrote:
> Attached patch has the following changes to it.
[...]
> >
> >  [...]
> >  >  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 */
> >
> Unless there is a way of getting the samples from the previous decoded
> frame then this is needed 

> since other ADPCM formats have the previous
> and current samples encoded in the stream

No some do not.


[...]
> >
> >  > +    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
> >
> Removed this as I am unsure of the time base.

So i assume you are sure that its unneeded to set pts here? If so please
explain why it is unneeded. If not please set pts to a correct value.


[...]
> @@ -667,8 +669,12 @@
>  {
>      ADPCMContext *c = avctx->priv_data;
>      unsigned int max_channels = 2;
> -

cosmetic change


[...]
> @@ -1235,6 +1242,30 @@
>              }
>          }
>          break;
> +    case CODEC_ID_ADPCM_EA_MAXIS_XA:

> +        for(channel = 0; channel < avctx->channels; channel++) {
> +              for (i=0; i<2; i++)
> +                    coeff[channel][i] = ea_adpcm_table[((*src >> 4) & 0x0F)+(4*i)];

one of the operations in there does nothing


> +            shift[channel] = (*src & 0x0F) + 8;
> +            src++;
> +        }

indention is inconsistant



> +        for (count1 = 0; count1 < ((buf_size - avctx->channels) / avctx->channels) ; count1++) {
> +            for(i = 4; i >= 0; i-=4) { /* Pairwise samples LL RR (st) or LL LL (mono) */
> +            int32_t sample;

random indention


> +            int prev_next = 0;

contradictionary variable name


> +                for(channel = 0; channel < avctx->channels; channel++, prev_next+=2) {
> +                    sample = ((((*(src+channel) >> i) & 0x0F) << 0x1C) >> shift[channel]);
> +                    sample = (sample +

> +                        (c->curr_prev_samples[prev_next+1] * coeff[channel][0]) +
> +                        (c->curr_prev_samples[prev_next] * coeff[channel][1]) + 0x80) >> 8;

this can be vertically aligned


> +                    c->curr_prev_samples[prev_next] = c->curr_prev_samples[prev_next+1];
> +                    c->curr_prev_samples[prev_next+1] = av_clip_int16(sample);

> +                    *samples++ = (unsigned short)c->curr_prev_samples[prev_next+1];

unneeded cast


[...]
> +typedef struct MaxisXADemuxContext {

> +    uint32_t out_size;
> +    uint32_t  sent_bytes;

inconsistant whitespace

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- 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/20080409/2ff88198/attachment.pgp>