[FFmpeg-devel] [PATCH 2/3] lavfi: EBU R.128 channel weights

[Clément Bœsch]

On Sat, Jan 19, 2013 at 08:24:53PM +0000, David A. Sedacca wrote:
> Correct the recognition of channel layouts for good channel weight
> in the loudness computation.
> This suggested patch is 2 of 3 for Ticket #2144 "libavfilter ebur128
> loudness inaccuracy, irregular time interval, LFE interference".
> 
> Signed-off-by: David A. Sedacca" <sedacca at comcast.net>
> 
> ---
> 
> This updated patch submission updates the style of the 'if' block
> per Clément's suggestion as clarified by Carl Eugen. It also
> reverts to the original pre-patch behavior to not allocate integrator
> cache memory for zero-weighted channels.
> 
>  libavfilter/f_ebur128.c |   24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/libavfilter/f_ebur128.c b/libavfilter/f_ebur128.c
> index 85fddad..0265767 100644
> --- a/libavfilter/f_ebur128.c
> +++ b/libavfilter/f_ebur128.c
> @@ -314,12 +314,15 @@ static int config_video_output(AVFilterLink *outlink)
>  static int config_audio_output(AVFilterLink *outlink)
>  {
>      int i;
> +    int idx_bitposn = 0;
>      AVFilterContext *ctx = outlink->src;
>      EBUR128Context *ebur128 = ctx->priv;
>      const int nb_channels = av_get_channel_layout_nb_channels(outlink->channel_layout);
>  
>  #define BACK_MASK (AV_CH_BACK_LEFT    |AV_CH_BACK_CENTER    |AV_CH_BACK_RIGHT| \
> -                   AV_CH_TOP_BACK_LEFT|AV_CH_TOP_BACK_CENTER|AV_CH_TOP_BACK_RIGHT)
> +                   AV_CH_TOP_BACK_LEFT|AV_CH_TOP_BACK_CENTER|AV_CH_TOP_BACK_RIGHT| \
> +                   AV_CH_SIDE_LEFT                          |AV_CH_SIDE_RIGHT| \

> +                   AV_CH_SURROUND_DIRECT_LEFT               |AV_CH_SURROUND_DIRECT_RIGHT)

What is the meaning of this "direct" left/right? It sounds like a "front"
speaker to me, but I may be wrong.

>  
>      ebur128->nb_channels  = nb_channels;
>      ebur128->ch_weighting = av_calloc(nb_channels, sizeof(*ebur128->ch_weighting));
> @@ -328,13 +331,24 @@ static int config_audio_output(AVFilterLink *outlink)
>  
>      for (i = 0; i < nb_channels; i++) {
>  
> +        /* find the next bit that is set starting from the right */
> +        while ((outlink->channel_layout & 1ULL<<idx_bitposn) == 0 && idx_bitposn < 63)
> +            idx_bitposn++;
> +
>          /* channel weighting */
> -        if ((outlink->channel_layout & 1ULL<<i) == AV_CH_LOW_FREQUENCY)
> -            continue;
> -        if (outlink->channel_layout & 1ULL<<i & BACK_MASK)
> +        if ((1ULL<<idx_bitposn & AV_CH_LOW_FREQUENCY) ||
> +            (1ULL<<idx_bitposn & AV_CH_LOW_FREQUENCY_2)) {


if (1ULL<<idx_bitposn & (AV_CH_LOW_FREQUENCY|AV_CH_LOW_FREQUENCY_2))?

nit: or you could #define LOW_FREQ_MASK to be consistent with BACK_MASK,
but feel free to ignore.

> +            ebur128->ch_weighting[i] = 0;
> +        } else if (1ULL<<idx_bitposn & BACK_MASK) {
>              ebur128->ch_weighting[i] = 1.41;
> -        else
> +        } else {
>              ebur128->ch_weighting[i] = 1.0;
> +        }
> +
> +        idx_bitposn++;
> +
> +        if (!ebur128->ch_weighting[i])
> +            continue;
>  
>          /* bins buffer for the two integration window (400ms and 3s) */
>          ebur128->i400.cache[i]  = av_calloc(I400_BINS,  sizeof(*ebur128->i400.cache[0]));

Except these details, the patch looks OK (my request about someone
familiar with channel layouts still stands though). I don't have time to
test it, but I believe you did at least with the conformance tests. Thanks
for the patch.

PS: Sorry for the long delay of the review, I'm still having a good time
on another planet.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130202/f3e28684/attachment.asc>