Hi Pierre,
I read the patch through. I propose using assertions liberally, but I
haven't done any research about how many ifs you'd have to add to the
calling code when the functions start to hit assertions with PA_MP3, or
whether there are even bigger issues with that approach.
On Tue, 2010-09-21 at 18:00 -0500, bossart.nospam at gmail.com wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart at intel.com>
>
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at intel.com>
>
> diff --git a/src/pulse/sample.c b/src/pulse/sample.c
> index 9698d8a..39ffca9 100644
> --- a/src/pulse/sample.c
> +++ b/src/pulse/sample.c
> @@ -49,7 +49,8 @@ static const size_t size_table[] = {
> [PA_SAMPLE_S24LE] = 3,
> [PA_SAMPLE_S24BE] = 3,
> [PA_SAMPLE_S24_32LE] = 4,
> - [PA_SAMPLE_S24_32BE] = 4
> + [PA_SAMPLE_S24_32BE] = 4,
> + [PA_MP3] = 1
> };
>
> size_t pa_sample_size_of_format(pa_sample_format_t f) {
> @@ -71,6 +72,9 @@ size_t pa_frame_size(const pa_sample_spec *spec) {
> pa_assert(spec);
> pa_return_val_if_fail(pa_sample_spec_valid(spec), 0);
>
> + if (spec->format == PA_MP3)
> + return size_table[spec->format];
> +
> return size_table[spec->format] * spec->channels;
> }
pa_frame_size() can't return anything meaningful for mp3, so I think it
would make sense to have pa_assert(spec->format != PA_MP3) here.
> @@ -78,6 +82,9 @@ size_t pa_bytes_per_second(const pa_sample_spec *spec) {
> pa_assert(spec);
> pa_return_val_if_fail(pa_sample_spec_valid(spec), 0);
>
> + if (spec->format == PA_MP3)
> + return 0;
> +
> return spec->rate * size_table[spec->format] * spec->channels;
> }
Again, unless there's a good reason not to, I'd put
pa_assert(spec->format != PA_MP3) here.
> @@ -85,6 +92,9 @@ pa_usec_t pa_bytes_to_usec(uint64_t length, const pa_sample_spec *spec) {
> pa_assert(spec);
> pa_return_val_if_fail(pa_sample_spec_valid(spec), 0);
>
> + if (spec->format == PA_MP3)
> + return 0;
> +
> return (((pa_usec_t) (length / (size_table[spec->format] * spec->channels)) * PA_USEC_PER_SEC) / spec->rate);
> }
Ditto.
> @@ -92,6 +102,9 @@ size_t pa_usec_to_bytes(pa_usec_t t, const pa_sample_spec *spec) {
> pa_assert(spec);
> pa_return_val_if_fail(pa_sample_spec_valid(spec), 0);
>
> + if (spec->format == PA_MP3)
> + return 0;
> +
> return (size_t) (((t * spec->rate) / PA_USEC_PER_SEC)) * (size_table[spec->format] * spec->channels);
> }
Ditto.
> @@ -151,6 +164,7 @@ const char *pa_sample_format_to_string(pa_sample_format_t f) {
> [PA_SAMPLE_S24BE] = "s24be",
> [PA_SAMPLE_S24_32LE] = "s24-32le",
> [PA_SAMPLE_S24_32BE] = "s24-32be",
> + [PA_MP3] = "binary stream"
> };
Why not "mp3" or "MP3 stream"? Surely you won't be using PA_MP3 as a
generic format for all kinds of compressed streams? That would be quite
confusing naming.
> if (f < 0 || f >= PA_SAMPLE_MAX)
> @@ -241,6 +255,8 @@ pa_sample_format_t pa_parse_sample_format(const char *format) {
> return PA_SAMPLE_S24_32NE;
> else if (strcasecmp(format, "s24-32re") == 0)
> return PA_SAMPLE_S24_32RE;
> + else if (strcasecmp(format, "mp3") == 0)
> + return PA_MP3;
>
> return -1;
> }
> diff --git a/src/pulse/sample.h b/src/pulse/sample.h
> index 7a4a55a..3ca723f 100644
> --- a/src/pulse/sample.h
> +++ b/src/pulse/sample.h
> @@ -164,6 +164,9 @@ typedef enum pa_sample_format {
> PA_SAMPLE_S24_32BE,
> /**< Signed 24 Bit PCM in LSB of 32 Bit words, big endian. \since 0.9.15 */
>
> + PA_MP3,
> + /**< Unsigned 8-bit MP3 binary stream */
> +
> PA_SAMPLE_MAX,
> /**< Upper limit of valid sample types */
Add a \since X.X.X tag.
> diff --git a/src/pulsecore/envelope.c b/src/pulsecore/envelope.c
> index 75e189c..a91c27b 100644
> --- a/src/pulsecore/envelope.c
> +++ b/src/pulsecore/envelope.c
> @@ -922,7 +922,7 @@ void pa_envelope_apply(pa_envelope *e, pa_memchunk *chunk) {
> }
> /* FIXME */
> pa_assert_not_reached();
> -
> + case PA_MP3:
> case PA_SAMPLE_MAX:
> case PA_SAMPLE_INVALID:
> pa_assert_not_reached();
> diff --git a/src/pulsecore/mime-type.c b/src/pulsecore/mime-type.c
> index b9fe944..55963fc 100644
> --- a/src/pulsecore/mime-type.c
> +++ b/src/pulsecore/mime-type.c
> @@ -133,6 +133,7 @@ void pa_sample_spec_mimefy(pa_sample_spec *ss, pa_channel_map *cm) {
> ss->format = PA_SAMPLE_U8;
> break;
>
> + case PA_MP3:
> case PA_SAMPLE_MAX:
> case PA_SAMPLE_INVALID:
> pa_assert_not_reached();
> diff --git a/src/pulsecore/sample-util.c b/src/pulsecore/sample-util.c
> index 74600de..48381de 100644
> --- a/src/pulsecore/sample-util.c
> +++ b/src/pulsecore/sample-util.c
> @@ -89,6 +89,8 @@ static uint8_t silence_byte(pa_sample_format_t format) {
> return 0xd5;
> case PA_SAMPLE_ULAW:
> return 0xff;
> + case PA_MP3:
> + return 0;
> default:
> pa_assert_not_reached();
> }
You can't silence mp3 streams by setting random bytes to some magic
value, so this function shouldn't be called for mp3 streams. Therefore,
the mp3 case should hit pa_assert_not_reached().
> @@ -917,6 +919,10 @@ pa_memchunk* pa_silence_memchunk_get(pa_silence_cache *cache, pa_mempool *pool,
> case PA_SAMPLE_ULAW:
> cache->blocks[PA_SAMPLE_ULAW] = b = silence_memblock_new(pool, 0xff);
> break;
> + case PA_MP3:
> + /* FIXME: silence does not make sense for compressed data */
> + cache->blocks[PA_MP3] = b = silence_memblock_new(pool,0);
> + break;
> default:
> pa_assert_not_reached();
> }
pa_silence_memchunck_get() shouldn't be called when handling mp3 streams
or sinks, so use pa_assert_not_reached().
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 190e2d0..fad2b8b 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -296,18 +296,20 @@ int pa_sink_input_new(
> !pa_sample_spec_equal(&data->sample_spec, &data->sink->sample_spec) ||
> !pa_channel_map_equal(&data->channel_map, &data->sink->channel_map)) {
>
> - if (!(resampler = pa_resampler_new(
> - core->mempool,
> - &data->sample_spec, &data->channel_map,
> - &data->sink->sample_spec, &data->sink->channel_map,
> - data->resample_method,
> - ((data->flags & PA_SINK_INPUT_VARIABLE_RATE) ? PA_RESAMPLER_VARIABLE_RATE : 0) |
> - ((data->flags & PA_SINK_INPUT_NO_REMAP) ? PA_RESAMPLER_NO_REMAP : 0) |
> - (core->disable_remixing || (data->flags & PA_SINK_INPUT_NO_REMIX) ? PA_RESAMPLER_NO_REMIX : 0) |
> - (core->disable_lfe_remixing ? PA_RESAMPLER_NO_LFE : 0)))) {
> - pa_log_warn("Unsupported resampling operation.");
> - return -PA_ERR_NOTSUPPORTED;
> - }
> + /* no resampler for binary content */
> + if( data->sample_spec.format != PA_MP3 )
Space after "if", but not after "(" or before ")".
> + if (!(resampler = pa_resampler_new(
> + core->mempool,
> + &data->sample_spec, &data->channel_map,
> + &data->sink->sample_spec, &data->sink->channel_map,
> + data->resample_method,
> + ((data->flags & PA_SINK_INPUT_VARIABLE_RATE) ? PA_RESAMPLER_VARIABLE_RATE : 0) |
> + ((data->flags & PA_SINK_INPUT_NO_REMAP) ? PA_RESAMPLER_NO_REMAP : 0) |
> + (core->disable_remixing || (data->flags & PA_SINK_INPUT_NO_REMIX) ? PA_RESAMPLER_NO_REMIX : 0) |
> + (core->disable_lfe_remixing ? PA_RESAMPLER_NO_LFE : 0)))) {
> + pa_log_warn("Unsupported resampling operation.");
> + return -PA_ERR_NOTSUPPORTED;
> + }
> }
>
> i = pa_msgobject_new(pa_sink_input);
> diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
> index 57ccc06..0280863 100644
> --- a/src/pulsecore/source-output.c
> +++ b/src/pulsecore/source-output.c
> @@ -440,6 +440,10 @@ void pa_source_output_push(pa_source_output *o, const pa_memchunk *chunk) {
> pa_assert(chunk);
> pa_assert(pa_frame_aligned(chunk->length, &o->source->sample_spec));
>
> + /* FIXME: this is a hack to disable monitoring until I figure out
> + how to do it in a nicer way */
> + return;
> +
> if (!o->push || o->thread_info.state == PA_SOURCE_OUTPUT_CORKED)
> return;
I believe it should work if you just protect the monitor source creation
code with "if (s->sample_spec.format != PA_MP3)"... ok, I did a quick
search for "monitor" in sink.c, and it turns out that you'll have to
also add "if (s->monitor_source)" in many places.
--
Tanu Kaskinen
