[FFmpeg-devel] [PATCH] wmapro decoder

[Sascha Sommer]

Hi,

On Freitag, 5. Juni 2009, Michael Niedermayer wrote:
> On Fri, Jun 05, 2009 at 07:06:32PM +0200, Sascha Sommer wrote:
> > 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.
>
> the escape code should be rare enough for the if() to be a non issue except
> if gcc pessimizes code outside the escape branch as a result
>
> > 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.
>
> i guess if they use floats internally floats should be output yes
>
> > 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...
>
> iam not asking for twiddling it into the exising decoder but to not
> duplicate (near) identical code. that can be done in many ways ...
>
> I did not review all code so i dont know how much is reasonable to merge,
> my mail was mostly a kind of note that i will insist on (near) identical
> code to be merged when i find some, it could surely be that i wont find
> any beyond the rle stuff ...
>
> [...]

Thanks for the clarification. I'm looking forward to your review ;)

Regards

Sascha