[FFmpeg-devel] [PATCH] EA ADPCM R1, R2 and R3
Aurelien Jacobs
aurel
Mon Oct 22 00:05:53 CEST 2007
On Sat, 20 Oct 2007 20:11:58 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:
> Hi
>
> On Sat, Oct 20, 2007 at 03:42:35PM +0200, Aurelien Jacobs wrote:
> > Hi,
> >
> > Attached patch adds support for EA ADPCM R1, R2 and R3.
> > It's based on the patch from Peter Ross.
> > I intend to apply it soon.
> >
> > Aurel
>
> [...]
> > @@ -637,9 +638,12 @@
> > {
> > ADPCMContext *c = avctx->priv_data;
> >
> > - if(avctx->channels > 2U){
> > + if ((avctx->channels > 2
> > + && avctx->codec->id != CODEC_ID_ADPCM_EA_R1
> > + && avctx->codec->id != CODEC_ID_ADPCM_EA_R2
> > + && avctx->codec->id != CODEC_ID_ADPCM_EA_R3)
> > + || avctx->channels > 6)
> > return -1;
> > - }
>
> this is wrong, channels is signed so it must be 2U not 2
OK.
> also this is not the most readable way to check this especially if more
> codecs get added ...
I agree. After a quick look, it seems this check is here to prevent
overflow of the status[] context var. As this patch increase the size
of status[] to 6, I think it's safe to simply check for channels > 6U.
I hope I'm right here ?
> > c->channel = 0;
> > c->status[0].predictor = c->status[1].predictor = 0;
> > @@ -1182,6 +1186,78 @@
> > }
> > }
> > break;
> > + case CODEC_ID_ADPCM_EA_R1:
> > + case CODEC_ID_ADPCM_EA_R2:
> > + case CODEC_ID_ADPCM_EA_R3: {
> > + /* channel numbering
> > + 2chan: 0=fl, 1=fr
> > + 4chan: 0=fl, 1=rl, 2=fr, 3=rr
> > + 6chan: 0=fl, 1=c, 2=fr, 3=rl, 4=rr, 5=sub */
> > + const int big_endian = avctx->codec->id == CODEC_ID_ADPCM_EA_R3;
> > + unsigned int channel;
> > + uint16_t *samplesC;
> > + uint8_t *srcC;
> > +
> > + samples_in_chunk = (big_endian ? bytestream_get_be32(&src)
> > + : bytestream_get_le32(&src)) / 28;
> > +
> > + for (channel=0; channel<avctx->channels; channel++) {
> > + srcC = src + (big_endian ? bytestream_get_be32(&src)
> > + : bytestream_get_le32(&src))
> > + + (avctx->channels-channel-1) * 4;
> > + samplesC = samples + channel;
> > +
> > + if (avctx->codec->id == CODEC_ID_ADPCM_EA_R1) {
> > + current_left_sample = (int16_t)bytestream_get_le16(&srcC);
> > + previous_left_sample = (int16_t)bytestream_get_le16(&srcC);
> > + } else {
> > + current_left_sample = c->status[channel].predictor;
> > + previous_left_sample = c->status[channel].prev_sample;
> > + }
> > +
> > + for (count1=0; count1<samples_in_chunk; count1++) {
> > + if (*srcC == 0xEE) { /* only seen in R2 and R3 */
> > + srcC++;
> > + current_left_sample = (int16_t)bytestream_get_be16(&srcC);
> > + previous_left_sample = (int16_t)bytestream_get_be16(&srcC);
> > +
> > + for (count2=0; count2<28; count2++) {
> > + *samplesC = (int16_t)bytestream_get_be16(&srcC);
> > + samplesC += avctx->channels;
> > + }
>
> completely missing checks for array bounds, this should be exploitable
Right. Check added at the start of this decoding code.
> > + } else {
> > + coeff1l = ea_adpcm_table[ (*srcC>>4) & 0x0F ];
> > + coeff2l = ea_adpcm_table[((*srcC>>4) & 0x0F) + 4];
> > + shift_left = (*srcC++ & 0x0F) + 8;
> > +
> > + for (count2=0; count2<28; count2++) {
> > + if (count2 & 1)
> > + next_left_sample = ((*srcC++&0x0F)<<28)>>shift_left;
> > + else
> > + next_left_sample = ((*srcC &0xF0)<<24)>>shift_left;
>
> this code doesnt seem to be executed with just the left channel so why
> is it called left?
Well, it simply reused some vars already defined, but indeed, this don't
looks like a good idea. So I defined some new, channel agnostic, vars.
> > + next_left_sample += (current_left_sample * coeff1l) +
> > + (previous_left_sample * coeff2l);
> > + next_left_sample = av_clip_int16(next_left_sample >> 8);
> > +
> > + previous_left_sample = current_left_sample;
> > + current_left_sample = next_left_sample;
>
> > + *samplesC = (int16_t)current_left_sample;
>
> senseless cast
Right.
Updated patch attached.
Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ea_adpcm_r123.diff
Type: text/x-diff
Size: 6995 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071022/08cc5bba/attachment.diff>
More information about the ffmpeg-devel
mailing list