[FFmpeg-devel] avfilter/af_amerge: add reorder_inputs option to be able to turn off reordering

Submitted by Marton Balint on Sept. 4, 2016, 7:45 p.m.

Details

Message ID 1473018301-23871-1-git-send-email-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint Sept. 4, 2016, 7:45 p.m.
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 doc/filters.texi        | 11 +++++++----
 libavfilter/af_amerge.c | 12 ++++++++----
 2 files changed, 15 insertions(+), 8 deletions(-)

Comments

Nicolas George Sept. 4, 2016, 7:48 p.m.
Le nonidi 19 fructidor, an CCXXIV, Marton Balint a écrit :
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  doc/filters.texi        | 11 +++++++----
>  libavfilter/af_amerge.c | 12 ++++++++----
>  2 files changed, 15 insertions(+), 8 deletions(-)

Can you explain the use case?

Regards,
Marton Balint Sept. 4, 2016, 8:08 p.m.
On Sun, 4 Sep 2016, Nicolas George wrote:

> Le nonidi 19 fructidor, an CCXXIV, Marton Balint a écrit :
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  doc/filters.texi        | 11 +++++++----
>>  libavfilter/af_amerge.c | 12 ++++++++----
>>  2 files changed, 15 insertions(+), 8 deletions(-)
>
> Can you explain the use case?
>

Some source files might have a mono track, and a stereo track. If I merge 
these, the order will change, because mono is handled as center.

Regards,
Marton
Nicolas George Sept. 4, 2016, 8:18 p.m.
Le nonidi 19 fructidor, an CCXXIV, Marton Balint a écrit :
> Some source files might have a mono track, and a stereo track. If I merge
> these, the order will change, because mono is handled as center.

And this is completely correct. What would you want instead?

Regards,
Marton Balint Sept. 4, 2016, 10:17 p.m.
On Sun, 4 Sep 2016, Nicolas George wrote:

> Le nonidi 19 fructidor, an CCXXIV, Marton Balint a écrit :
>> Some source files might have a mono track, and a stereo track. If I merge
>> these, the order will change, because mono is handled as center.
>
> And this is completely correct. What would you want instead?

The order of the channels to remain unchanged regardless of the input 
channel layout (which is often guessed only). Filtering in ffmpeg is 
unfortunately forcing you to use channel layouts, even if you don't want 
to, even if you only want to say: this track has 2 channels, that stream 
track has 1 channels. Don't assume anything else.

Another example: You have a file, with mono tracks. Some tracks have 
correct metadata describing the channel layout of the files, others don't. 
You wan't the channels in file-order, regardless of the correctness of the 
input channel layout metadata in the file.

Regards,
Marton
Carl Eugen Hoyos Sept. 4, 2016, 10:34 p.m.
Hi!

2016-09-05 0:17 GMT+02:00 Marton Balint <cus@passwd.hu>:
> Filtering in ffmpeg is unfortunately forcing you to use channel
> layouts, even if you don't want to, even if you only want to say:
> this track has 2 channels, that stream track has 1 channels.

But can't this be fixed?

Carl Eugen
Nicolas George Sept. 5, 2016, 2:43 p.m.
Le decadi 20 fructidor, an CCXXIV, Marton Balint a écrit :
> The order of the channels to remain unchanged regardless of the input
> channel layout (which is often guessed only). Filtering in ffmpeg is
> unfortunately forcing you to use channel layouts, even if you don't want to,
> even if you only want to say: this track has 2 channels, that stream track
> has 1 channels. Don't assume anything else.

I feel Carl Eugen has the right of it.

libavfilter can work with streams with unknown layouts, but some individual
filters may not. To make them work is relatively easy. First, make sure they
do not use the channel layout: most filters from the fork will use the
channel layout to know the number of channels, we have a separated field for
that. Then declare accordingly the list of formats in query formats.

The real problem is ffmpeg.c: it will try to guess the channel layout when
none is specified. I think I added an option to avoid that, but it may have
been lost at some point.

And of course, most of the time, there is a channel layout, you just have to
make sure it is properly set at input and let the logic do its work.

