[MPlayer-dev-eng] [PATCH] Automatic downmix

[Clément Bœsch]

On Mon, Sep 13, 2010 at 05:01:07PM +0200, Nicolas George wrote:
> Le septidi 27 fructidor, an CCXVIII, Clément Bœsch a écrit :
> > Mmh. The pan filter simply does not apply in this case, and there is this
> > message:
> > 
> >   [AO SDL] Samplerate: 44100Hz Channels: Stereo Format floatle
> >   [AO SDL] Unsupported audio format: 0x1d.
> 
> I am not sure what you mean here, but if the change prevents the audio from
> playing with SDL, I think a lot of people would be entitled to complain.
> 

Sorry I misunderstood this with something else, forget it.

> > +        [3] = "pan=2:" /*FL*/"0.6:0:" /*FR*/"0:0.6:"                                  /*FC*/"0.4:0.4",
> > +        [4] = "pan=2:" /*FL*/"0.6:0:" /*FR*/"0:0.6:" /*BL*/ "0.4:0:"  /*BR*/"0:0.4",
> 
> I think something like this would probably be a little more readable:
> 
> +        /*               FL       FR       RL       RR       FC       LF */
> +        [3] = "pan=2:" "0.6:0:" "0:0.6:"                  "0.4:0.4",
> +        [4] = "pan=2:" "0.6:0:" "0:0.6:" "0.4:0:" "0:0.4",
> 
> with a single line of comments as column titles.
> 

Oh sure, I changed that.

> 
> > +    // Append a downmix pan filter to the end of the chain if needed
> > +    af_downmix(s);
> > +
> 
> I still think the place is not right. I mean: you are tinkering with the
> number of channels. I see just a few lines earlier something that adds a
> "channels" filter. It looks really redundant to me.
> 

I thought it was the right place because it was there where it indeed
deals with the number of channels. Sorry, I was lost with all the filters
init. I think I understand it better now.

After wandering in a few places in the af code, I tried to put the downmix
somewhere in af_init function where it already appends the different
filters.

I justify here the placement in the patch I propose now:

So the af_init function start by checking if it's the first time call.  I
don't understand what is the first init really for, but as the number of
channels is not set (even after the first dummy filter is added), it's not
the correct time to do that.

After this "first-call-processing" a reinit is done. I can't put the code
just before because the number of channels was still not set (I get random
data in it since filter were not init). But after that code, a correct
number of channels is now set for the last filter so I can think about
inserting the downmix filter code.

Following that, there is a first check ("make sure the chain is not empty
and valid") so I won't append the filter just before or I'll just make
that check useless.

Next step is "Check output format". So I think adding the code just before
this final chain-filter-checking is appropriated. Also, as this check is
treating data in the last filter I just added, I think I have to init it
by calling af_reinit on this last added filter. Maybe I should reinit the
whole chain, but since all the previous filters do not depend on this last
one and are already initialized I thought not.

But maybe I'm wrong from the beginning. I'm just explaining why I put it
here right now.

So here is a new version of the patch.

In this version, I also fixed a stupid mistake I introduced it last time
by droping the audio_output_channels check (downmix was done everytime,
even if we get -channels 6).

I also kept the coding style of the function (just for the af_downmix()
call): K&R but 2 spaces indent and 8 spaces replaced with a tab (thought I
didn't had to go to that depth).

Thanks a lot for your patience and help, It's really appreciated.

Regards,

-- 
Clément B.
-------------- next part --------------
Index: libaf/af.c
===================================================================
--- libaf/af.c	(revision 32237)
+++ libaf/af.c	(working copy)
@@ -21,6 +21,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include "osdep/strsep.h"
+#include "libmpcodecs/dec_audio.h"
 
 #include "af.h"
 
@@ -412,6 +413,26 @@
     return AF_OK;
 }
 
+/**
+ * Automatic downmix to stereo in case the codec does not implement it.
+ */
+static void af_downmix(af_stream_t* s)
+{
+    static char * const downmix_strs[AF_NCH + 1] = {
+        /*                FL       FR       RL       RR          FC          LF         AL      AR */
+        [3] = "pan=2:" "0.6:0:" "0:0.6:"                     "0.4:0.4",
+        [4] = "pan=2:" "0.6:0:" "0:0.6:" "0.4:0:"  "0:0.4",
+        [5] = "pan=2:" "0.5:0:" "0:0.5:" "0.2:0:"  "0:0.2:"  "0.3:0.3",
+        [6] = "pan=2:" "0.4:0:" "0:0.4:" "0.2:0:"  "0:0.2:"  "0.3:0.3:"   "0.1:0.1",
+        [7] = "pan=2:" "0.4:0:" "0:0.4:" "0.2:0:"  "0:0.2:"  "0.3:0.3:"              "0.1:0:" "0:0.1",
+        [8] = "pan=2:" "0.4:0:" "0:0.4:" "0.15:0:" "0:0.15:" "0.25:0.25:" "0.1:0.1:" "0.1:0:" "0:0.1",
+    };
+    char *af_pan_str = downmix_strs[s->last->data->nch];
+
+    if (af_pan_str)
+        af_append(s, s->last, af_pan_str);
+}
+
 /* Initialize the stream "s". This function creates a new filter list
    if necessary according to the values set in input and output. Input
    and output should contain the format of the current movie and the
@@ -460,6 +481,13 @@
     if (!af_append(s,s->first,"dummy") || AF_OK != af_reinit(s,s->first))
       return -1;
 
+  // Append a downmix pan filter to the end of the chain if needed
+  if (audio_output_channels == 2) {
+    af_downmix(s);
+    if (AF_OK != af_reinit(s, s->last))
+      return -1;
+  }
+
   // Check output format
   if((AF_INIT_TYPE_MASK & s->cfg.force) != AF_INIT_FORCE){
     af_instance_t* af = NULL; // New filter