[Ffmpeg-devel] [PATCH] ATRAC3 decoder
Michael Niedermayer
michaelni
Mon Feb 19 01:57:40 CET 2007
Hi
On Sun, Feb 18, 2007 at 11:34:29AM +0100, Benjamin Larsson wrote:
> Incomplete specifications can be found here:
>
> http://wiki.multimedia.cx/index.php?title=RealAudio_atrc
>
> Samples here:
>
> http://samples.mplayerhq.hu/real/AC-atrc/
>
> and here:
>
> http://samples.mplayerhq.hu/A-codecs/ATRAC3/
>
> Currently atrac3 in oma/omg (http://samples.mplayerhq.hu/oma/) or psmf
> (http://samples.mplayerhq.hu/PSMF/) is not supported. Transcoding
> to/from the rm container would need a bitstream filter which is not
> supported either (yet).
>
> Any clarifications or fixes are welcome.
[...]
>
> #include "atrac3data.h"
>
> #define JOINT_STEREO 0x12
> #define STEREO 0x2
>
> #define OK 0
> #define ERROR -1
id prefer 0 -1 instead of these names, the fact that -1 means error is very
consistently true in ffmpeg
anyway this is of course a minor issue not related to patch acceptance
>
>
> /* These structures are needed to store the parsed gain control data. */
> typedef struct {
> int num_gain_data;
> int levcode[8];
> int loccode[8];
> } GAIN_INFO;
>
> typedef struct {
> GAIN_INFO gBlock[4];
> } GAIN_BLOCK;
>
> /* tonal component structure */
> typedef struct {
> int pos;
> int numCoefs;
> float coef[8];
> } TONE_COMP;
the comment is not a doxygen compatible comment and actually it is not
very usefull tonal component says little more than TONE_COMP
i also would prefer if the names wouldnt ba all uppercased as this is very
inconsistant to the rest of ffmpeg
>
> typedef struct {
> int bandsCoded;
> int numComponents;
> TONE_COMP components[64];
> float prevFrame[1024];
> int gcBlkSwitch;
> GAIN_BLOCK gainBlock[2];
>
> DECLARE_ALIGNED_16(float, spectrum[1024]);
> DECLARE_ALIGNED_16(float, IMDCT_buf[1024]);
>
> /* qmf delay buffers */
not doxy compatible ...
[...]
> static DECLARE_ALIGNED_16(float,mdct_window[512]);
> static float qmf_window[48];
> static VLC spectral_coeff_tab[7];
> static float SFTable[64];
> static float gain_tab1[16];
> static float gain_tab2[31];
> /* FIXME: Should this be moved to the ATRAC3Context?
> * Regarding multiple instances. */
> static MDCTContext mdct_ctx;
> static DSPContext dsp;
non constant things MUST be moved into the context, i dont know though
if this applies to MDCTContext, a quick look indicates it might work
like above ...
>
>
> /* quadrature mirror synthesis filter */
> static void iqmf (float *inlo, float *inhi, unsigned int nIn, float *pOut, float *delayBuf, float *temp, float *pWindow)
> {
> int cnt, j;
> float *p1, *p2, *p3;
> float s1, s2;
>
> memcpy(temp, delayBuf, 46*sizeof(float));
>
> p1 = inlo;
> p2 = inhi;
> p3 = temp + 46;
>
> /* loop1 */
> for (cnt = nIn / 2; cnt != 0; cnt--) {
> /* Butterfly operation */
> p3[0] = p1[0] + p2[0];
> p3[1] = p1[0] - p2[0];
> p3[2] = p1[1] + p2[1];
> p3[3] = p1[1] - p2[1];
>
> p1 += 2;
> p2 += 2;
> p3 += 4;
> }
instead of p1/p2 inlo/inhi could be used, that would make the code slightly
more readable as you dont have to look up what p1/p2 are, same applies to
p2 below
[...]
> memcpy(delayBuf, (char *)(temp + nIn*2), 46*sizeof(float));
ugly (char*) cast
> }
>
> /**
> * Regular 512 points IMDCT, with the exception of the swapping of odd bands
> *
> * @param pInput float input
> * @param pOutput float output
> * @param odd_band 1 if the band is an odd band
> */
>
> void IMLT_NoOverlap (float *pInput, float *pOutput, int odd_band)
> {
> //FIXME alignment problem when using SIMD ?
> float ref_out[512];
what about DECLARE_ALIGNED_16 ? if it doesnt work its not our problem ...
or do i missunderstand your comment?
[...]
>
> static int atrac3_decode_close(AVCodecContext *avctx)
> {
> ATRAC3Context *q = avctx->priv_data;
> //int i;
> av_log(NULL,AV_LOG_DEBUG, "Deallocating memory.\n");
>
> /* Free allocated memory buffers. */
> av_free(q->pUnits);
> av_free(q->decoded_bytes_buffer);
>
> ff_mdct_end(&mdct_ctx);
> //FIXME needed or not ?
well definitly not if its a global used by other decoders ...
[...]
> /**
> * Mantissa decoding
> *
> * @param gb the GetBit context
> * @param selector what table is the output values coded with
> * @param codingFlag constant length coding or variable length coding
> * @param mantissas mantissa output table
> * @param numCodes amount of values to get
> */
>
> static void DecSpec (GetBitContext *gb, int selector, int codingFlag, int* mantissas, int numCodes)
functions in ffmpeg generally start with lowercase
> {
> int numBits, cnt, signShift, code, huffSymb;
> uint8_t *pTable;
>
>
> if (selector == 1)
> numCodes /= 2;
>
> if (codingFlag != 0) {
> /* constant length coding (CLC) */
> numBits = CLCLengthTab[selector];
>
> if (selector > 1) {
> signShift = 32 - numBits;
>
> for (cnt = 0; cnt < numCodes; cnt++) {
> code = get_bits(gb, numBits);
> /* sign extension */
> code = (code << signShift) >> signShift;
this code is wrong, int is not guranteed to be 32 bits also
get_sbits() might do all that for you?
> mantissas[cnt] = code;
> }
> } else {
> for (cnt = 0; cnt < numCodes; cnt++) {
> code = get_bits(gb, numBits);
isnt numBits guranteed to be 4 here? if so it could be hardcoded to 4
if it can also be 0 then the code is looks broken as get_bits(0) is IIRC
not allowed
[...]
> /**
> * Restore the quantized tonal components
> *
> * @param gb the GetBit context
> * @param numComponents tonal components to report back
> * @param pComponent tone component
> * @param numBands amount of coded bands
> */
>
> static int decodeTonalComponents (GetBitContext *gb, int *numComponents, TONE_COMP *pComponent, int numBands)
> {
> int component_count, components, var48, var4C, i, var54, quant_step_index, cnt, j, var5C, var40;
> int var60, sfIndx, coded_values, eax;
> int band_flags[4];
> int mantissa[8];
> float *pCoef;
> float sf;
>
> component_count = 0;
> *numComponents = 0;
>
> components = get_bits(gb,5);
>
> /* no tonal components */
> if (components == 0)
> return OK;
>
> var48 = get_bits(gb,2);
> if (var48 == 2)
> return ERROR;
>
> var4C = var48 & 1;
the way the variables are named looks like they come straight from a
dissassembler
i remind you that taking disassembler output and converting that 1:1 to
C is a copyright violation (of course i dont know if you did that or not)
codecs for ffmpeg which where based on reverse engeneered binary codecs
MUST be real new implementations not just a per hand decompilation of the
dissassembler output! if theres a 1:1 correspondance of functions or
loops in the functions or of their arguments that is definitly not acceptable
ill review the remainder when you clearly say that its not a "translation"
of a binary codec but a new implementation
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- 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/20070219/7ee2adc8/attachment.pgp>
More information about the ffmpeg-devel
mailing list