> Another example: You have a file, with mono tracks. Some tracks have correct
> metadata describing the channel layout of the files, others don't.
                                                       ^^^^^^^^^^^^
This is what needs fixing.

>								     You wan't
> the channels in file-order, regardless of the correctness of the input
> channel layout metadata in the file.

Not true. Codecs work with the channel layouts, for inter-channel
correlation, for the cutoff frequency on the subwoofer, etc.

Regards,
Marton Balint Sept. 5, 2016, 6:51 p.m.
On Mon, 5 Sep 2016, Nicolas George wrote:

> Le decadi 20 fructidor, an CCXXIV, Marton Balint a écrit :
>> The order of the channels to remain unchanged regardless of the input
>> channel layout (which is often guessed only). Filtering in ffmpeg is
>> unfortunately forcing you to use channel layouts, even if you don't want to,
>> even if you only want to say: this track has 2 channels, that stream track
>> has 1 channels. Don't assume anything else.
>
> I feel Carl Eugen has the right of it.
>
> libavfilter can work with streams with unknown layouts, but some individual
> filters may not. To make them work is relatively easy. First, make sure they
> do not use the channel layout: most filters from the fork will use the
> channel layout to know the number of channels, we have a separated field for
> that. Then declare accordingly the list of formats in query formats.
>
> The real problem is ffmpeg.c: it will try to guess the channel layout when
> none is specified. I think I added an option to avoid that, but it may have
> been lost at some point.
>
> And of course, most of the time, there is a channel layout, you just have to
> make sure it is properly set at input and let the logic do its work.

The work needed fixing this is definitely not comparable to the work I 
did creating this patch, or the work anyone will have to do maintaining 
it. Asking me to go in this direction, seems an unreasonable amount of 
extra work for me, and most probably I will not do it, and rather abandon 
the patch, or maintain it privately.

>> Another example: You have a file, with mono tracks. Some tracks have correct
>> metadata describing the channel layout of the files, others don't.
>                                                       ^^^^^^^^^^^^
> This is what needs fixing.

I don't trust the input channel layouts, I want to specify that all my 
input channel layouts are unkown. Can/will I do this with -channel_layout 
0 or something like that?

>> 								     You wan't
>> the channels in file-order, regardless of the correctness of the input
>> channel layout metadata in the file.
>
> Not true. Codecs work with the channel layouts, for inter-channel
> correlation, for the cutoff frequency on the subwoofer, etc.
>

Sure, I plan to use uncompressed audio on the output side.

Regards,
Marton
Michael Niedermayer Sept. 5, 2016, 7:36 p.m.
On Mon, Sep 05, 2016 at 04:43:02PM +0200, Nicolas George wrote:
> Le decadi 20 fructidor, an CCXXIV, Marton Balint a écrit :
> > The order of the channels to remain unchanged regardless of the input
> > channel layout (which is often guessed only). Filtering in ffmpeg is
> > unfortunately forcing you to use channel layouts, even if you don't want to,
> > even if you only want to say: this track has 2 channels, that stream track
> > has 1 channels. Don't assume anything else.
> 
> I feel Carl Eugen has the right of it.
> 

> libavfilter can work with streams with unknown layouts, but some individual
> filters may not. To make them work is relatively easy. First, make sure they
> do not use the channel layout: most filters from the fork will use the
> channel layout to know the number of channels, we have a separated field for
> that. Then declare accordingly the list of formats in query formats.

is there some easy way to identify test for filters that require
the layout unneccessary ?
"git grep av_get_channel_layout_nb_channels" and manual checking ?

I might be interrested to help fixing these ...

