diff mbox

[FFmpeg-devel] avfilter/af_pan: ignore named channels not in current layout

Message ID 20170510192202.29800-1-wiiaboo@gmail.com
State New
Headers show

Commit Message

Ricardo Constantino May 10, 2017, 7:22 p.m. UTC
instead of erroring out.

af pan=stereo|FL=FL|FR=FR|FC=FC would error because there's no
Front Center in stereo layout.

This allows just changing the output channel layout and keeping
the same channel configuration across more than one output.

Example usecase
* changing balance to the right-side channels for stereo and 7.1

before:
pan=stereo|FL=0.2*FL+0.8*FR|FR=0*FL+1*FR
pan=7.1|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|BL=0.2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR

after:
pan=stereo|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|BL=0.2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR
pan=7.1|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|BL=0.2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR
---
 libavfilter/af_pan.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Marton Balint May 10, 2017, 11:01 p.m. UTC | #1
On Wed, 10 May 2017, Ricardo Constantino wrote:

> instead of erroring out.
>
> af pan=stereo|FL=FL|FR=FR|FC=FC would error because there's no
> Front Center in stereo layout.
>
> This allows just changing the output channel layout and keeping
> the same channel configuration across more than one output.
>
> Example usecase
> * changing balance to the right-side channels for stereo and 7.1
>
> before:
> pan=stereo|FL=0.2*FL+0.8*FR|FR=0*FL+1*FR
> pan=7.1|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|BL=0.2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR
>
> after:
> pan=stereo|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|BL=0.2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR
> pan=7.1|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|BL=0.2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR

You still have to use a different command line for stereo and 7.1, so I 
don't quite see how this simplifies your use case. Even if your use case 
were justified, I'd still think the current behaviour should be preserved 
as a default.

Regards,
Marton
Ricardo Constantino May 11, 2017, 12:41 a.m. UTC | #2
On 11 May 2017 at 00:01, Marton Balint <cus@passwd.hu> wrote:

>
> On Wed, 10 May 2017, Ricardo Constantino wrote:
>
> instead of erroring out.
>>
>> af pan=stereo|FL=FL|FR=FR|FC=FC would error because there's no
>> Front Center in stereo layout.
>>
>> This allows just changing the output channel layout and keeping
>> the same channel configuration across more than one output.
>>
>> Example usecase
>> * changing balance to the right-side channels for stereo and 7.1
>>
>> before:
>> pan=stereo|FL=0.2*FL+0.8*FR|FR=0*FL+1*FR
>> pan=7.1|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|BL=0
>> .2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR
>>
>> after:
>> pan=stereo|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|
>> BL=0.2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR
>> pan=7.1|FL=0.2*FL+0.8*FR|FR=0.0*FL+1.0*FR|FC=FC|LFE=LFE|BL=0
>> .2*BL+0.8*BR|BR=0.0*BL+1.0*BR|SL=0.2*SL+0.8*SR|SR=0.0*SL+1.0*SR
>>
>
> You still have to use a different command line for stereo and 7.1, so I
> don't quite see how this simplifies your use case. Even if your use case
> were justified, I'd still think the current behaviour should be preserved
> as a default.
>

My usecase is a script that changes the balance from left to right
without having to assume a certain channel layout.

As is, the script needs to know which channels exist in which layout for
every single layout and change the left and right channels. If you happen to
give pan a channel that doesn't exist it'll fail instead of just ignoring
it.
Nicolas George May 11, 2017, 7:49 a.m. UTC | #3
Le duodi 22 floréal, an CCXXV, Marton Balint a écrit :
> You still have to use a different command line for stereo and 7.1, so I
> don't quite see how this simplifies your use case. Even if your use case
> were justified, I'd still think the current behaviour should be preserved as
> a default.

I agree. Bailing on unknown channels also means catching trivial user
mistakes. Furthermore, mixing from different channel layouts requires
different coefficients, otherwise the volume will be too low.

This use case seems too specific to be the default.

Regards,
Ricardo Constantino May 11, 2017, 10:52 a.m. UTC | #4
On 11 May 2017 at 08:49, Nicolas George <george@nsup.org> wrote:

> Le duodi 22 floréal, an CCXXV, Marton Balint a écrit :
> > You still have to use a different command line for stereo and 7.1, so I
> > don't quite see how this simplifies your use case. Even if your use case
> > were justified, I'd still think the current behaviour should be
> preserved as
> > a default.
>
> I agree. Bailing on unknown channels also means catching trivial user
> mistakes. Furthermore, mixing from different channel layouts requires
> different coefficients, otherwise the volume will be too low.
>
> This use case seems too specific to be the default.
>

Sure. The hard part in the script is already done anyway so it's not like I
was
banking on this being merged anyway.

Making a different codepath or a whole new filter just to do easy audio
balance
is way too much sand for my truck, so disregard the patch.
diff mbox

Patch

diff --git a/libavfilter/af_pan.c b/libavfilter/af_pan.c
index 63d7750f35..9c44ba1cda 100644
--- a/libavfilter/af_pan.c
+++ b/libavfilter/af_pan.c
@@ -136,10 +136,9 @@  static av_cold int init(AVFilterContext *ctx)
         }
         if (named) {
             if (!((pan->out_channel_layout >> out_ch_id) & 1)) {
-                av_log(ctx, AV_LOG_ERROR,
-                       "Channel \"%.8s\" does not exist in the chosen layout\n", arg0);
-                ret = AVERROR(EINVAL);
-                goto fail;
+                av_log(ctx, AV_LOG_VERBOSE,
+                       "Channel \"%.8s\" does not exist in the chosen layout, skipping\n", arg0);
+                continue;
             }
             /* get the channel number in the output channel layout:
              * out_channel_layout & ((1 << out_ch_id) - 1) are all the