[FFmpeg-devel] [PATCH] wmapro decoder

[Michael Niedermayer]

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 ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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-devel/attachments/20090605/46bae72a/attachment.pgp>