thx
[...]
Tobias Rapp Sept. 6, 2016, 7:04 a.m.
On 05.09.2016 16:43, Nicolas George wrote:
> Le decadi 20 fructidor, an CCXXIV, Marton Balint a écrit :
>> The order of the channels to remain unchanged regardless of the input
>> channel layout (which is often guessed only). Filtering in ffmpeg is
>> unfortunately forcing you to use channel layouts, even if you don't want to,
>> even if you only want to say: this track has 2 channels, that stream track
>> has 1 channels. Don't assume anything else.
>
> I feel Carl Eugen has the right of it.
>
> libavfilter can work with streams with unknown layouts, but some individual
> filters may not. To make them work is relatively easy. First, make sure they
> do not use the channel layout: most filters from the fork will use the
> channel layout to know the number of channels, we have a separated field for
> that. Then declare accordingly the list of formats in query formats.
>
> The real problem is ffmpeg.c: it will try to guess the channel layout when
> none is specified. I think I added an option to avoid that, but it may have
> been lost at some point.

-guess_layout_max 0

But indeed when using it I stumbled over similar filter issues (amerge 
or pan?). In the end I stopped using it and let the guessing do its 
work, instead I added "-write_channel_mask off" on AVI output side.

And there seems to be no option to explicitly set an unknown input 
channel layout (as Marton noted).

> And of course, most of the time, there is a channel layout, you just have to
> make sure it is properly set at input and let the logic do its work.
>
>> Another example: You have a file, with mono tracks. Some tracks have correct
>> metadata describing the channel layout of the files, others don't.
>                                                        ^^^^^^^^^^^^
> This is what needs fixing.
>
>> 								     You wan't
>> the channels in file-order, regardless of the correctness of the input
>> channel layout metadata in the file.
>
> Not true. Codecs work with the channel layouts, for inter-channel
> correlation, for the cutoff frequency on the subwoofer, etc.

Regards,
Tobias
Carl Eugen Hoyos Sept. 6, 2016, 10:07 a.m.
2016-09-05 20:51 GMT+02:00 Marton Balint <cus@passwd.hu>:

> Can/will I do this with -channel_layout 0 or
> something like that?

This (-channel_layout 0) was broken in 2012.

Carl Eugen
Nicolas George Sept. 10, 2016, 9:58 a.m.
Le decadi 20 fructidor, an CCXXIV, Marton Balint a écrit :
> The work needed fixing this is definitely not comparable to the work I did
> creating this patch, or the work anyone will have to do maintaining it.
> Asking me to go in this direction, seems an unreasonable amount of extra
> work for me, and most probably I will not do it, and rather abandon the
> patch, or maintain it privately.

I understand that, but the patch as is goes in the wrong direction. I am
sure we can discuss this a bit further and find a solution that goes in the
right direction but is still acceptable with regard to the efforts you can
propose.

> I don't trust the input channel layouts, I want to specify that all my input
> channel layouts are unkown. Can/will I do this with -channel_layout 0 or
> something like that?

I think the tool should allow to override all values from the input, as long
as it makes sense, and this applies to the channel layout without doubt.

In other words, I strongly support an option to override the channel layout
detected from inputs, including to set it to unknown whenever it makes
sense.

I do not know if "-channel_layout" is the right option for the job. At a
quick glance, it seems to do a lot of things. A simpler option that checks
the number of channels and overrides the layout may be a better idea; and
naming all the overriding options consistently could be a nice extra
feature.

But...

> Sure, I plan to use uncompressed audio on the output side.

... I do not think that allowing unknown layouts too easily is a good idea.

Sometimes, it makes sense to have an unknown layout (at least until we have
a more expressive system, if it happens); for example,
dual-language-disguised-as-stereo as found in some TV broadcasts can not be
expressed as a layout. But most of the time, there is a channel layout, and
the correct solution is to make sure the tools and library know it as early
as possible.

I am pretty sure you are able to make the difference. But even like that you
may prepare yourself trouble for the future: maybe at some point you will
update your code to change uncompressed PCM to Opus, forget you neglected
the layout issue, and thus lose inter-channel correlation.

Unfortunately, our average users may not have that clear an understanding. I
suspect a lot of them, subjected to an issue, would try setting an unknown
channel layout as suggested by an anonymous on some barely-related
stackexchange question, see it cuts right through their problem and not
realize they are creating slightly invalid files.

Regards,
Carl Eugen Hoyos Sept. 15, 2016, 10:38 a.m.
2016-09-10 11:58 GMT+02:00 Nicolas George <george@nsup.org>:

