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.
