Re: [pulseaudio-discuss] [PATCH v4] alsa-sink/source, sink, source: Consider sample format for avoid-resampling/passthrough
2018년 11월 14일 (수) 오후 6:38, Tanu Kaskinen 님이 작성:
>
> > -if (!supported) {
> > -pa_log_info("Sink does not support sample rate of %d Hz",
> > spec->rate);
> > -return -1;
> > +if (!format_supported) {
> > +pa_log_info("Sink does not support sample format of %s, set it to
> > default value",
> > +pa_sample_format_to_string(spec->format));
> > +pa_sink_set_sample_format(u->sink,
> > u->core->default_sample_spec.format);
>
> This is still not right. We can't assume that u->core-
> >default_sample_spec.format is supported by the hardware.
>
> When the sink is created, it opens the device for the first time, using
> the default format. If that format isn't supported, some other format
> is selected instead (or otherwise the whole sink initialization fails).
> I think this format should be used as the default here, because it's
> known to work.
I got your point. I'll correct it with keeping the 'verified' sample
spec and setting it here.
> >
> > /* Passthrough status change is handled during unsuspend */
> >
> > -return -1;
> > +return 0;
>
> The function now reports success always. This made me have a look at
> the places that check for errors from reconfigure(). There are just two
> places that currently care about the reconfigure() return values:
> pa_sink_reconfigure(), when it reconfigures a monitor sink, and
> pa_source_reconfigure(), when it reconfigures a monitored sink. To me
> it looks like those can be safely changed to just assume that the
> reconfiguration always succeeds. We should change the reconfigure()
> callback return types to void.
I agree with that, it'll be revised also. I appreciate your comment.
Regards,
Sangchul
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4] alsa-sink/source, sink, source: Consider sample format for avoid-resampling/passthrough
On Wed, 2018-10-31 at 23:48 +0900, Sangchul Lee wrote:
> Sample format(e.g. 16 bit, 24 bit) was not considered even if the
> avoid-resampling option is set or the passthrough mode is used.
> This patch checks both sample format and rate of a stream to
> determine whether to avoid resampling in case of the option is set.
> In other word, it is possble to use the stream's original sample
> format and rate without resampling as long as these are supported
> by the device.
>
> pa_sink_input_update_rate() and pa_source_output_update_rate() are
> renamed to pa_sink_input_update_resampler() and pa_source_output
> _update_resampler() respectively.
>
> functions are added as below.
> pa_sink_set_sample_format(), pa_sink_set_sample_rate(),
> pa_source_set_sample_format(), pa_source_set_sample_rate()
>
> Signed-off-by: Sangchul Lee
> ---
> @@ -1674,33 +1717,41 @@ static bool sink_set_formats(pa_sink *s, pa_idxset
> *formats) {
> static int sink_reconfigure_cb(pa_sink *s, pa_sample_spec *spec, bool
> passthrough) {
> struct userdata *u = s->userdata;
> int i;
> -bool supported = false;
> -
> -/* FIXME: we only update rate for now */
> +bool format_supported = false;
> +bool rate_supported = false;
>
> pa_assert(u);
>
> -for (i = 0; u->supported_rates[i]; i++) {
> -if (u->supported_rates[i] == spec->rate) {
> -supported = true;
> +for (i = 0; u->supported_formats[i] != PA_SAMPLE_MAX; i++) {
> +if (u->supported_formats[i] == spec->format) {
> +pa_sink_set_sample_format(u->sink, spec->format);
> +format_supported = true;
> break;
> }
> }
>
> -if (!supported) {
> -pa_log_info("Sink does not support sample rate of %d Hz",
> spec->rate);
> -return -1;
> +if (!format_supported) {
> +pa_log_info("Sink does not support sample format of %s, set it to
> default value",
> +pa_sample_format_to_string(spec->format));
> +pa_sink_set_sample_format(u->sink,
> u->core->default_sample_spec.format);
This is still not right. We can't assume that u->core-
>default_sample_spec.format is supported by the hardware.
When the sink is created, it opens the device for the first time, using
the default format. If that format isn't supported, some other format
is selected instead (or otherwise the whole sink initialization fails).
I think this format should be used as the default here, because it's
known to work.
> }
>
> -if (!PA_SINK_IS_OPENED(s->state)) {
> -pa_log_info("Updating rate for device %s, new rate is %d",
> u->device_name, spec->rate);
> -u->sink->sample_spec.rate = spec->rate;
> -return 0;
> +for (i = 0; u->supported_rates[i]; i++) {
> +if (u->supported_rates[i] == spec->rate) {
> +pa_sink_set_sample_rate(u->sink, spec->rate);
> +rate_supported = true;
> +break;
> +}
> +}
> +
> +if (!rate_supported) {
> +pa_log_info("Sink does not support sample rate of %u, set it to
> default value", spec->rate);
> +pa_sink_set_sample_rate(u->sink, u->core->default_sample_spec.rate);
Same problem here with the rate as with the format earlier.
> }
>
> /* Passthrough status change is handled during unsuspend */
>
> -return -1;
> +return 0;
The function now reports success always. This made me have a look at
the places that check for errors from reconfigure(). There are just two
places that currently care about the reconfigure() return values:
pa_sink_reconfigure(), when it reconfigures a monitor sink, and
pa_source_reconfigure(), when it reconfigures a monitored sink. To me
it looks like those can be safely changed to just assume that the
reconfiguration always succeeds. We should change the reconfigure()
callback return types to void.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH v4] alsa-sink/source, sink, source: Consider sample format for avoid-resampling/passthrough
Sample format(e.g. 16 bit, 24 bit) was not considered even if the
avoid-resampling option is set or the passthrough mode is used.
This patch checks both sample format and rate of a stream to
determine whether to avoid resampling in case of the option is set.
In other word, it is possble to use the stream's original sample
format and rate without resampling as long as these are supported
by the device.
pa_sink_input_update_rate() and pa_source_output_update_rate() are
renamed to pa_sink_input_update_resampler() and pa_source_output
_update_resampler() respectively.
functions are added as below.
pa_sink_set_sample_format(), pa_sink_set_sample_rate(),
pa_source_set_sample_format(), pa_source_set_sample_rate()
Signed-off-by: Sangchul Lee
---
Thanks for the kind comments.
src/modules/alsa/alsa-sink.c | 103 +++-
src/modules/alsa/alsa-source.c | 100 ++-
src/pulsecore/sink-input.c | 22 -
src/pulsecore/sink-input.h | 2 +-
src/pulsecore/sink.c | 90 ---
src/pulsecore/sink.h | 5 +-
src/pulsecore/source-output.c | 22 -
src/pulsecore/source-output.h | 2 +-
src/pulsecore/source.c | 104 +
src/pulsecore/source.h | 5 +-
10 files changed, 314 insertions(+), 141 deletions(-)
diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index 200d12b..3c2110b 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -113,11 +113,19 @@ struct userdata {
pa_sample_format_t *supported_formats;
unsigned int *supported_rates;
+struct {
+size_t fragment_size;
+size_t nfrags;
+size_t tsched_size;
+size_t tsched_watermark;
+size_t rewind_safeguard;
+} initial_info;
size_t
frame_size,
fragment_size,
hwbuf_size,
+tsched_size,
tsched_watermark,
tsched_watermark_ref,
hwbuf_unused,
@@ -1081,13 +1089,36 @@ static void reset_watermark(struct userdata *u, size_t
tsched_watermark, pa_samp
(double) u->tsched_watermark_usec / PA_USEC_PER_MSEC);
}
+/* Called from IO Context on unsuspend */
+static void update_size(struct userdata *u, pa_sample_spec *ss) {
+pa_assert(u);
+pa_assert(ss);
+
+u->frame_size = pa_frame_size(ss);
+u->frames_per_block = pa_mempool_block_size_max(u->core->mempool) /
u->frame_size;
+
+/* use initial values including module arguments */
+u->fragment_size = u->initial_info.fragment_size;
+u->hwbuf_size = u->initial_info.nfrags * u->fragment_size;
+u->tsched_size = u->initial_info.tsched_size;
+u->tsched_watermark = u->initial_info.tsched_watermark;
+u->rewind_safeguard = u->initial_info.rewind_safeguard;
+
+u->tsched_watermark_ref = u->tsched_watermark;
+
+pa_log_info("Updated frame_size %zu, frames_per_block %lu, fragment_size
%zu, hwbuf_size %zu, tsched(size %zu, watermark %zu), rewind_safeguard %zu",
+u->frame_size, (unsigned long) u->frames_per_block,
u->fragment_size, u->hwbuf_size, u->tsched_size, u->tsched_watermark,
u->rewind_safeguard);
+}
+
/* Called from IO context */
static int unsuspend(struct userdata *u) {
pa_sample_spec ss;
int err;
bool b, d;
-snd_pcm_uframes_t period_size, buffer_size;
+snd_pcm_uframes_t period_frames, buffer_frames;
+snd_pcm_uframes_t tsched_frames = 0;
char *device_name = NULL;
+bool frame_size_changed = false;
pa_assert(u);
pa_assert(!u->pcm_handle);
@@ -,13 +1142,19 @@ static int unsuspend(struct userdata *u) {
goto fail;
}
+if (pa_frame_size(>sink->sample_spec) != u->frame_size) {
+update_size(u, >sink->sample_spec);
+tsched_frames = u->tsched_size / u->frame_size;
+frame_size_changed = true;
+}
+
ss = u->sink->sample_spec;
-period_size = u->fragment_size / u->frame_size;
-buffer_size = u->hwbuf_size / u->frame_size;
+period_frames = u->fragment_size / u->frame_size;
+buffer_frames = u->hwbuf_size / u->frame_size;
b = u->use_mmap;
d = u->use_tsched;
-if ((err = pa_alsa_set_hw_params(u->pcm_handle, , _size,
_size, 0, , , true)) < 0) {
+if ((err = pa_alsa_set_hw_params(u->pcm_handle, , _frames,
_frames, tsched_frames, , , true)) < 0) {
pa_log("Failed to set hardware parameters: %s", pa_alsa_strerror(err));
goto fail;
}
@@ -1132,11 +1169,17 @@ static int unsuspend(struct userdata *u) {
goto fail;
}
-if (period_size*u->frame_size != u->fragment_size ||
-buffer_size*u->frame_size != u->hwbuf_size) {
-pa_log_warn("Resume failed, couldn't restore original fragment
settings. (Old: %lu/%lu, New %lu/%lu)",
-(unsigned long) u->hwbuf_size, (unsigned long)
u->fragment_size,
-