> In other words, I strongly support an option to override the
> channel layout detected from inputs, including to set it to
> unknown whenever it makes sense.
>
> I do not know if "-channel_layout" is the right option for the job.

Why not?
Afair, I had tested it extensively before it was broken.

Carl Eugen
Nicolas George Sept. 15, 2016, 1:46 p.m.
Le decadi 30 fructidor, an CCXXIV, Carl Eugen Hoyos a écrit :
> Why not?
> Afair, I had tested it extensively before it was broken.

Because "providing a value that is not available" and "overriding a value"
are two very different tasks.

Sometimes you have to specify the channel layout because the format does not
provide the information, for example raw PCM.

What we are discussing here is overriding information provided by the format
but supposed wrong.

I would like to avoid people doing the second when they think they are doing
the first one, or just have no clue about what they are doing exactly.

Regards,
Carl Eugen Hoyos Sept. 15, 2016, 1:55 p.m.
2016-09-15 15:46 GMT+02:00 Nicolas George <george@nsup.org>:
> Le decadi 30 fructidor, an CCXXIV, Carl Eugen Hoyos a écrit :
>> Why not?
>> Afair, I had tested it extensively before it was broken.
>
> Because "providing a value that is not available" and
> "overriding a value" are two very different tasks.

But are we doing this differentiation for any FFmpeg option?

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/doc/filters.texi b/doc/filters.texi
index c12b093..b1e3890 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -1069,15 +1069,18 @@  The filter accepts the following options:
 @item inputs
 Set the number of inputs. Default is 2.
 
+@item reorder_inputs
+Try to reorder the inputs based on their channel layout. Default is true.
+
 @end table
 
 If the channel layouts of the inputs are disjoint, and therefore compatible,
 the channel layout of the output will be set accordingly and the channels
 will be reordered as necessary. If the channel layouts of the inputs are not
-disjoint, the output will have all the channels of the first input then all
-the channels of the second input, in that order, and the channel layout of
-the output will be the default value corresponding to the total number of
-channels.
+disjoint or @var{reorder_inputs} is set to false, the output will have all the
+channels of the first input then all the channels of the second input, in that
+order, and the channel layout of the output will be the default value
+corresponding to the total number of channels.
 
 For example, if the first input is in 2.1 (FL+FR+LF) and the second input
 is FC+BL+BR, then the output will be in 5.1, with the channels in the
diff --git a/libavfilter/af_amerge.c b/libavfilter/af_amerge.c
index 2b4edb0..419e696 100644
--- a/libavfilter/af_amerge.c
+++ b/libavfilter/af_amerge.c
@@ -37,6 +37,7 @@ 
 typedef struct {
     const AVClass *class;
     int nb_inputs;
+    int reorder_inputs;
     int route[SWR_CH_MAX]; /**< channels routing, see copy_samples */
     int bps;
     struct amerge_input {
@@ -53,6 +54,8 @@  typedef struct {
 static const AVOption amerge_options[] = {
     { "inputs", "specify the number of inputs", OFFSET(nb_inputs),
       AV_OPT_TYPE_INT, { .i64 = 2 }, 2, SWR_CH_MAX, FLAGS },
+    { "reorder_inputs", "try to reorder inputs based on their channel layout", OFFSET(reorder_inputs),
+      AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, FLAGS },
     { NULL }
 };
 
@@ -103,10 +106,11 @@  static int query_formats(AVFilterContext *ctx)
         av_log(ctx, AV_LOG_ERROR, "Too many channels (max %d)\n", SWR_CH_MAX);
         return AVERROR(EINVAL);
     }
-    if (overlap) {
-        av_log(ctx, AV_LOG_WARNING,
-               "Input channel layouts overlap: "
-               "output layout will be determined by the number of distinct input channels\n");
+    if (!s->reorder_inputs || overlap) {
+        if (overlap)
+            av_log(ctx, AV_LOG_WARNING,
+                   "Input channel layouts overlap: "
+                   "output layout will be determined by the number of distinct input channels\n");
         for (i = 0; i < nb_ch; i++)
             s->route[i] = i;
         outlayout = av_get_default_channel_layout(nb_ch);