[Ffmpeg-devel] [RFC] Musepack decoding support

[Kostya]

On Wed, Dec 20, 2006 at 01:46:28AM +0100, Michael Niedermayer wrote:
> Hi
> 
> On Mon, Dec 18, 2006 at 07:25:08AM +0200, Kostya wrote:
> > Here is Musepack SV7 decoder. Now it should work fine (except maybe the last frame).
> > Please test how it works (seeking should work too).
> > Also a small note: some Musepack files may fail to detect - they have ID3 tag before
> > the beginning of MPC stream.
> 
> [...]
> 
> > +static int vlc_inited = 0;
> 
> this one could be moved into mpc7_decode_init()
> 
> 
> > +
> > +static DECLARE_ALIGNED_16(MPA_INT, mpa_window[512]);
> > +
> > +typedef struct {
> > +    DSPContext dsp;
> > +    int IS, MSS, gapless;
> > +    int lastframelen, bands;
> > +    int oldDSCFR[BANDS], oldDSCFL[BANDS];
> 
> oldDSCF[2][BANDS] should allow quite a bit of code simplification

Yes, it really should be done as array (so a lot of code duplication goes away)
[...]

> > +// FIXME use standard lavc random generator
> 
> yes, iam definitly in favor of that, but we dont have one yet IIRC
> maybe you/someone could look at the google ffmpeg soc stuff, there was
> one or just write your own mersene twister/LFG based one


[...] 
> [...]
> > +    /* dequantize */
> > +    for(i = 0; i < SAMPLES_PER_BAND; i++)
> > +        for(j = 0; j < BANDS; j++)
> > +            c->sb_samples[0][i][j] = c->sb_samples[1][i][j] = 0;
> 
> memset(0) ?

Yeah, it should be.

[...]
> > +        if(bands[i].msf){
> > +            int32_t t1, t2;
> 
> why not int?

just the same type as sb_samples

> > +static int mpc_read_header(AVFormatContext *s, AVFormatParameters *ap)
> > +{
> > +    MPCContext *c = s->priv_data;
> > +    AVStream *st;
> > +    int samplerate;
> > +    uint32_t t;
> > +    uint8_t buf[8];
> > +
> > +    t = get_le32(&s->pb);
> > +    if(((t & 0xFF) != 'M') || (((t>>8)& 0xFF) != 'P') || (((t>>16)& 0xFF) != '+')){
> 
> maybe this can be simplified with ff_get_fourcc() or MK(BE)TAG()

I'll look at it

> 
> [...]
> 
> > +    c->streamed = url_is_streamed(&s->pb);
> 
> why not use url_is_streamed() directly, it would be more readable?

I use it for indexless reading and it may be also toggled later.
 
> [...]
> > +    get_buffer(&s->pb, buf, 4);
> > +    t = LE_32(buf);
> > +    samplerate = mpc_rate[(t >> 16) & 3];
> > +    get_le32(&s->pb);
> > +    get_le32(&s->pb);
> > +    get_buffer(&s->pb, buf + 4, 4);
> 
> hmm thats a slightly odd way to read extradata, whats in the 2 32bit values
> which are skiped? and why are they skiped?

Different peak values. 

> [...]
> > +    st->codec->extradata_size = 8;
> > +    if(st->codec->extradata_size+FF_INPUT_BUFFER_PADDING_SIZE <= (unsigned)st->codec->extradata_size){
> > +        //this check is redundant as get_buffer should fail
> 
> maybe change it to a assert()?

Better drop it.

> 
> > +        av_log(s, AV_LOG_ERROR, "extradata_size too large\n");
> > +        return -1;
> > +    }
> > +    st->codec->extradata = av_mallocz(st->codec->extradata_size+FF_INPUT_BUFFER_PADDING_SIZE);
> > +    memcpy(st->codec->extradata, buf, 8);
> 
> you could alloc extradata and then directly read into it avoiding the memcpy

Yes 
 
[...]
> uhm, reading through the whole file unconditionally during header parsing
> is not ok, also this should be done in a generic way (like storing the
> needed data in mpc_read_packet() and useing something like
> av_build_index_raw() if the user wants

It's not that easy - frames make continious bitstream and mostly they won't end at byte
boundaries (and bitstream is packed into 32-bit LE words read MSB)

What about giving error if url_is_streamed()!=0 (without those seeks the code will be a bit more
complicated - we'll need to store additional 4 bytes of data for each frame).
and index building may be skipped with flag AVFMT_FLAG_IGNIDX?
 
> [...]
> > +    if((c->ver & 0xF) == 7){
> 
> what is this check good for, read_header() should have already failed if
> it wherent true

Just thought to use it for upcoming SV8 but looks like they adopted NUT-like format
so I'll drop this check.

> [...]
> > +        if(c->curbits)
> > +            url_fseek(&s->pb, -4, SEEK_CUR);
> 
> this is not guranteed to succeed in non seekable streams
> 
> 
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> No snowflake in an avalanche ever feels responsible. -- Voltaire
>