[Ffmpeg-devel] [RFC] Musepack decoding support
Michael Niedermayer
michaelni
Wed Dec 20 01:46:28 CET 2006
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
> + int rnd;
> + /* for synthesis */
> + DECLARE_ALIGNED_16(MPA_INT, synth_buf[MPA_MAX_CHANNELS][512*2]);
> + int synth_buf_offset[MPA_MAX_CHANNELS];
> + DECLARE_ALIGNED_16(int32_t, sb_samples[MPA_MAX_CHANNELS][36][SBLIMIT]);
> +} MPCContext;
> +
> +/** Subband structure - hold all variables for each subband */
> +typedef struct {
> + int msf; ///< mid-stereo flag
> + int resR, resL;
> + int scfiR, scfiL;
> + int scf_idxR[3], scf_idxL[3];
> + int QR, QL;
more R/L vs [2]
[...]
> +// 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
[...]
> + if(bands[i].resL){
> + bands[i].scf_idxL[2] = c->oldDSCFL[i];
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxL[0] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxL[2] + t);
> + switch(bands[i].scfiL){
> + case 0:
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxL[1] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxL[0] + t);
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxL[2] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxL[1] + t);
> + break;
> + case 1:
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxL[1] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxL[0] + t);
> + bands[i].scf_idxL[2] = bands[i].scf_idxL[1];
> + break;
> + case 2:
> + bands[i].scf_idxL[1] = bands[i].scf_idxL[0];
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxL[2] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxL[1] + t);
> + break;
> + case 3:
> + bands[i].scf_idxL[2] = bands[i].scf_idxL[1] = bands[i].scf_idxL[0];
> + break;
> + }
> + c->oldDSCFL[i] = bands[i].scf_idxL[2];
> + }
> + if(bands[i].resR){
> + bands[i].scf_idxR[2] = c->oldDSCFR[i];
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxR[0] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxR[2] + t);
> + switch(bands[i].scfiR){
> + case 0:
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxR[1] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxR[0] + t);
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxR[2] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxR[1] + t);
> + break;
> + case 1:
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxR[1] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxR[0] + t);
> + bands[i].scf_idxR[2] = bands[i].scf_idxR[1];
> + break;
> + case 2:
> + bands[i].scf_idxR[1] = bands[i].scf_idxR[0];
> + t = get_vlc2(&gb, dscf_vlc.table, MPC7_DSCF_BITS, 1) - 7;
> + bands[i].scf_idxR[2] = (t == 8) ? get_bits(&gb, 6) : (bands[i].scf_idxR[1] + t);
> + break;
> + case 3:
> + bands[i].scf_idxR[2] = bands[i].scf_idxR[1] = bands[i].scf_idxR[0];
> + break;
> + }
> + c->oldDSCFR[i] = bands[i].scf_idxR[2];
> + }
this looks duplicated
[...]
> + /* 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) ?
> + off = 0;
> + for(i = 0; i <= mb; i++, off += SAMPLES_PER_BAND){
> + if(bands[i].resL){
> + j = 0;
> + mul = mpc_CC[bands[i].resL] * mpc7_SCF[bands[i].scf_idxL[0]];
> + for(; j < 12; j++)
> + c->sb_samples[0][j][i] = mul * LQ[j + off];
> + mul = mpc_CC[bands[i].resL] * mpc7_SCF[bands[i].scf_idxL[1]];
> + for(; j < 24; j++)
> + c->sb_samples[0][j][i] = mul * LQ[j + off];
> + mul = mpc_CC[bands[i].resL] * mpc7_SCF[bands[i].scf_idxL[2]];
> + for(; j < 36; j++)
> + c->sb_samples[0][j][i] = mul * LQ[j + off];
> + }
> + if(bands[i].resR){
> + j = 0;
> + mul = mpc_CC[bands[i].resR] * mpc7_SCF[bands[i].scf_idxR[0]];
> + for(; j < 12; j++)
> + c->sb_samples[1][j][i] = mul * RQ[j + off];
> + mul = mpc_CC[bands[i].resR] * mpc7_SCF[bands[i].scf_idxR[1]];
> + for(; j < 24; j++)
> + c->sb_samples[1][j][i] = mul * RQ[j + off];
> + mul = mpc_CC[bands[i].resR] * mpc7_SCF[bands[i].scf_idxR[2]];
> + for(; j < 36; j++)
> + c->sb_samples[1][j][i] = mul * RQ[j + off];
> + }
another R/L duplication
> + if(bands[i].msf){
> + int32_t t1, t2;
why not int?
> + for(j = 0; j < SAMPLES_PER_BAND; j++){
> + t1 = c->sb_samples[0][j][i];
> + t2 = c->sb_samples[1][j][i];
> + c->sb_samples[0][j][i] = t1 + t2;
> + c->sb_samples[1][j][i] = t1 - t2;
> + }
> + }
[...]
> +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()
[...]
> + c->streamed = url_is_streamed(&s->pb);
why not use url_is_streamed() directly, it would be more readable?
[...]
> + 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?
[...]
> + 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()?
> + 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
> + /* scan for seekpoints */
> + if(!c->streamed){
> + int cur = 0, curbits = 8;
> + int size, size2;
> + int64_t tmp, pos;
> +
> + for(cur = 0; cur < c->fcount; cur++){
> + pos = url_ftell(&s->pb);
> + tmp = get_le32(&s->pb);
> + if(curbits <= 12){
> + size2 = (tmp >> (12 - curbits)) & 0xFFFFF;
> + }else{
> + tmp = (tmp << 32) | get_le32(&s->pb);
> + size2 = (tmp >> (44 - curbits)) & 0xFFFFF;
> + }
> + curbits += 20;
> + url_fseek(&s->pb, pos, SEEK_SET);
> +
> + size = ((size2 + curbits + 31) & ~31) >> 3;
> + c->frames[cur].pos = pos;
> + c->frames[cur].size = size;
> + c->frames[cur].skip = curbits - 20;
> + curbits = (curbits + size2) & 0x1F;
> + url_fskip(&s->pb, size - (curbits ? 4 : 0));
> + av_add_index_entry(st, cur, cur, size, 0, AVINDEX_KEYFRAME);
> +// av_log(s, AV_LOG_DEBUG, "Adding frame - %lli+%i, size %i\n", c->frames[cur].pos, c->frames[cur].skip, c->frames[cur].size);
> + }
> + }
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
[...]
> + if((c->ver & 0xF) == 7){
what is this check good for, read_header() should have already failed if
it wherent true
[...]
> + 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
More information about the ffmpeg-devel
mailing list