[FFmpeg-devel] [PATCH] WMAPRO packet parser

[Sascha Sommer]

Hi,

On Donnerstag, 14. August 2008, Michael Niedermayer wrote:
> On Thu, Aug 14, 2008 at 08:16:38PM +0200, Sascha Sommer wrote:
> > Hi,
> >
> > attached patch adds support for a wma3 decoder skeleton and packet
> > parser. Basically less than what I expected to do during SOC but before I
> > can continue with the bitstream parsing after SOC I will first have to
> > reverse engineer and understand more of the inner parts of the codec.
>
> well, if you do not have a functional decoder then i think there is only
> limited sense in commiting any of this into svn ...
> anyway, review below, also please see wma/wmadec.c the likely existing
> similarities could help reverse engeneering and understanding. And
> factorizing common code out would be required anyway ...
>

Yes the question will be how similar it will be in the end. I think the 
decoder has to at least to be able to decode some samples before one can 
decide how the wma 1, 2, 3 decoders should be split.

>
> [...]
>
> > +typedef struct {
> > +    uint8_t num_subframes;     //< number of subframes for the current
> > channel +    uint16_t subframe_len[MAX_SUBFRAMES]; //< subframe len in
> > samples +    uint16_t channel_len;                 //< channel len in
> > samples +} wma_channel_t;
>
> vertical align of comments
>

Fixed.

> > +
> > +
> > +/**
> > + *@brief main decoder context
> > + */
> > +typedef struct WMA3DecodeContext {
> > +    AVCodecContext*  avctx;           //< codec context for av_log
> > +    GetBitContext    gb;              //< getbitcontext for the packet
> > +    int              buf_bit_size;    //< buffer size in bits
> > +
> > +    /** Packet info */
> > +    uint8_t          packet_sequence_number; //< current packet nr
> > +    uint8_t          bit5;                   //< padding bit? (cbr
> > files) +    uint8_t          bit6;
> > +    uint8_t          packet_loss;            //< set in case of
> > bitstream error +
> > +    /** Stream info */
> > +    uint16_t         samples_per_frame;     //< nr of outputed samples
> > +    uint8_t          log2_block_align;      //< block align bits
> > +    uint8_t          log2_block_align_bits; //< nr bits for block align
> > len +    uint16_t         log2_frame_size;       //< frame size
> > +    uint8_t          lossless;              //< lossless mode
> > +    uint8_t          no_tiling;             //< frames are split in
> > subframes +    int8_t           nb_channels;           //< nr of channels
> > +    wma_channel_t    channel[MAX_CHANNELS]; //< per channel data
>
> doxygen has special tags for comments covering a block @{ @} IIRC
>

Will have to check that.

> > +
> > +    /** Extradata */
> > +    unsigned int     decode_flags;          //< used compression
> > features +    unsigned int     dwChannelMask;
> > +    uint8_t          sample_bit_depth;      //< bits per sample
> > +
> > +    /** General frame info */
> > +    unsigned int     frame_num;             //< current frame number
> > +    uint8_t          len_prefix;            //< frame is prefixed with
> > its len +    uint8_t          allow_subframes;       //< frames may
> > contain subframes +    uint8_t          max_num_subframes;     //<
> > maximum number of subframes +    uint16_t        
> > min_samples_per_subframe; //< minimum samples per subframe +    uint8_t  
> >        dynamic_range_compression;//< frame contains drc data +    uint8_t
> >          drc_gain;                 //< gain for the drc tool +    uint8_t
> >          update_samples_per_frame; //< recalculate output size +
> > +
> > +    /** Buffered frame data */
> > +    int              prev_frame_bit_size;      //< saved number of bits
> > +    uint8_t*         prev_frame;               //< prev frame data
> > +
> > +} WMA3DecodeContext;
> >
> > +
> > +/**
> > + *@brief helper function to print the most important members of the
> > context + *@param s context
> > + */
> > +static void dump_context(WMA3DecodeContext *s)
> > +{
> > +#define PRINT(a,b) av_log(NULL,AV_LOG_ERROR," %s = %d\n", a, b);
> > +#define PRINT_HEX(a,b) av_log(NULL,AV_LOG_ERROR," %s = %x\n", a, b);
>
> NULL is not a good choice as context
>

Fixed.

>
> [...]
>
> > +/**
> > + *@brief Get the samples per frame for this stream
> > + *@param sample_rate output sample_rate
> > + *@param decode_flags codec compression features
> > + *@return number of output samples per frame
> > + */
> > +static int get_samples_per_frame(int sample_rate, unsigned int
> > decode_flags) { +
> > +    int samples_per_frame;
> > +    int tmp;
> > +
> > +    if (sample_rate <= 16000)
> > +        samples_per_frame = 512;
> > +    else if (sample_rate <= 22050)
> > +        samples_per_frame = 1024;
> > +    else if (sample_rate <= 48000)
> > +        samples_per_frame = 2048;
> > +    else if (sample_rate <= 96000)
> > +        samples_per_frame = 4096;
> > +    else
> > +        samples_per_frame = 8192;
>
> this can be merged with wma.c
>

Ok.

>
> [...]
>
> > +    /** Generic init */
> > +    s->packet_loss = 0;
> > +    s->log2_block_align = av_log2(avctx->block_align);
> >
> > +    s->log2_block_align_bits = av_log2(avctx->block_align*8);
>
> hmm, that looks rather useless
>
> > +    s->log2_frame_size = s->log2_block_align_bits + 1;
>
> that as well
>

I think the number of frame size bits should be enough. The name is misleading 
however.

>
> [...]
>
> > +    while(more_frames && remaining_bits(s) > s->log2_frame_size){
> > +        int frame_size = show_bits(&s->gb, s->log2_frame_size);
> > +
> > +        /** there is enough data for a full frame */
> > +        if(remaining_bits(s) >= frame_size){
> > +            /** decode the frame */
> > +            more_frames = wma_decode_frame(s,&s->gb);
> > +
> > +            if(!more_frames){
> > +                av_log(avctx, AV_LOG_ERROR, "no more frames\n");
> > +            }
> > +        }else
> > +            more_frames = 0;
> > +    }
>
> this reminds me of the first loop in wma_decode_frame()
>

Yeah it should be similar for wma2, 3 and the lossless variant as far as I can 
tell.

Regards

Sascha