[FFmpeg-devel] [PATCH] wmapro decoder

[Sascha Sommer]

Hi,

On Freitag, 5. Juni 2009, Michael Niedermayer wrote:
> On Mon, Jun 01, 2009 at 05:28:07PM +0200, Sascha Sommer wrote:
> > Hi,
> >
> > attached patch adds support for decoding wmapro.
> >
> > I'm awaiting your review so that we can sort out the remaining issues.
>
> I think the first thing to do is to compare it to wma and merge what is
> similar
> rle decoding for example looks very similar
>
> ill do a proper review in the next days, of course if you can reduce
> code duplication relative to wma, that would make my review easier
> and ill insist on all code duplication to be removed anyway when i
> find some.
>
> [...]

Well, let's see

Parsing the start of a packet:

wmadec.c:

        skip_bits(&s->gb, 4); /* super frame index */
        nb_frames = get_bits(&s->gb, 4) - 1;



wmapro:

    /** parse packet header */
    init_get_bits(&gb, buf, s->buf_bit_size);
    packet_sequence_number    = get_bits(&gb, 4);
    s->bit5                   = get_bits1(&gb);
    s->bit6                   = get_bits1(&gb);

    /** get number of bits that need to be added to the previous frame */
    num_bits_prev_frame = get_bits(&gb, s->log2_frame_size);

=> not similar


The same is true for the included packet headers. (Wma1/2 for example store 
the length of the previous subframe in the header wmapro has some header that 
contains the sizes of all subframes, wma1/2 only supports M/S stereo to 
improve compression for multichannel streams, wmapro supports channel 
grouping...)

Decoding of the scale factors:

The vlc version of wmadec.c is similar to the one of wmapro apart from the 
first value and the used vlc table. Also wmapro does the pow before the imdct 
because the decoded scale factors can be recycled for other subframes.
The run level coding is not part of wmadec.c. The lsp coding not part of 
wmapro.

When it comes to coefficient decoding, you are right that the run level part 
(wmadec does not have the vector coding part like wmapro that happens before 
this step) also uses more than one run and level tables and uses code==1 to 
exit the loop, code > 1 for normal coefficients and code == 0 for escape 
decoding. Unfortunatelly the escape decoding is different: 


                } else if (code == 0) {
                    /* escape */
                    level = get_bits(&s->gb, coef_nb_bits);
                    /* NOTE: this is rather suboptimal. reading
                       block_len_bits would be better */
                    run = get_bits(&s->gb, s->frame_len_bits);
                } else {

vs. 
                val = wma_get_large_val(s);
                /** escape decode */
                if (get_bits1(&s->gb)) {
                    if (get_bits1(&s->gb)) {
                        if (get_bits1(&s->gb)) {
                            av_log(s->avctx,AV_LOG_ERROR,
                                   "broken escape sequence\n");
                            return 0;
                        }else
                            cur_coeff += get_bits(&s->gb,s->esc_len) + 4;
                    }else
                        cur_coeff += get_bits(&s->gb,2) + 1;
                }
            }

As this loop should be quit speed critical, I don't know if an if(codec == 
wmapro) is a good idea.


Tansformations:
M/S stereo coding can probably be shared but it is only a
            a = s->coefs[0][i];
            b = s->coefs[1][i];
            s->coefs[0][i] = a + b;
            s->coefs[1][i] = a - b;


The imdct and windowing should be the same but one would first have to make 
wmadec.c use ff_imdct_half. But then this are only a few calls to the imdct 
and dsp functions that are probably similar for other imdct based codecs like 
AAC (for the sine window case), too.
Apart from that, the float to short conversion and the memcpy of the second 
half of the output buffer for the next iteration are identical. I'm not sure 
here, if the decoders should not be changed to output floats instead or at 
least use the appropriate dsp helper function.
  
All in all I don't see so many code duplications that would make it a 
requirement that the decoder is twiddled into the existing wma1/2 decoder but 
it is possible that I missed a few things...

Regards

Sascha