[FFmpeg-devel] [PATCH] tools: add make_chlayout_test.plx.

[Stefano Sabatini]

On date Saturday 2012-07-28 15:18:02 +0200, Nicolas George encoded:
> This script uses the flite source to produce files
> suitable to test channels order and layout.
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  tools/make_chlayout_test.plx |  166 ++++++++++++++++++++++++++++++++++++++++++

Why "plx" rather than "pl"? Also, maybe drop the suffix, the other
scripts in tools don't have it.

>  1 file changed, 166 insertions(+)
>  create mode 100755 tools/make_chlayout_test.plx
> 
> Could be simplified a lot by adding a "-channel_layouts" option to ffmpeg
> that lists all known channel layouts with their decomposition and all
> channels with their description. I'll think about it later. 
> 
> diff --git a/tools/make_chlayout_test.plx b/tools/make_chlayout_test.plx
> new file mode 100755
> index 0000000..14a68a3
> --- /dev/null
> +++ b/tools/make_chlayout_test.plx
> @@ -0,0 +1,166 @@
> +#!/usr/bin/env perl
> +
> +# Copyright (c) 2012 Nicolas George
> +#
> +# This file is part of FFmpeg.
> +#
> +# FFmpeg is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +#
> +# FFmpeg is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> +# See the GNU Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public License
> +# along with FFmpeg; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> +
> +
> +# Produce a multichannel test file with the channels clearly identified.
> +#
> +# The test file can be used to check that the layout and order of channels
> +# is correctly handled by a piece of software, either a part of ffmpeg or
> +# not.
> +#
> +# Usage:
> +# tools/make_chlayout_test.plx [out options] out [--] channels
> +#
> +# <out options> are any valid ffmpeg option; if any, -- must be used to
                                        ^^^^^^

options? Or "is a list of any valid ffmpeg option".

> +# separate the output specification from the list of channels.
> +#
> +# Note that some output codecs or formats can not handle arbitrary channel
> +# layout.
> +#
> +# This script requires a ffmpeg source tree with a ffmpeg binary; it must
> +# have the flite audio source enabled.
> +#
> +# Examples:
> +#
> +# Check that the speakers are correctly plugged:
> +# tools/make_chlayout_test.plx -f alsa default -- FL FR
> +#
> +# Produce a 5.1 FLAC file:
> +# tools/make_chlayout_test.plx surround.flac 5.1
> +
> +
> +use strict;
> +use warnings;
> +
> +sub usage($) {
> +  my ($die) = @_;
> +  my $msg = "Usage: $0 [out options] out [--] channels\n";
> +  die $msg if $die;
> +  print $msg;
> +  exit 0;
> +}
> +
> +my $ffmpeg = exists $ENV{FFMPEG} ? $ENV{FFMPEG} :
> +           $0 =~ /(.*)\// && -e "$1/../ffmpeg" ? "$1/../" :
> +           undef;
> +defined $ffmpeg && -e "$ffmpeg/ffmpeg" && -e "$ffmpeg/libavutil/audioconvert.c"
> +  or die "ffmpeg compiled sources not found\n";

This requirement is really strong, and could be avoided by exposing a
list of channel layouts through an option/interface, and could also
simplify much the code below (and make it more robust).

> +
> +sub longhex($) {
> +  my ($s) = @_;
> +  my $r = 0;
> +  for my $d (split "", $s) {
> +    $r = ($r << 4) + hex($d);
> +  }
> +  return $r;
> +}
> +
> +sub log2($) {
> +  my ($v0) = @_;
> +  my $v = $v0;
> +  my $r = 0;
> +  for(my $s = 32; $s > 0; $s--) {
> +    if ($v >= 1 << $s) {
> +      $r += $s;
> +      $v >>= $s;
> +    }
> +  }
> +  die sprintf "0x%x != 1 << %d\n", $v0, $r unless $v0 == 1 << $r;
> +  return $r;
> +}
> +
> +my @all_channels;
> +my %channel_label_to_descr;
> +my @channel_index_to_label;
> +my %layout_define_to_channels;
> +my %layout_label_to_channels;

> +for my $ext ("c", "h") {

Using a single loop for two distinct parsing logics looks very
confusing to me.

> +  my $fn = "$ffmpeg/libavutil/audioconvert.$ext";
> +  open my $f, "<", $fn or die "$fn: $!\n";
> +  while (<$f>) {
> +    if (my ($index, $label, $descr) =

> +        /^\s*\[(\d+)\]\s*=\s*"(\w+)",?\s*\/\*\s*(.*\w)\s*\*\//) {

Showing a comment with:
// parse lines of the form:
//    [3]  = "LFE",       /* low frequency */

should explain what is that about

> +      push @all_channels, $label;
> +      $channel_label_to_descr{$label} = $descr;
> +      $channel_index_to_label[$index] = $label;
> +    }

> +    if (my ($label, $define) = /^\s*\{\s*"(.*?)",\s*\d+,\s*(AV_CH_\w+)\s*\}/) {

    { "mono",        1,  AV_CH_LAYOUT_MONO },

> +      $layout_label_to_channels{$label} = $define;
> +    }
> +    if (my ($name, $val) = /^#define\s+(AV_CH_\w+)\s+0x([0-9A-Fa-f]+)[UL]*/) {
> +      my $index = log2 longhex $val;
> +      next if $index >= 63;
> +      my $channel = $channel_index_to_label[$index]
> +        or die "Channel #$index used and not defined\n";
> +      $layout_define_to_channels{$name} = [ $channel ];
> +    }
> +    if (my ($name, $def) = /^#define\s+(AV_CH_LAYOUT_\w+)\s+\(([\w|]+)\)/) {
> +      my @def = split /\|/, $def;
> +      my @c;
> +      for my $l (@def) {
> +        my $ld = $layout_define_to_channels{$l}
> +          or die "Constant $l used and not defined\n";
> +        push @c, @$ld;
> +      }
> +      $layout_define_to_channels{$name} = \@c;
> +    }
> +  }

> +}
> +for my $l (keys %layout_label_to_channels) {

missing empty line

> +  my $d = $layout_label_to_channels{$l};
> +  $layout_label_to_channels{$l} = $layout_define_to_channels{$d}
> +    or die "Constant $d used and not defined\n";
> +}

...

Again parsing logic looks too complex and depending on the internal
structure of the code, exposing that information at the API or
commandline level would be useful for many reasons and would simplify
this script a lot, so I'd prefer to follow that path.

Also adding some comments in the script with an high level description
would greatly enhance readability.

[...]
-- 
FFmpeg = Furious and Fancy Mastering Ponderous Enigmatic Guru