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.
