[FFmpeg-devel] [PATCH] ac3enc: add num_rematrixing_bands to AC3EncodeContext and use it instead of the hardcoded value.

[Reimar Döffinger]

On Sat, Mar 05, 2011 at 04:39:15PM -0500, Justin Ruggles wrote:
> On 03/05/2011 06:24 AM, Reimar D?ffinger wrote:
> > On Fri, Mar 04, 2011 at 04:47:14PM -0500, Justin Ruggles wrote:
> >>
> >> Currently it is always 4, but this change will allow it to be adjusted when
> >> bandwidth-related features are added such as channel coupling, enhanced
> >> channel coupling, and spectral extension.
> > 
> > Did you benchmark? If the compiler could do loop unrolling before it
> > might be significant.
> 
> I can benchmark it, but it will have to be changed eventually anyway.
> 
> >> @@ -1304,7 +1307,7 @@ static void output_audio_block(AC3EncodeContext *s, int blk)
> >>          put_bits(&s->pb, 1, block->new_rematrixing_strategy);
> >>          if (block->new_rematrixing_strategy) {
> >>              /* rematrixing flags */
> >> -            for (rbnd = 0; rbnd < 4; rbnd++)
> >> +            for (rbnd = 0; rbnd < s->num_rematrixing_bands; rbnd++)
> >>                  put_bits(&s->pb, 1, block->rematrixing_flags[rbnd]);
> > 
> > Also it seems a bad idea to me to replace the constant 4 in the loop
> > bounds while not doing so in the rematrixing_flags declaration.
> > E.g. an assert that s->num_rematrixing_bands is not larger than
> > the rematrixing_flags size might make sense to add somewhere.
> 
> 
> It never will be greater than 4.  An assert() could certainly be added
> later, but even when it's calculated during each frame encode it will be
> a simple equation that is obviously always between 0 and 4.

Ok, the reason I asked is because these two answers together mean that for
example you could completely unroll it manually and use a switch.
But of course it's all pointless if it has not significant performance
impact.
But if it does, I am sure this way is far slower than it needs to be.