[Ffmpeg-devel] [RFC] Musepack decoding support

[Michael Niedermayer]

Hi

On Sat, Dec 23, 2006 at 07:39:44AM +0200, Kostya wrote:
> On Sat, Dec 23, 2006 at 12:27:37AM +0100, Michael Niedermayer wrote:
> > Hi
> > 
> > On Thu, Dec 21, 2006 at 08:12:16AM +0200, Kostya wrote:
> > > Here is updated decoder and demuxer.
> > > And again about seeking: now it just builds index as it goes,
> > 
> > [...]
> > > +    GetBitContext gb;
> > > +    uint8_t buf[8];
> > > +    float f1=1.20050805774840750476 * 256;
> > > +    static int vlc_inited = 0;
> > > +
> > > +    if(avctx->extradata_size < 8){
> > > +        av_log(avctx, AV_LOG_ERROR, "Too small extradata size (%i)!\n", avctx->extradata_size);
> > > +        return -1;
> > > +    }
> > > +    memset(c->oldDSCF, 0, sizeof(c->oldDSCF));
> > > +    c->rnd = 0xDEADBEEF;
> > > +    dsputil_init(&c->dsp, avctx);
> > > +    c->dsp.bswap_buf(buf, avctx->extradata, 2);
> > 
> > hmm doesnt the argument to bswap_buf() need to be aligned by more?
> 
> Looks like it does not: no comment in dsputil.h and I found only C version.
>  
> > 
> > > +    ff_mpa_synth_init(mpa_window);
> > > +    init_get_bits(&gb, buf, 64);
> > > +
> > > +    c->IS = get_bits1(&gb);
> > > +    c->MSS = get_bits1(&gb);
> > > +    c->bands = get_bits(&gb, 6);
> > > +    if(c->bands >= BANDS){
> > > +        av_log(avctx, AV_LOG_ERROR, "Too many bands: %i\n", c->bands);
> > > +        return -1;
> > > +    }
> > > +    skip_bits(&gb, 88);
> > > +    c->gapless = get_bits1(&gb);
> > > +    c->lastframelen = get_bits(&gb, 11);
> > 
> > something is wrong here, 1+1+6+88+1+11 > 64 ...
>  
> Right, forgot to update extradata size here
>  
> > [...]
> > > +/**
> > > + * Process decoded Musepack data and produce PCM
> > > + * TODO make it available for MPC8 and MPC6
> > 
> > nitpick: i think doxygen had a special tag for todo items maybe @todo ?
> 
> Done
> 
> > [...]
> > > +    /* get quantizers */
> > > +    memset(Q, 0, sizeof(Q));
> > > +    off = 0;
> > > +    for(i = 0; i < BANDS; i++, off += SAMPLES_PER_BAND)
> > > +        for(ch = 0; ch < 2; ch++)
> > > +            idx_to_quant(c, &gb, bands[i].res[ch], Q[ch] + off);
> > 
> > hmm, isnt Q[ch] always 0 here?
> 
> No, that's two-dimensional array.
> 
> > [...]
> > > +    t = get_le32(&s->pb);
> > > +    if((t & 0xFFFFFF) != MKTAG('M', 'P', '+', 0)){
> > > +        av_log(s, AV_LOG_ERROR, "Not a Musepack file\n");
> > > +        return -1;
> > > +    }
> > > +    c->ver = t >> 24;
> > 
> > if(get_le24(&s->pb) != MKTAG('M', 'P', '+', 0)){
> >     av_log(s, AV_LOG_ERROR, "Not a Musepack file\n");
> >     return -1;
> > }
> > c->ver = get_byte(&s->pb);
> 
> Done
> 
> > [...]
> > > +    get_buffer(&s->pb, buf, 16);
> > > +    t = LE_32(buf);
> > > +    samplerate = mpc_rate[(t >> 16) & 3];
> > > +
> > > +    c->curbits = 8;
> > > +
> > > +    st = av_new_stream(s, 0);
> > > +    av_set_pts_info(st, 32, MPC_FRAMESIZE, samplerate);
> > > +    if (!st)
> > > +        return AVERROR_NOMEM;
> > > +    st->codec->codec_type = CODEC_TYPE_AUDIO;
> > > +    st->codec->codec_id = CODEC_ID_MUSEPACK7;
> > > +    st->codec->channels = 2;
> > > +    st->codec->sample_rate = samplerate;
> > > +    st->codec->bits_per_sample = 16;
> > > +
> > > +    st->codec->extradata_size = 16;
> > > +    st->codec->extradata = av_mallocz(st->codec->extradata_size+FF_INPUT_BUFFER_PADDING_SIZE);
> > > +    memcpy(st->codec->extradata, buf, 16);
> > 
> > c= st->codec;
> > c->extradata_size = 16;
> > c->extradata = av_mallocz(c->extradata_size+FF_INPUT_BUFFER_PADDING_SIZE);
> > get_buffer(&s->pb, c->extradata, 16);
> > c->sample_rate = mpc_rate[c->extradata[2] & 3];
> 
> Done
> 
> > [...]
> > > +    if (av_new_packet(pkt, size) < 0)
> > > +        return AVERROR_IO;
> > > +
> > > +    pkt->data[0] = curbits;
> > > +    pkt->data[1] = (c->curframe > c->fcount);
> > > +    pkt->data[2] = seeked;
> > 
> > uhm, this is ugly ...
> > 
> > to detect seeking in the decoder theres AVCodec.flush() which must be
> > called by the application when seeking ...
> > the other 2 are ugly too but i dunno how to avoid them ...
> 
> Dropped (pkt->data[2] was not used anyway).

patch looks ok

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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061223/e063b87d/attachment.pgp>