[PATCH] Changes for mono-only field in enable-remixing option for bug 65288

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Tanu,

        Thanks for the detailed review. I am looking into the changes
you have mentioned, will get back to you soon.

regards,
Dinesh

On Wed, May 8, 2013 at 10:52 PM, Tanu Kaskinen <tanu.kaskinen at intel.com> wrote:
> Hi Dinesh (is that your first name?),
>
> Thank you for the patch. I have quite a lot of feedback, but don't take
> the criticism personally.
>
> On Mon, 2013-05-06 at 11:30 +0530, rtdinesh wrote:
>> From: DINESH R T <rtdinesh.89 at gmail.com>
>
> Are you sure you want your name in all-caps? I don't mind it, I just
> want to make sure that you're aware that it will look different than
> most of the names in "git shortlog -s" output.
>
>> Extended enable-remixing field with mono-only option which enables remixing only when source/sink is mono. I couldn't simulate the mono scenario in   source/sink, so i din't test the code but compiled the code with pulseaduio head.
>
> Please wrap the commit message at 70-80 characters.
>
> About testing a mono sink or source: you can load module-null-sink with
> argument channels=1 (e.g. "pactl load-module module-null-sink
> channels=1"). That will create a mono sink, and its monitor source can
> be used as a mono source. You can see the mix matrix for every resampler
> in the verbose log. The matrix looks like this:
>
> D: [pulseaudio] resampler.c: Channel matrix:
> D: [pulseaudio] resampler.c:        I00   I01
> D: [pulseaudio] resampler.c:     +------------
> D: [pulseaudio] resampler.c: O00 | 0.500 0.500
>
> I00 and I01 are the input channels and O00 is the output channel (here
> I'm playing a stereo stream to a mono sink).
>
>>
>> ---
>>  src/daemon/daemon-conf.c      |   26 ++++++++++++++++++++++----
>>  src/daemon/daemon-conf.h      |    2 +-
>>  src/daemon/main.c             |    2 +-
>>  src/pulse/sample.c            |   28 ++++++++++++++++++++++++++++
>>  src/pulse/sample.h            |   19 +++++++++++++++++++
>>  src/pulsecore/core.c          |    2 +-
>>  src/pulsecore/core.h          |    2 +-
>>  src/pulsecore/resampler.c     |   29 ++++++++++++++++++++++++++---
>>  src/pulsecore/resampler.h     |    3 ++-
>>  src/pulsecore/sink-input.c    |    8 +++++---
>>  src/pulsecore/source-output.c |    8 +++++---
>>  11 files changed, 111 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/daemon/daemon-conf.c b/src/daemon/daemon-conf.c
>> index 2c43cf9..45976b1 100644
>> --- a/src/daemon/daemon-conf.c
>> +++ b/src/daemon/daemon-conf.c
>> @@ -84,7 +84,7 @@ static const pa_daemon_conf default_conf = {
>>      .log_meta = FALSE,
>>      .log_time = FALSE,
>>      .resample_method = PA_RESAMPLER_AUTO,
>> -    .disable_remixing = FALSE,
>> +    .remixing_option = PA_DISABLE_REMIXING,
>
> This changes the default. The old default "disable_remixing = FALSE"
> enables remixing.
>
>>      .disable_lfe_remixing = TRUE,
>>      .config_file = NULL,
>>      .use_pid_file = TRUE,
>> @@ -511,6 +511,24 @@ static int parse_nice_level(pa_config_parser_state *state) {
>>      return 0;
>>  }
>>
>> +
>> +
>
> Extra empty lines.
>
>> +static int parse_enable_remixing(pa_config_parser_state *state) {
>> +    pa_daemon_conf *c;
>> +    pa_remixing_option_t f;
>> +
>> +    pa_assert(state);
>> +
>> +    c = state->data;
>> +
>> +    if ((f = pa_parse_remixing(state->rvalue)) < 0) {
>
> This causes a compiler warning, because pa_parse_remixing() is not
> declared before it's being used here. Please fix all compiler warnings
> before submitting patches.
>
>> +        pa_log(_("[%s:%u] Invalid remixing option '%s'."), state->filename, state->lineno, state->rvalue);
>> +        return -1;
>> +    }
>> +
>> +    c->remixing_option = f;
>> +    return 0;
>> +}
>
> Missing empty line.
>
>>  static int parse_rtprio(pa_config_parser_state *state) {
>>  #if !defined(OS_IS_WIN32) && defined(HAVE_SCHED_H)
>>      pa_daemon_conf *c;
>> @@ -600,8 +618,8 @@ int pa_daemon_conf_load(pa_daemon_conf *c, const char *filename) {
>>          { "deferred-volume-extra-delay-usec",
>>                                          pa_config_parse_int,      &c->deferred_volume_extra_delay_usec, NULL },
>>          { "nice-level",                 parse_nice_level,         c, NULL },
>> -        { "disable-remixing",           pa_config_parse_bool,     &c->disable_remixing, NULL },
>> -        { "enable-remixing",            pa_config_parse_not_bool, &c->disable_remixing, NULL },
>> +        { "disable-remixing",           parse_enable_remixing,    &c->remixing_option, NULL },
>> +        { "enable-remixing",            parse_enable_remixing,         &c->remixing_option, NULL },
>
> You can't use the same code to parse both disable-remixing and
> enable-remixing, the two options are supposed to have opposite results.
>
> Handling disable-remixing is a bit tricky, because I don't think
> "mono-only" makes sense there. The disable-remixing option is supported
> only as legacy, so the only goal is to stay compatible with old
> configuration files. Therefore, we should parse it as a boolean
> (pa_config_parse_bool() can't be used here, though, because
> c->remixing_option is not a boolean).
>
> Also, on the "enable-remixing" line there's a tab character that
> shouldn't be there.
>
>>          { "disable-lfe-remixing",       pa_config_parse_bool,     &c->disable_lfe_remixing, NULL },
>>          { "enable-lfe-remixing",        pa_config_parse_not_bool, &c->disable_lfe_remixing, NULL },
>>          { "load-default-script-file",   pa_config_parse_bool,     &c->load_default_script_file, NULL },
>> @@ -790,7 +808,7 @@ char *pa_daemon_conf_dump(pa_daemon_conf *c) {
>>      pa_strbuf_printf(s, "log-target = %s\n", c->auto_log_target ? "auto" : (c->log_target == PA_LOG_SYSLOG ? "syslog" : "stderr"));
>>      pa_strbuf_printf(s, "log-level = %s\n", log_level_to_string[c->log_level]);
>>      pa_strbuf_printf(s, "resample-method = %s\n", pa_resample_method_to_string(c->resample_method));
>> -    pa_strbuf_printf(s, "enable-remixing = %s\n", pa_yes_no(!c->disable_remixing));
>> +    pa_strbuf_printf(s, "enable-remixing = %s\n", pa_remixing_option_to_string(c->remixing_option));
>
> This causes compiler warnings too:
>
> daemon/daemon-conf.c: In function ?pa_daemon_conf_dump?:
> daemon/daemon-conf.c:811:5: warning: implicit declaration of function ?pa_remixing_option_to_string? [-Wimplicit-function-declaration]
> daemon/daemon-conf.c:811:5: warning: format ?%s? expects argument of type ?char *?, but argument 3 has type ?int? [-Wformat]
>
>>      pa_strbuf_printf(s, "enable-lfe-remixing = %s\n", pa_yes_no(!c->disable_lfe_remixing));
>>      pa_strbuf_printf(s, "default-sample-format = %s\n", pa_sample_format_to_string(c->default_sample_spec.format));
>>      pa_strbuf_printf(s, "default-sample-rate = %u\n", c->default_sample_spec.rate);
>> diff --git a/src/daemon/daemon-conf.h b/src/daemon/daemon-conf.h
>> index faf2540..da42cfc 100644
>> --- a/src/daemon/daemon-conf.h
>> +++ b/src/daemon/daemon-conf.h
>> @@ -68,7 +68,6 @@ typedef struct pa_daemon_conf {
>>          system_instance,
>>          no_cpu_limit,
>>          disable_shm,
>> -        disable_remixing,
>>          disable_lfe_remixing,
>>          load_default_script_file,
>>          disallow_exit,
>> @@ -84,6 +83,7 @@ typedef struct pa_daemon_conf {
>>          realtime_priority,
>>          nice_level,
>>          resample_method;
>> +    pa_remixing_option_t remixing_option;
>
> I think "remixing_mode" would be slightly more descriptive name for the
> variable, and similarly pa_remixing_mode_t for the enum type.
>
>>      char *script_commands, *dl_search_path, *default_script_file;
>>      pa_log_target_t log_target;
>>      pa_log_level_t log_level;
>> diff --git a/src/daemon/main.c b/src/daemon/main.c
>> index c18524f..6f8c160 100644
>> --- a/src/daemon/main.c
>> +++ b/src/daemon/main.c
>> @@ -1026,7 +1026,7 @@ int main(int argc, char *argv[]) {
>>      c->resample_method = conf->resample_method;
>>      c->realtime_priority = conf->realtime_priority;
>>      c->realtime_scheduling = !!conf->realtime_scheduling;
>> -    c->disable_remixing = !!conf->disable_remixing;
>> +    c->remixing_option = conf->remixing_option;
>>      c->disable_lfe_remixing = !!conf->disable_lfe_remixing;
>>      c->deferred_volume = !!conf->deferred_volume;
>>      c->running_as_daemon = !!conf->daemonize;
>> diff --git a/src/pulse/sample.c b/src/pulse/sample.c
>> index b613612..cbb2da7 100644
>> --- a/src/pulse/sample.c
>> +++ b/src/pulse/sample.c
>> @@ -157,6 +157,18 @@ const char *pa_sample_format_to_string(pa_sample_format_t f) {
>>
>>      return table[f];
>>  }
>> +const char *pa_remixing_option_to_string(pa_remixing_option_t f) {
>
> I don't think sample.c is the right place for this function. I suggest
> that you put this in resampler.c instead.
>
> "f" as an argument name seems pretty random. Please use "o" or "option"
> instead (or "m" or "mode", if pa_remixing_option_t gets renamed to
> pa_remixing_mode_t).
>
>> +    static const char* const table[]= {
>> +        [PA_DISABLE_REMIXING] = "NO",
>> +        [PA_ENABLE_REMIXING] = "YES",
>> +        [PA_MONO_ONLY] = "MONO-ONLY"
>
> I'd prefer lower-case strings.
>
>> +    };
>> +
>> +    if (f < 0 || f >= PA_REMIXING_MAX)
>> +        return NULL;
>
> You can replace this with an assertion, because it's a bug in our own
> code if the argument is ever invalid.
>
>> +
>> +    return table[f];
>> +}
>>
>>  char *pa_sample_spec_snprint(char *s, size_t l, const pa_sample_spec *spec) {
>>      pa_assert(s);
>> @@ -244,6 +256,22 @@ pa_sample_format_t pa_parse_sample_format(const char *format) {
>>      return PA_SAMPLE_INVALID;
>>  }
>>
>> +
>> +pa_remixing_option_t pa_parse_remixing(const char *format){
>
> Move this to resampler.c too. And rename it to pa_parse_remixing_mode.
>
> The argument name seems to be copy-pasted from somewhere else; it
> doesn't make sense in this context.
>
>> +    pa_assert(format);
>> +    if (strcasecmp(format, "mono-only") == 0)
>> +        return PA_MONO_ONLY;
>> +    else if (strcasecmp(format, "yes") == 0)
>> +        return PA_ENABLE_REMIXING;
>> +    else if (strcasecmp(format,"true") == 0)
>> +        return PA_ENABLE_REMIXING;
>> +    else if(strcasecmp(format, "no") == 0)
>> +        return PA_DISABLE_REMIXING;
>> +    else if(strcasecmp(format, "false") == 0)
>> +        return PA_DISABLE_REMIXING;
>
> It would be simpler to first try pa_parse_boolean(), and if that fails,
> then use strcasecmp(mode, "mono-only").
>
>> +
>> + return PA_REMIXING_INVALID;
>
> Instead of returning INVALID, please return -1 to indicate an error. The
> function can take a pa_remixing_mode_t pointer as an argument for
> returning the actual mode.
>
>> +}
>>  int pa_sample_format_is_le(pa_sample_format_t f) {
>>      pa_assert(f >= PA_SAMPLE_U8);
>>      pa_assert(f < PA_SAMPLE_MAX);
>> diff --git a/src/pulse/sample.h b/src/pulse/sample.h
>> index 9067951..65272a0 100644
>> --- a/src/pulse/sample.h
>> +++ b/src/pulse/sample.h
>> @@ -180,6 +180,21 @@ typedef enum pa_sample_format {
>>      /**< An invalid value */
>>  } pa_sample_format_t;
>>
>> +/* Remixing Options */
>
> This isn't a very useful comment. If you don't have anything non-obvious
> to say, don't write a comment at all.
>
>> +typedef enum pa_remixing_option{
>
> This can be moved to resampler.h, and as I already mentioned, I'd prefer
> pa_remixing_mode as the name.
>
> Also, missing space before {.
>
>> +     /* ENable Remixing */
>
> No tabs, please. "Remixing" shouldn't start with upper-case R.
>
>> +     PA_ENABLE_REMIXING=0,
>
> Enumeration values should have proper prefixes, for example,
> PA_REMIXING_MODE_ENABLE (PA_REMIXING_MODE_ is the prefix).
>
>> +    /* Enable Remix if input/output/both  is mono-only */
>
> s/Remix/remixing/
>
> Extra space after "both".
>
>> +     PA_MONO_ONLY=1,
>> +    /* Disable Remixing */
>> +     PA_DISABLE_REMIXING=2,
>> +    /*Upper limit of valid remixing type */
>> +   PA_REMIXING_MAX=4,
>
> Why 4? This isn't a flag enumeration, so you don't have to specify any
> values yourself.
>
>> +
>> +     PA_REMIXING_INVALID = -1
>
> This is not necessary.
>
>> +}pa_remixing_option_t;
>
> Missing space after }.
>
>> +
>> +
>>  #ifdef WORDS_BIGENDIAN
>>  /** Signed 16 Bit PCM, native endian */
>>  #define PA_SAMPLE_S16NE PA_SAMPLE_S16BE
>> @@ -244,6 +259,10 @@ typedef enum pa_sample_format {
>>  #define PA_SAMPLE_S24BE PA_SAMPLE_S24BE
>>  #define PA_SAMPLE_S24_32LE PA_SAMPLE_S24_32LE
>>  #define PA_SAMPLE_S24_32BE PA_SAMPLE_S24_32BE
>> +
>> +#define PA_ENABLE_REMIXING PA_ENABLE_REMIXING
>> +#define PA_DISABLE_REMIXING PA_DISABLE_REMIXING
>> +#define PA_MONO_ONLY PA_MONO_ONLY
>
> These won't be needed after moving the enumeration to resampler.h
> (sample.h is part of the public API, which requires some special
> treatment).
>
>>  /** \endcond */
>>
>>  /** A sample format and attribute specification */
>> diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
>> index 2ca50c2..330fe04 100644
>> --- a/src/pulsecore/core.c
>> +++ b/src/pulsecore/core.c
>> @@ -138,7 +138,7 @@ pa_core* pa_core_new(pa_mainloop_api *m, pa_bool_t shared, size_t shm_size) {
>>      c->running_as_daemon = FALSE;
>>      c->realtime_scheduling = FALSE;
>>      c->realtime_priority = 5;
>> -    c->disable_remixing = FALSE;
>> +    c->remixing_option = PA_DISABLE_REMIXING;
>
> This changes the default again from enabled to disabled.
>
>>      c->disable_lfe_remixing = FALSE;
>>      c->deferred_volume = TRUE;
>>      c->resample_method = PA_RESAMPLER_SPEEX_FLOAT_BASE + 1;
>> diff --git a/src/pulsecore/core.h b/src/pulsecore/core.h
>> index a8cff5c..0f14878 100644
>> --- a/src/pulsecore/core.h
>> +++ b/src/pulsecore/core.h
>> @@ -174,11 +174,11 @@ struct pa_core {
>>      pa_bool_t disallow_exit:1;
>>      pa_bool_t running_as_daemon:1;
>>      pa_bool_t realtime_scheduling:1;
>> -    pa_bool_t disable_remixing:1;
>>      pa_bool_t disable_lfe_remixing:1;
>>      pa_bool_t deferred_volume:1;
>>
>>      pa_resample_method_t resample_method;
>> +    pa_remixing_option_t remixing_option;
>
> I'd prefer remixing_mode as the variable name here too.
>
>>      int realtime_priority;
>>
>>      pa_server_type_t server_type;
>> diff --git a/src/pulsecore/resampler.c b/src/pulsecore/resampler.c
>> index bc45f06..53f4b03 100644
>> --- a/src/pulsecore/resampler.c
>> +++ b/src/pulsecore/resampler.c
>> @@ -203,7 +203,6 @@ pa_resampler* pa_resampler_new(
>>          pa_resample_flags_t flags) {
>>
>>      pa_resampler *r = NULL;
>> -
>
> No good reason to remove this empty line. The convention is to have an
> empty line between the function's variable declarations and the initial
> assertions.
>
>>      pa_assert(pool);
>>      pa_assert(a);
>>      pa_assert(b);
>> @@ -700,7 +699,7 @@ static void calc_map_table(pa_resampler *r) {
>>                      m->map_table_f[oc][ic] = 1.0f;
>>              }
>>          }
>> -    } else {
>> +    }else {
>
> Don't do that.
>
>>
>>          /* OK, we shall do the full monty: upmixing and downmixing. Our
>>           * algorithm is relatively simple, does not do spacialization, delay
>> @@ -770,11 +769,35 @@ static void calc_map_table(pa_resampler *r) {
>>              ic_unconnected_left = 0,
>>              ic_unconnected_right = 0,
>>              ic_unconnected_center = 0,
>> -            ic_unconnected_lfe = 0;
>> +            ic_unconnected_lfe = 0,
>> +            c;
>>          bool ic_unconnected_center_mixed_in = 0;
>> +        bool input_is_mono_only = true;
>> +        bool output_is_mono_only = true;
>>
>>          pa_assert(remix);
>>
>> +        if(r->flags & PA_RESAMPLER_MONO_ONLY){
>> +
>> +        for (c = 0; c < r->i_cm.channels; c++) {
>> +          if (r->i_cm.map[c] != PA_CHANNEL_POSITION_MONO) {
>> +            input_is_mono_only = false;
>> +            break;
>> +          }
>> +        }
>> +
>> +        for (c = 0; c < r->o_cm.channels; c++) {
>> +          if (r->o_cm.map[c] != PA_CHANNEL_POSITION_MONO) {
>> +            output_is_mono_only = false;
>> +            break;
>> +          }
>> +        }
>> +
>> +        if ((!input_is_mono_only) || (!output_is_mono_only))
>> +          return;
>> +        }
>> +
>> +
>>          for (ic = 0; ic < n_ic; ic++) {
>>              if (on_left(r->i_cm.map[ic]))
>>                  ic_left++;
>> diff --git a/src/pulsecore/resampler.h b/src/pulsecore/resampler.h
>> index 742de6a..1d09ff4 100644
>> --- a/src/pulsecore/resampler.h
>> +++ b/src/pulsecore/resampler.h
>> @@ -52,7 +52,8 @@ typedef enum pa_resample_flags {
>>      PA_RESAMPLER_VARIABLE_RATE = 0x0001U,
>>      PA_RESAMPLER_NO_REMAP      = 0x0002U,  /* implies NO_REMIX */
>>      PA_RESAMPLER_NO_REMIX      = 0x0004U,
>> -    PA_RESAMPLER_NO_LFE        = 0x0008U
>> +    PA_RESAMPLER_NO_LFE        = 0x0008U,
>> +    PA_RESAMPLER_MONO_ONLY     = 0x0010U
>
> I think it would be better to not have another flag here, because it's
> mutually exclusive with PA_RESAMPLER_NO_REMIX. pa_resampler_new()
> interface could be changed so that it takes a remixing mode as a
> parameter, and then stores a boolean in pa_resampler that indicates
> whether remixing should be done or not. Then the NO_REMIX flag could be
> removed too.
>
> This also allows the code that you added to calc_map_table() to be moved
> to pa_resampler_new(). calc_map_table() is complicated enough already as
> it is.
>
> --
> Tanu
>
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki
> Business Identity Code: 0357606 - 4
> Domiciled in Helsinki
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux