[FFmpeg-soc] [Patch] Maxis EA XA decoder - GSoC Task
Michael Niedermayer
michaelni at gmx.at
Tue Apr 8 04:24:02 CEST 2008
On Tue, Apr 08, 2008 at 02:02:54AM +0200, Robert Marston wrote:
> On Tue, Apr 8, 2008 at 1:43 AM, Diego Biurrun <diego at biurrun.de> wrote:
>
> > On Tue, Apr 08, 2008 at 01:36:04AM +0200, Robert Marston wrote:
> > >
> > > Attached are the patches for Maxis EA .XA file decoding. Was out of
> > > town for the weekend hence the late submission.
> > >
> > > Please comment on any improvements/errors?
> >
> > Please do not split this patch by file - nothing could be more
> > inconvenient.
>
>
> Apologies, attached is one file with the changes.
>
>
> >
> >
> > Next up: Lose all the tabs and trailing whitespace.
>
>
> Tried to be pretty careful with this but may have let a few slip through.
> Any good way of checking this automatically?
sed can remove them, grep can find them and many editors can be configured
to not add them silently or to show them vissually.
yes there are still some left in there
[...]
> @@ -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.
[...]
> typedef struct MaxisXADemuxContext {
> uint32_t xaId;
> uint32_t outSize;
unused
> int tag;
> int channels;
> uint32_t sampleRate;
redundant
[...]
> 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
> 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;
[...]
> 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
> if (ret != packet_size) {
> av_log(s, AV_LOG_WARNING, "Size mismatch");
> return AVERROR(EIO);
> }
memleak
> 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 wrong
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/ce8b307b/attachment.pgp>
More information about the FFmpeg-soc
mailing list