[FFmpeg-soc] QCELP review
Michael Niedermayer
michaelni at gmx.at
Tue Aug 28 16:13:09 CEST 2007
Hi
a little late, but heres my qcelp review
qcelpdata.h:
[...]
> /**
> * @file qcelpdata.h
> * QCELP decoder
> * @author Reynaldo H. Verdejo Pinochet
> */
>
> #define QCELP_FXQ(v) (roundf(16384.0*v)/16384.0)
unused
[...]
> typedef struct {
> int index; /*!< index into the reference frame */
> int bitpos; /*!< bit position in the value's byte */
> } QCELPBitmap;
both fit in uint8_t
>
>
> /**
> * WARNING
> *
> * YOU WONT SEE ANY mention of a REFERENCE nor an UNIVERSAL frame
> * in the specs, this is just some internal way of handling the
> * reordering needed to unify the decoding process _inside_ this
> * code, nothing more.
> *
> *
> * UNIVERSAL FRAME
> * ---------------
> *
> * Format of QCELPFrame.data
> *
> * QCELP_X0_POS
> * |
> * CBSIGNs 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> * CBGAINs 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> * CINDEXs 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
> * PLAGs 48 49 50 51
> * PFRACs 52 53 54 55
> * PGAINs 56 57 58 59
> * LSPVs 60 61 62 63 64
> * RSVD 65
> * LSP 66 67 68 69 70 71 72 73 74 75
> * CBSEED 76
> *
> *
> * REFERENCE FRAME
> * ---------------
> *
> *
> * What follows are the reference frame slices. Each tuple will be mapped
> * to a QCELPBitmap showing the location of each bit in the input with respect
> * to a transmission code in the 'universal frame'.
> *---------------------------------------------------------------------------*/
>
>
> #define QCELP_RATE_FULL_BITMAP \
> {62,2},{62,1},{62,0},{61,6},{61,5},{61,4},{61,3},{61,2},\
> {61,1},{61,0},{60,5},{60,4},{60,3},{60,2},{60,1},{60,0},\
> {64,5},{64,4},{64,3},{64,2},{64,1},{64,0},{63,5},{63,4},\
> {63,3},{63,2},{63,1},{63,0},{62,6},{62,5},{62,4},{62,3},\
> { 0,0},{16,3},{16,2},{16,1},{16,0},{52,0},{48,6},{48,5},\
> {48,4},{48,3},{48,2},{48,1},{48,0},{56,2},{56,1},{56,0},\
> {33,3},{33,2},{33,1},{33,0},{ 1,0},{17,3},{17,2},{17,1},\
> {17,0},{32,6},{32,5},{32,4},{32,3},{32,2},{32,1},{32,0},\
> {19,0},{34,6},{34,5},{34,4},{34,3},{34,2},{34,1},{34,0},\
> { 2,0},{18,3},{18,2},{18,1},{18,0},{33,6},{33,5},{33,4},\
> {49,2},{49,1},{49,0},{57,2},{57,1},{57,0},{35,6},{35,5},\
> {35,4},{35,3},{35,2},{35,1},{35,0},{ 3,0},{19,2},{19,1},\
> {36,5},{36,4},{36,3},{36,2},{36,1},{36,0},{ 4,0},{20,3},\
> {20,2},{20,1},{20,0},{53,0},{49,6},{49,5},{49,4},{49,3},\
> {22,2},{22,1},{22,0},{37,6},{37,5},{37,4},{37,3},{37,2},\
> {37,1},{37,0},{ 5,0},{21,3},{21,2},{21,1},{21,0},{36,6},\
> {39,2},{39,1},{39,0},{ 7,0},{23,2},{23,1},{23,0},{38,6},\
> {38,5},{38,4},{38,3},{38,2},{38,1},{38,0},{ 6,0},{22,3},\
> {24,0},{54,0},{50,6},{50,5},{50,4},{50,3},{50,2},{50,1},\
> {50,0},{58,2},{58,1},{58,0},{39,6},{39,5},{39,4},{39,3},\
> { 9,0},{25,3},{25,2},{25,1},{25,0},{40,6},{40,5},{40,4},\
> {40,3},{40,2},{40,1},{40,0},{ 8,0},{24,3},{24,2},{24,1},\
> {42,3},{42,2},{42,1},{42,0},{10,0},{26,3},{26,2},{26,1},\
> {26,0},{41,6},{41,5},{41,4},{41,3},{41,2},{41,1},{41,0},\
> {59,1},{59,0},{43,6},{43,5},{43,4},{43,3},{43,2},{43,1},\
> {43,0},{11,0},{27,2},{27,1},{27,0},{42,6},{42,5},{42,4},\
> {44,1},{44,0},{12,0},{28,3},{28,2},{28,1},{28,0},{55,0},\
> {51,6},{51,5},{51,4},{51,3},{51,2},{51,1},{51,0},{59,2},\
> {45,5},{45,4},{45,3},{45,2},{45,1},{45,0},{13,0},{29,3},\
> {29,2},{29,1},{29,0},{44,6},{44,5},{44,4},{44,3},{44,2},\
> {31,2},{31,1},{31,0},{46,6},{46,5},{46,4},{46,3},{46,2},\
> {46,1},{46,0},{14,0},{30,3},{30,2},{30,1},{30,0},{45,6},\
> {65,1},{65,0},{47,6},{47,5},{47,4},{47,3},{47,2},{47,1},\
> {47,0},{15,0}
>
> #define QCELP_RATE_HALF_BITMAP \
> {62,2},{62,1},{62,0},{61,6},{61,5},{61,4},{61,3},{61,2},\
> {61,1},{61,0},{60,5},{60,4},{60,3},{60,2},{60,1},{60,0},\
> {64,5},{64,4},{64,3},{64,2},{64,1},{64,0},{63,5},{63,4},\
> {63,3},{63,2},{63,1},{63,0},{62,6},{62,5},{62,4},{62,3},\
> { 0,0},{16,3},{16,2},{16,1},{16,0},{52,1},{48,6},{48,5},\
> {48,4},{48,3},{48,2},{48,1},{48,0},{56,2},{56,1},{56,0},\
> {49,5},{49,4},{49,3},{49,2},{49,1},{49,0},{57,2},{57,1},\
> {57,0},{32,6},{32,5},{32,4},{32,3},{32,2},{32,1},{32,0},\
> {58,1},{58,0},{33,6},{33,5},{33,4},{33,3},{33,2},{33,1},\
> {33,0},{ 1,0},{17,3},{17,2},{17,1},{17,0},{53,0},{49,6},\
> {34,1},{34,0},{ 2,0},{18,3},{18,2},{18,1},{18,0},{54,0},\
> {50,6},{50,5},{50,4},{50,3},{50,2},{50,1},{50,0},{58,2},\
> {55,0},{51,6},{51,5},{51,4},{51,3},{51,2},{51,1},{51,0},\
> {59,2},{59,1},{59,0},{34,6},{34,5},{34,4},{34,3},{34,2},\
> {35,6},{35,5},{35,4},{35,3},{35,2},{35,1},{35,0},{ 3,0},\
> {19,3},{19,2},{19,1},{19,0}
>
> #define QCELP_RATE_4THR_BITMAP \
> {62,2},{62,1},{62,0},{61,6},{61,5},{61,4},{61,3},{61,2},\
> {61,1},{61,0},{60,5},{60,4},{60,3},{60,2},{60,1},{60,0},\
> {64,5},{64,4},{64,3},{64,2},{64,1},{64,0},{63,5},{63,4},\
> {63,3},{63,2},{63,1},{63,0},{62,6},{62,5},{62,4},{62,3},\
> {19,3},{19,2},{19,1},{19,0},{18,3},{18,2},{18,1},{18,0},\
> {17,3},{17,2},{17,1},{17,0},{16,3},{16,2},{16,1},{16,0},\
> {65,1},{65,0},{20,3},{20,2},{20,1},{20,0}
>
> #define QCELP_RATE_8THR_BITMAP \
> {65,0},{65,1},{65,2},{65,3},{16,0},{16,1},{75,0},{76,0},\
> {74,0},{73,0},{72,0},{76,1},{71,0},{70,0},{69,0},{76,2},\
> {68,0},{67,0},{66,0},{76,3}
why not store the data in a normal sane struct and make the
above tables contain byte/bit indexes into the struct?
that would make the code accessing it look cleaner ...
also the above tables would be cleaner as they would use
the names of the fields not meaningless numbers
[...]
> static const qcelp_fvector qcelp_lspvq1[]={
> {.0327,.0118},{.0919,.0111},{.0427,.0440},{.1327,.0185},
> {.0469,.0050},{.1272,.0091},{.0892,.0059},{.1771,.0193},
> {.0222,.0158},{.1100,.0127},{.0827,.0055},{.0978,.0791},
> {.0665,.0047},{.0700,.1401},{.0670,.0859},{.1913,.1048},
> {.0471,.0215},{.1046,.0125},{.0645,.0298},{.1599,.0160},
> {.0593,.0039},{.1187,.0462},{.0749,.0341},{.1520,.0511},
> {.0290,.0792},{.0909,.0362},{.0753,.0081},{.1111,.1058},
> {.0519,.0253},{.0828,.0839},{.0685,.0541},{.1421,.1258},
> {.0386,.0130},{.0962,.0119},{.0542,.0387},{.1431,.0185},
> {.0526,.0051},{.1175,.0260},{.0831,.0167},{.1728,.0510},
> {.0273,.0437},{.1172,.0113},{.0771,.0144},{.1122,.0751},
> {.0619,.0119},{.0492,.1276},{.0658,.0695},{.1882,.0615},
> {.0415,.0200},{.1018,.0088},{.0681,.0339},{.1436,.0325},
> {.0555,.0122},{.1042,.0485},{.0826,.0345},{.1374,.0743},
> {.0383,.1018},{.1005,.0358},{.0704,.0086},{.1301,.0586},
> {.0597,.0241},{.0832,.0621},{.0555,.0573},{.1504,.0839}};
>
> static const qcelp_fvector qcelp_lspvq2[]={
> {.0255,.0293},{.0904,.0219},{.0151,.1211},{.1447,.0498},
> {.0470,.0253},{.1559,.0177},{.1547,.0994},{.2394,.0242},
> {.0091,.0813},{.0857,.0590},{.0934,.1326},{.1889,.0282},
> {.0813,.0472},{.1057,.1494},{.0450,.3315},{.2163,.1895},
> {.0538,.0532},{.1399,.0218},{.0146,.1552},{.1755,.0626},
> {.0822,.0202},{.1299,.0663},{.0706,.1732},{.2656,.0401},
> {.0418,.0745},{.0762,.1038},{.0583,.1748},{.1746,.1285},
> {.0527,.1169},{.1314,.0830},{.0556,.2116},{.1073,.2321},
> {.0297,.0570},{.0981,.0403},{.0468,.1103},{.1740,.0243},
> {.0725,.0179},{.1255,.0474},{.1374,.1362},{.1922,.0912},
> {.0285,.0947},{.0930,.0700},{.0593,.1372},{.1909,.0576},
> {.0588,.0916},{.1110,.1116},{.0224,.2719},{.1633,.2220},
> {.0402,.0520},{.1061,.0448},{.0402,.1352},{.1499,.0775},
> {.0664,.0589},{.1081,.0727},{.0801,.2206},{.2165,.1157},
> {.0566,.0802},{.0911,.1116},{.0306,.1703},{.1792,.0836},
> {.0655,.0999},{.1061,.1038},{.0298,.2089},{.1110,.1753},
> {.0361,.0311},{.0970,.0239},{.0265,.1231},{.1495,.0573},
> {.0566,.0262},{.1569,.0293},{.1341,.1144},{.2271,.0544},
> {.0214,.0877},{.0847,.0719},{.0794,.1384},{.2067,.0274},
> {.0703,.0688},{.1099,.1306},{.0391,.2947},{.2024,.1670},
> {.0471,.0525},{.1245,.0290},{.0264,.1557},{.1568,.0807},
> {.0718,.0399},{.1193,.0685},{.0883,.1594},{.2729,.0764},
> {.0500,.0754},{.0809,.1108},{.0541,.1648},{.1523,.1385},
> {.0614,.1196},{.1209,.0847},{.0345,.2242},{.1442,.1747},
> {.0199,.0560},{.1092,.0194},{.0349,.1253},{.1653,.0507},
> {.0625,.0354},{.1376,.0431},{.1187,.1465},{.2164,.0872},
> {.0360,.0974},{.1008,.0698},{.0704,.1346},{.2114,.0452},
> {.0720,.0816},{.1240,.1089},{.0439,.2475},{.1498,.2040},
> {.0336,.0718},{.1213,.0187},{.0451,.1450},{.1368,.0885},
> {.0592,.0578},{.1131,.0531},{.0861,.1855},{.1764,.1500},
> {.0444,.0970},{.0935,.0903},{.0424,.1687},{.1633,.1102},
> {.0793,.0897},{.1060,.0897},{.0185,.2011},{.1205,.1855}};
>
> static const qcelp_fvector qcelp_lspvq3[]={
> {.0225,.0283},{.1296,.0355},{.0543,.0343},{.2073,.0274},
> {.0204,.1099},{.1562,.0523},{.1388,.0161},{.2784,.0274},
> {.0112,.0849},{.1870,.0175},{.1189,.0160},{.1490,.1088},
> {.0969,.1115},{.0659,.3322},{.1158,.1073},{.3183,.1363},
> {.0517,.0223},{.1740,.0223},{.0704,.0387},{.2637,.0234},
> {.0692,.1005},{.1287,.1610},{.0952,.0532},{.2393,.0646},
> {.0490,.0552},{.1619,.0657},{.0845,.0670},{.1784,.2280},
> {.0191,.1775},{.0272,.2868},{.0942,.0952},{.2628,.1479},
> {.0278,.0579},{.1565,.0218},{.0814,.0180},{.2379,.0187},
> {.0276,.1444},{.1199,.1223},{.1200,.0349},{.3009,.0307},
> {.0312,.0844},{.1898,.0306},{.0863,.0470},{.1685,.1241},
> {.0513,.1727},{.0711,.2233},{.1085,.0864},{.3398,.0527},
> {.0414,.0440},{.1356,.0612},{.0964,.0147},{.2173,.0738},
> {.0465,.1292},{.0877,.1749},{.1104,.0689},{.2105,.1311},
> {.0580,.0864},{.1895,.0752},{.0652,.0609},{.1485,.1699},
> {.0514,.1400},{.0386,.2131},{.0933,.0798},{.2473,.0986},
> {.0334,.0360},{.1375,.0398},{.0621,.0276},{.2183,.0280},
> {.0311,.1114},{.1382,.0807},{.1284,.0175},{.2605,.0636},
> {.0230,.0816},{.1739,.0408},{.1074,.0176},{.1619,.1120},
> {.0784,.1371},{.0448,.3050},{.1189,.0880},{.3039,.1165},
> {.0424,.0241},{.1672,.0186},{.0815,.0333},{.2432,.0324},
> {.0584,.1029},{.1137,.1546},{.1015,.0585},{.2198,.0995},
> {.0574,.0581},{.1746,.0647},{.0733,.0740},{.1938,.1737},
> {.0347,.1710},{.0373,.2429},{.0787,.1061},{.2439,.1438},
> {.0185,.0536},{.1489,.0178},{.0703,.0216},{.2178,.0487},
> {.0154,.1421},{.1414,.0994},{.1103,.0352},{.3072,.0473},
> {.0408,.0819},{.2055,.0168},{.0998,.0354},{.1917,.1140},
> {.0665,.1799},{.0993,.2213},{.1234,.0631},{.3003,.0762},
> {.0373,.0620},{.1518,.0425},{.0913,.0300},{.1966,.0836},
> {.0402,.1185},{.0948,.1385},{.1121,.0555},{.1802,.1509},
> {.0474,.0886},{.1888,.0610},{.0739,.0585},{.1231,.2379},
> {.0661,.1335},{.0205,.2211},{.0823,.0822},{.2480,.1179}};
>
> static const qcelp_fvector qcelp_lspvq4[]={
> {.0348,.0311},{.0812,.1145},{.0552,.0461},{.1826,.0263},
> {.0601,.0675},{.1730,.0172},{.1523,.0193},{.2449,.0277},
> {.0334,.0668},{.0805,.1441},{.1319,.0207},{.1684,.0910},
> {.0582,.1318},{.1403,.1098},{.0979,.0832},{.2700,.1359},
> {.0624,.0228},{.1292,.0979},{.0800,.0195},{.2226,.0285},
> {.0730,.0862},{.1537,.0601},{.1115,.0509},{.2720,.0354},
> {.0218,.1167},{.1212,.1538},{.1074,.0247},{.1674,.1710},
> {.0322,.2142},{.1263,.0777},{.0981,.0556},{.2119,.1710},
> {.0193,.0596},{.1035,.0957},{.0694,.0397},{.1997,.0253},
> {.0743,.0603},{.1584,.0321},{.1346,.0346},{.2221,.0708},
> {.0451,.0732},{.1040,.1415},{.1184,.0230},{.1853,.0919},
> {.0310,.1661},{.1625,.0706},{.0856,.0843},{.2902,.0702},
> {.0467,.0348},{.1108,.1048},{.0859,.0306},{.1964,.0463},
> {.0560,.1013},{.1425,.0533},{.1142,.0634},{.2391,.0879},
> {.0397,.1084},{.1345,.1700},{.0976,.0248},{.1887,.1189},
> {.0644,.2087},{.1262,.0603},{.0877,.0550},{.2203,.1307}};
>
> static const qcelp_fvector qcelp_lspvq5[]={
> {.0360,.0222},{.0820,.1097},{.0601,.0319},{.1656,.0198},
> {.0604,.0513},{.1552,.0141},{.1391,.0155},{.2474,.0261},
> {.0269,.0785},{.1463,.0646},{.1123,.0191},{.2015,.0223},
> {.0785,.0844},{.1202,.1011},{.0980,.0807},{.3014,.0793},
> {.0570,.0180},{.1135,.1382},{.0778,.0256},{.1901,.0179},
> {.0807,.0622},{.1461,.0458},{.1231,.0178},{.2028,.0821},
> {.0387,.0927},{.1496,.1004},{.0888,.0392},{.2246,.0341},
> {.0295,.1462},{.1156,.0694},{.1022,.0473},{.2226,.1364},
> {.0210,.0478},{.1029,.1020},{.0722,.0181},{.1730,.0251},
> {.0730,.0488},{.1465,.0293},{.1303,.0326},{.2595,.0387},
> {.0458,.0584},{.1569,.0742},{.1029,.0173},{.1910,.0495},
> {.0605,.1159},{.1268,.0719},{.0973,.0646},{.2872,.0428},
> {.0443,.0334},{.0835,.1465},{.0912,.0138},{.1716,.0442},
> {.0620,.0778},{.1316,.0450},{.1186,.0335},{.1446,.1665},
> {.0486,.1050},{.1675,.1019},{.0880,.0278},{.2214,.0202},
> {.0539,.1564},{.1142,.0533},{.0984,.0391},{.2130,.1089}};
these can be stored as shorts (multiplied by 10000)
(yes iam trying to reduce the amount of used floats ...)
[...]
> static const float qcelp_g12ga[]={
> 1.000, 1.125, 1.250, 1.375, 1.625, 1.750, 2.000, 2.250,
> 2.500, 2.875, 3.125, 3.500, 4.000, 4.500, 5.000, 5.625,
> 6.250, 7.125, 8.000, 8.875, 10.000, 11.250, 12.625, 14.125,
> 15.875, 17.750, 20.000, 22.375, 25.125, 28.125, 31.625, 35.500,
> 39.750, 44.625, 50.125, 56.250, 63.125, 70.750, 79.375, 89.125,
> 100.000, 112.250, 125.875, 141.250, 158.500,177.875,199.500,223.875,
> 251.250, 281.875, 316.250, 354.875, 398.125,446.625,501.125,563.375,
> 631.000, 708.000, 794.375, 891.250,1000.000};
all these values are multiplies of 0.125 so they could easily be stored
as 10.3 fixed point values
>
> static const int qcelp_cumulative_gainloss[]={0,1,2,6};
> static const float qcelp_cumulative_pitchsaturation[]={0.9,0.6,0.3,0.0};
unused
[...]
qcelpdec.c:
[...]
> typedef struct
> {
> qcelp_packet_rate rate;
> uint8_t data[76]; /*!< data from a _parsed_ frame */
> int bits;
> } QCELPFrame;
>
> typedef struct {
> GetBitContext gb;
> QCELPFrame *frame;
why 2 structs? it doesnt seem like there ever is a QCELPFrame anywhere
except the single one in QCELPContext
also the { placement is inconsistant
[...]
>
> static float qcelp_hammsinc(float i)
> {
> return (sin(M_PI*i)/(M_PI*i))*(0.5+0.46*cos(M_PI*i/4.0));
> }
the 8 values could be stored in a simple static const table
i doubt very much that the code used to generate the table
is smaller than 4*8 bytes which the table would need
>
> static void qcelp_update_pitchf_mem(float *pitchf_mem, float *last)
> {
> float tmp[150];
>
> memcpy(tmp, pitchf_mem+40, 110*sizeof(float));
> memcpy(tmp+110, last, 40*sizeof(float));
> memcpy(pitchf_mem, tmp, 150*sizeof(float));
> }
tmp is unneeded and as already said this should use memmove()
[...]
> q->frame_num=0;
>
> memset(q->prev_lspf , 0, sizeof(q->prev_lspf ));
> memset(q->pitchf_mem , 0, sizeof(q->pitchf_mem ));
> memset(q->pitchp_mem , 0, sizeof(q->pitchp_mem ));
> memset(q->formant_mem, 0, sizeof(q->formant_mem));
libavcodec memset(0) your context, so this shouldnt be needed
>
> /**
> * Fill hammsinc table
> */
>
> for(i=0; i<8; i++)
> q->hammsinc_table[i]=qcelp_hammsinc(i-3.5);
this doesnt need to be duplicated per decoder instance
[...]
> /**
> * Decodes the 10 quantized LSP frequencies from the LSPV/LSP
> * transsmision codes of any frame rate.
> *
> * TIA/EIA/IS-733 2.4.3.2.6.2-2
> */
> void qcelp_decode_lspf(const QCELPFrame *frame, float *lspf)
this should be static
> {
> const uint8_t *lspv;
> int i;
>
> switch(frame->rate)
> {
> case RATE_FULL:
> case RATE_HALF:
> case RATE_QUARTER:
> lspv=frame->data+QCELP_LSPV0_POS;
>
> lspf[0]= qcelp_lspvq1[lspv[0]].x;
> lspf[1]=lspf[0]+qcelp_lspvq1[lspv[0]].y;
> lspf[2]=lspf[1]+qcelp_lspvq2[lspv[1]].x;
> lspf[3]=lspf[2]+qcelp_lspvq2[lspv[1]].y;
> lspf[4]=lspf[3]+qcelp_lspvq3[lspv[2]].x;
> lspf[5]=lspf[4]+qcelp_lspvq3[lspv[2]].y;
> lspf[6]=lspf[5]+qcelp_lspvq4[lspv[3]].x;
> lspf[7]=lspf[6]+qcelp_lspvq4[lspv[3]].y;
> lspf[8]=lspf[7]+qcelp_lspvq5[lspv[4]].x;
> lspf[9]=lspf[8]+qcelp_lspvq5[lspv[4]].y;
if the 5 qcelp_lspvq* tables where split in x and y tables and there
were a table with 10 pointers to them
then we could just write:
lspf[0]= qcelp_lspvq[0][lspv[0]];
for(i=1; i<10; i++)
lspf[i]= lspf[i-1] + qcelp_lspvq[i][lspv[i>>1]];
also all this can be done with integers, no need for floats
if the table has been scaled up by 10000
>
> break;
> case RATE_OCTAVE:
> lspv=frame->data+QCELP_LSP0_POS;
> for(i=0; i<10; i++)
> {
> lspf[i]=lspv[i]? 0.02:-0.02; /* 2.4.3.3.1-1 */
> }
> }
if(frame->rate == RATE_OCTAVE){
}else{
}
[...]
> switch(frame->rate)
> {
> case RATE_FULL:
> case RATE_HALF:
> for(i=0; i<16; i++)
> {
> if(frame->rate == RATE_HALF && i>=4) break;
this check is not needed
>
> gs[i]=QCELP_CBSIGN2GS(cbsign[i]);
> g0[i]=QCELP_CBGAIN2G0(cbgain[i]);
is it really necessary to hide *4 behind a macro?
>
> /**
> * Spec has errors on the predictor determination formula
> * it needs 6 to be subtracted from it to give RI results.
> */
>
> if(frame->rate == RATE_FULL && i > 0 && !((i+1) & 3))
> predictor=QCELP_FIX_SPEC_PREDICTOR
> (av_clip(floor((g1[i-1]+g1[i-2]+g1[i-3])/3.0), 6,
> 38));
> else
> predictor=0;
>
> g1[i]=g0[i]+predictor;
>
> if(g1[i]<0 || g1[i]>60)
> {
how can it become <0 ?
[...]
> ga[i]=qcelp_g12ga[g1[i]];
>
> gain[i]=ga[i]*gs[i];
> index[i]=(gs[i] == 1)? cindex[i]:(cindex[i]-89) & 127;
gain[i]= qcelp_g12ga[g1[i]];
index[i] = cindex[i];
if(cbsign[i]){
index[i] = (index[i] - 89) & 127;
gain[i] *= -1;
}
this skips intalizing arrays which are never used and is IMHO more readable
> }
>
> break;
> case RATE_QUARTER:
> for(i=0; i<5; i++)
> {
> g0[i]=g1[i]=QCELP_CBGAIN2G0(cbgain[i]);
> gs[i]=1;
> ga[i]=qcelp_g12ga[g1[i]];
half of these are unused remove them please
[...]
> switch(rate)
> {
> case RATE_FULL:
>
> for(i=0; i<160; i++)
> {
> cdn_vector[i]=
> gain[i/10]*qcelp_fullrate_ccodebook[(j-index[i/10]) & 127];
>
> j=QCELP_FIX_SPEC_MISSING_CLAMP(j);
> }
> break;
> case RATE_HALF:
>
> for(i=0; i<160; i++)
> {
> cdn_vector[i]=
> gain[i/40]*qcelp_halfrate_ccodebook[(j-index[i/40]) & 127];
>
> j=QCELP_FIX_SPEC_MISSING_CLAMP(j);
my gut feeling says that the QCELP_FIX_SPEC_MISSING_CLAMP should use 39
instead of 9 here
and i hate these macros ... a simple i%(3)9 instead of j would do ...
> }
> break;
the RATE_FULL and RATE_HALF cases can be merged
> case RATE_QUARTER:
> for(i=0; i<160; i++)
> {
> new_cbseed=(521*cbseed+259) & 65535;
> cbseed=rnd[i]=
> QCELP_SQRT1887*(((new_cbseed+32768) & 65535)-32768)/32768.0;
this is not what is written in the spec, it also makes no sense, the
PRNG must be deterministic floats are not ...
>
> /* FIR filter */
>
> cdn_vector[i]=qcelp_rnd_fir_coefs[1]*rnd[i];
> for(j=1; j<22 && !(i-j+1); j++)
> {
> cdn_vector[i]+=qcelp_rnd_fir_coefs[j]*rnd[i-j];
> }
>
> /* final scaling */
>
> cdn_vector[i]*=gain[i/20];
> }
> break;
> case RATE_OCTAVE:
> for(i=0; i<160; i++)
> {
> new_cbseed=(521*cbseed+259) & 65535;
> cbseed=rnd[i]=
> QCELP_SQRT1887*(((new_cbseed+32768) & 65535)-32768)/32768.0;
>
> cdn_vector[i]=gain[0]*rnd[i];
> }
the 1/4 and 1/8 cases also look like they could be merged
[...]
> /**
> * Pitch filters or pre-filters pv, returns 0 if everything goes
> * well, otherwise it returns the index of the failing-to-be-pitched
> * element and -1 if an invalid (140.5, 141.5, 142.5) lag is found or
> * an invalid operation mode is requested.
> *
> * This function implements both, the pitch filter and the pitch pre-filter
> * whose results gets stored in pv.
> *
> * TIA/EIA/IS-733 2.4.5.2
> *
> * @param step Mode, 1 for pitch filter or 2 for pitch pre-filter
> */
> static int qcelp_do_pitchfilter(QCELPFrame *frame, float *pitch_mem, int step,
> float *pv, float *hammsinc_table)
> {
> int i, j, k, tmp;
> uint8_t *pgain, *plag, *pfrac;
> float gain[4], lag[4], hamm_tmp;
gain and lag here can and should be fixed point
>
> if(step != 1 && step != 2)
> return -1;
assert() as this cant happen unless this codec is buggy
[...]
> if(pfrac[i])
> lag[i]+=0.5;
>
> if(lag[i] == 140.5 || lag[i] == 141.5 || lag[i] == 142.5)
> return -1;
143.5 is missing, also this can be put as if(>140) under the if(pfrac[i])
which is the only place capable to cause .5
[...]
> if(pfrac[i/40]) /* if is a fractional lag... */
> {
> hamm_tmp=0.0;
>
> for(j=-4; j<4; j++)
> {
> tmp = k+j+0.5-lag[i/40];
>
> if(tmp < 0)
> hamm_tmp+=hammsinc_table[j+4]
> * pitch_mem[150+tmp];
> else
> hamm_tmp+=hammsinc_table[j+4]
> * pv [tmp];
> }
>
> pv[i]+=gain[i/40]*hamm_tmp;
>
> }else
> {
> tmp=k-lag[i/40];
>
> if(tmp < 0)
> pv[i]+=lrintf(gain[i/40]*pitch_mem[150+tmp]);
> else
> pv[i]+=lrintf(gain[i/40]*pv[i - lrintf(lag[i/40])]);
why lrintf here? why not in the hammsinc case?
[...]
> /**
> * Computes interpolated lsp frequencies for a given rate & pitch subframe
> *
> * TIA/EIA/IS-733 2.4.3.3.4
> *
> * @param rate Frame rate
> * @param prev_lspf Previous frame LSP freqs vector
> * @param curr_lspf Current frame LSP freqs vector
> * @param interpolated_lspf Float vector to put the resulting LSP freqs
> * @param frame_num Frame number in decoded stream
> */
> void qcelp_do_interpolate_lspf(qcelp_packet_rate rate, float *prev_lspf,
> float *curr_lspf, float *interpolated_lspf, int sample_num, int frame_num)
> {
> int i;
> float curr_weight, prev_weight;
>
> switch(rate)
> {
> case RATE_FULL:
> case RATE_HALF:
> case RATE_QUARTER:
>
> if(!frame_num)
> {
> curr_weight=1.0;
> prev_weight=0.0;
> }else
> {
> switch(sample_num)
> {
> case 0:
> curr_weight=0.25;
> prev_weight=0.75;
> break;
> case 40:
> curr_weight=0.5;
> prev_weight=0.5;
> break;
> case 80:
> curr_weight=0.75;
> prev_weight=0.25;
> break;
> case 120:
> curr_weight=1.0;
> prev_weight=0;
> }
i is multiplied by 40 then this swicth is done to initalize these ...
ugly
dont multiply by 40 its senseless
and then use
curr_weight= sample_num + 1
prev_weight= 4 - curr_weight
or the equivalent /4.0 floats
[...]
> /**
> * Linear convolution of two vectors, with max resultant
> * vector dim = 12 -- just what we need. Result gets stored
> * in v1 so it must have enough room to hold d1+d2-1.
> *
> * WIP this is a heavily suboptimal implementation
> *
> * @param d1 real dimension of v1 prior convolution
> * @param d2 dimension of v2
> */
> static void qcelp_convolve(float *v1, const float *v2, int d1, int d2)
> {
> float copy[12];
> int i,j,dim;
>
> memcpy(copy, v1, sizeof(copy));
> dim=d1+d2-1;
>
> for(i=0;i<dim;i++)
> {
> v1[i]=0.0;
> for(j=0;j<=i;j++)
> v1[i]+=(((i-j)>=d1 || (i-j)<0)? 0:copy[i-j])*(j>=d2? 0:v2[j]);
> }
>
> }
>
> /**
> * Computes the Pa and Qa coeficients needed at LSP to LPC conversion.
> *
> * TIA/EIA/IS-733 2.4.3.3.5-1/2
> */
> static void qcelp_lsp2poly(float *lspf, float *pa, float *qa)
> {
> int i;
> float v1[12];
> float v2[3];
> int limit[]={2,4,6,8,10};
>
> v2[0]=1;
> v2[2]=1;
>
> /**
> * Compute Pa coefs
> */
>
> v1[0]=1.0;
> v1[1]=1.0;
>
> for(i=0; i<5; i++)
> {
> v2[1]=-2*cos(M_PI*lspf[2*i]);
> qcelp_convolve(v1, v2, limit[i], 3);
> }
>
> for(i=0;i<5;i++)
> pa[i]=v1[i+1];
>
> /**
> * Compute Qa coefs
> */
>
> v1[0]= 1.0;
> v1[1]=-1.0;
>
> for(i=0; i<5; i++)
> {
> v2[1]=-2*cos(M_PI*lspf[2*i+1]);
> qcelp_convolve(v1, v2, limit[i], 3);
> }
>
> for(i=0;i<5;i++)
> qa[i]=v1[i+1];
>
> }
the above can be simplified, 2/3 of the multiplications done by
qcelp_convolve() are by 1 or -1
[...]
> /**
> * Formant sythesis filter
> *
> * TIA/EIA/IS-733 2.4.3.1 (NOOOOT)
> */
> static void qcelp_do_formant(float *in, float *out, float *lpc_coefs,
> float *memory)
> {
> float tmp[50];
> int i,j;
>
> /**
> * Copy over previous ten samples generated
> */
>
> memcpy(tmp, memory, 10*sizeof(float));
> memcpy(tmp+10, in, 40*sizeof(float));
>
> for(i=10;i<50;i++)
> {
> for(j=1;j<11;j++)
> {
> tmp[i]+=tmp[i-j]*lpc_coefs[j-1];
> }
> }
>
> /**
> * Update memory for next pitch subframe
> */
>
> memcpy(memory, tmp+40, 10*sizeof(float));
>
> /**
> * Write filtered samples to *out
> */
>
> memcpy(out, tmp+10, 40*sizeof(float));
> }
this can be done with fewer memcpy
>
> /**
> * Detilt used in the adaptive postfilter after the formant synthesis
> * filter.
> *
> * TIA/EIA/IS-733 2.4.8.6-2
> */
> static float qcelp_detilt(float z)
> {
> if(z)
> return (1.0/(1.0 + 0.3 / z));
> else
> return 0;
this is nonsense the detilt filter is a IIR filter not some f(scaler)
see http://en.wikipedia.org/wiki/Infinite_impulse_response
[...]
> switch(buf_size)
> {
> case 35:
> is_codecframe_fmt=1;
> case 34:
> q->frame->rate = RATE_FULL;
> q->frame->bits = qcelp_bits_per_rate[RATE_FULL];
> order = QCELP_REFERENCE_FRAME + QCELP_FULLPKT_REFERENCE_POS;
> break;
> case 17:
> is_codecframe_fmt=1;
> case 16:
> q->frame->rate = RATE_HALF;
> q->frame->bits = qcelp_bits_per_rate[RATE_HALF];
> order = QCELP_REFERENCE_FRAME + QCELP_HALFPKT_REFERENCE_POS;
> break;
> case 8:
> is_codecframe_fmt=1;
> case 7:
> q->frame->rate = RATE_QUARTER;
> q->frame->bits = qcelp_bits_per_rate[RATE_QUARTER];
> order = QCELP_REFERENCE_FRAME + QCELP_4THRPKT_REFERENCE_POS;
> break;
> case 4:
> is_codecframe_fmt=1;
> case 3:
> q->frame->rate = RATE_OCTAVE;
> q->frame->bits = qcelp_bits_per_rate[RATE_OCTAVE];
> order = QCELP_REFERENCE_FRAME + QCELP_8THRPKT_REFERENCE_POS;
> break;
> case 1:
> is_codecframe_fmt=1;
> case 0:
> q->frame->rate = BLANK;
> q->frame->bits = 0;
> order = NULL;
> break;
> default:
> av_log(avctx, AV_LOG_ERROR, "Error decoding frame"
> " -- Unknown framerate, unsupported size: %d\n",
> buf_size);
> return -1;
> }
q->frame->bits = qcelp_bits_per_rate[q->frame->rate];
after the switch ...
and the same can be done to simplify the order init
>
> if(is_codecframe_fmt)
> {
> claimed_rate=get_bits(&q->gb, 8);
>
> if((claimed_rate == 0 && q->frame->rate != BLANK ) ||
> (claimed_rate == 1 && q->frame->rate != RATE_OCTAVE ) ||
> (claimed_rate == 2 && q->frame->rate != RATE_QUARTER) ||
> (claimed_rate == 3 && q->frame->rate != RATE_HALF ) ||
> (claimed_rate == 4 && q->frame->rate != RATE_FULL ))
if the actual values of the enum dont matter then you could make it the same
as claimed_rate and use claimed_rate != q->frame->rate
[...]
> memset(q->frame->data, 0, 76);
> for(n=0; n < q->frame->bits; n++)
> {
> q->frame->data[ order[n].index ] |=
> get_bits1(&q->gb)<<order[n].bitpos;
>
> if(n<20)
> {
> if(n>3) /* this is the random seed for rate 1/8 frames */
> cbseed |= q->frame->data[ order[n].index ]>>n;
> if(n<16) /* this is for a rate 1/8 only sanity check */
> first16 |= q->frame->data[ order[n].index ]>>n;
> }
this code makes no sense, the >>n is not correct
[...]
> if(q->frame->rate != RATE_OCTAVE)
> {
>
> /* check for outbound LSP freqs and codebook gain params */
>
> if(q->frame->rate != RATE_QUARTER)
> {
> if(qtzd_lspf[9] <= .66 || qtzd_lspf[9] >= .985)
> {
> av_log(avctx, AV_LOG_WARNING,
> "IFQ: 9th LSPF=%4f outside [.66,.985]\n", qtzd_lspf[9]);
> is_ifq=1;
> }
>
> for(n=4; !is_ifq && n<10; n++)
> {
> if(FFABS(qtzd_lspf[n]-qtzd_lspf[n-4]) < .0931)
> {
> av_log(avctx, AV_LOG_WARNING,
> "Wrong data, outbound LSPFs\n");
> is_ifq=1;
> }
> }
> }else
> {
> if(qtzd_lspf[9] <= .70 || qtzd_lspf[9] >= .97)
> is_ifq=1;
>
> for(n=3; !is_ifq && n<10; n++)
> {
> if(FFABS(qtzd_lspf[n]-qtzd_lspf[n-2]) < .08)
> is_ifq=1;
> }
>
> /* FIXME This should be implemented into qcelp_decode_params() */
yes and the rest should be in qcelp_decode_lspf()
[...]
> /**
> * Copy current lspf freqs over to prev_lspf
> */
>
> memcpy(q->prev_lspf, qtzd_lspf, 10*sizeof(float));
sizeof(q->prev_lspf)
and doxygen comments should not be in the middle of functions
>
> q->frame_num++;
AVCodecContext.frame_number could be used instead maybe
[...]
qcelp_parser.c:
[...]
> /**
> * Finds the end of the current frame in the bitstream.
> *
> * @return the position of the first byte of the next frame, or -1
> */
>
> static int qcelp_find_frame_end(ParseContext *pc, const uint8_t *buf,
> int buf_size)
> {
> /**
> * Let's try and see if this packet holds exactly one frame
> */
>
> switch(buf_size)
> {
> case 35: // RATE_FULL in 'codec frame' fmt
> case 34: // RATE_FULL
> case 17: // RATE_HALF in 'codec frame' fmt
> case 16: // RATE_HALF
> case 8: // RATE_QUARTER in 'codec frame' fmt
> case 7: // RATE_QUARTER
> case 4: // RATE_OCTAVE in 'codec frame' fmt
> case 3: // RATE_OCTAVE
> return buf_size;
> }
whatever you try to do here it isnt correct
a parser receives random chunks of the bytestream and has to convert that into
proper frames, theres no reason that any of the above sizes would ever occur
also maybe the demuxer actually knows the frame size if its a constant number
of bytes and is buggy and doesnt split the frames properly ...
>
> /**
> * If we reach this point it means the packet holds a multiset of
> * frames, each one of them in codec frame format, all with the same
> * framerate, as described in:
> *
> * http://tools.ietf.org/html/draft-mckay-qcelp-02
> */
this is RTP, if a RTP header reaches the parser than theres a VERY SERIOUS
problem in the demuxer (and in the head of who designed the method of storing
qcelp in the specific container)
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The educated differ from the uneducated as much as the living from the
dead. -- Aristotle
-------------- 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/20070828/fb4934e7/attachment.pgp>
More information about the FFmpeg-soc
mailing list