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

[Michael Niedermayer]

On Tue, Apr 08, 2008 at 09:29:41PM +0200, Robert Marston wrote:
> On Tue, Apr 8, 2008 at 8:11 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Tue, Apr 08, 2008 at 04:43:25PM +0200, Robert Marston wrote:
> >  [...]
> >
> >
> >
> >
> >  >
> >  >
> >  > >
> >  > >
> >  > > [...]
> >  > > > 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 ...
> >
> 
> Set this to 64 then?

yes


[...]
> 
> >
> >  [...]
> >  >  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
> >
> 
> Not sure why unneeded since for each block the previous blocks prev
> and curr samples need to be remembered and are not present in the
> stream.

Other adpcm decoder need them too, they though dont seem to need these 4
variables because they work fine without them.


> 
> >
> >  [...]
> >  > @@ -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
> >
> 
> Not sure what the duplication refers too? If statement has duplicate
> code to what precedes it or duplicate to already implemented EA ADPCM?

I meant precedes here. This doesnt mean its not duplicte to an existing
ADPCM decoder (which ive not investigated carefully yet).


[...]
> >  [...]
> >  > +    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
> >
> 
> What are you referring to when you say this is wrong?

I mean that it can segfault


> 
> >
> >  > +    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
> >
> >
> 
> Is this then number_samples per packet / sample rate?

no, what is the time_base?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/2cbdb382/attachment.pgp>