[pulseaudio-discuss] [RFC - MP3 passthrough over A2DP 2/2] mp3 passthrough: Bluetooth changes

[pl bossart]

Thanks Tanu for your review. Here are some replies on the comments not
related to formatting. There's a nice idea on passing frame-aligned
data in memchunks that looks interesting, this is valuable feedback.
-Pierre

>> +    uint64_t s_read_index, s_write_index; /* count in samples instead of bytes */
>
> The unit seems to be actually pcm frames, not samples.

It's actually number of samples. Frames only makes sense for PCM, for
compressed data what you need is the number of samples. Frames are
undefined for compressed data.

>> +    codec = (codec_capabilities_t *) rsp->data; /** ALIGNMENT? **/
>
> codec_capabilities_t contains only 8-bit variables, which to my
> understanding guarantees that it doesn't matter where the data happens
> to be located in the memory. If you agree (I don't consider myself an
> alignment expert), then I suggest you remove the "ALIGNMENT?" comment
> from here and from parse_caps.

This is code I copy/pasted from the existing code...No idea if there's
a problem here.

>> +    u->a2dp.has_mpeg = has_mpeg;
>
> As far as I can see, the local has_mpeg variable is redundant; you can
> use "u->a2dp.has_mpeg = TRUE" here.

Right

>> +            /* vbr - we always say its vbr, we don't have how to know it */
>> +            mcap->bitrate = 0x8000;
>
> I'm not sure what "we don't have how to know it" tries to say.

This is taken from the mpapay element in gstreamer. What this is
saying is that bitrate doesn't really matter.

> Also, does every BT device that supports MP3 support VBR?

They should. This is indicated in the endpoint capabilities. I don't
have any headset that doesn't support all valid rates.

>> +            pa_log("setup_a2dp: Trying to set-up A2DP Passthrough configuration but no MPEG endpoint available");
>> +            return -1; /* this should not happen */
>
> If it happens anyway, can the bug be anywhere else than in Pulseaudio's
> code? If not, use an assertion.

No, in that case the profile is not loaded.

>> +
>> +        /* FIXME : number of samples per packet */
>> +        u->samples_per_block = 1152;
>
> So is this assuming certain link_mtu and certain constant (or average)
> bitrate? Since the samples per block ratio doesn't stay constant with
> vbr streams, is this variable going to become unneeded when you
> implement the timing logic correctly?

The issue is more that the mp3 frame is 576 samples below 32kHz, and
1152 above. This was the meaning of my comment.

>> +static int a2dp_passthrough_process_render(struct userdata *u) {
>
> I think you should add "Run from IO thread" comment here. I know
> a2dp_process_render() doesn't have the comment either, but I think it
> should.

Right

>> +    /* inits for output buffer */
>> +    a2dp_prepare_buffer(u);
>
> Not really related to this patch, but I checked a2dp_prepare_buffer
> implementation, and it seemed broken: it sometimes calls pa_xmalloc()
> from the IO thread. Fixing that doesn't belong this patch, though. I
> just thought I'd mention. Someone should fix that, but it's a completely
> separate task, so there's no reason that someone should be you...

yes, I also didn't understand why the buffer is 2x larger than needed.

> I suggest you do the parsing of the mp3 frames as soon as possible, i.e.
> in pa_stream_write(), and drop the data that you can't parse
> immediately. That way there will never be gaps when the data is
> processed in the server (the server must still not crash if such gaps
> are found, in case the client is using something else than libpulse to
> communicate with the server).
>
> I also suggest that you make sure that memchunks contain only whole
> frames. That way it's always possible to map a memchunk's data to time,
> and also map a memblockq's data to time. I would guess that those
> mappings will be needed.

It's assumed that some level of parsing is done before going into
pulseaudio (such as mp3parse in gstreamer). I initially through I
would only pass bytes through the pa_stream interface. But maybe you
are right, passing frame-aligned binary data with additional fields
such as type and length would make my life simpler. This is an
interesting idea, let me think about it. That could avoid some of the
timing issues and API breakage we are facing otherwise.

>> +    header->m = talk_spurt;
>
> talk_spurt is a redundant variable. You can use simply header->m = 1
> here.

Yes. I used to set this variable only when a first sync word was
found, as explained in the RTP spec, but apparently it's set to one
all the time in the mpapay implementation.

>> +    header->sequence_number = htons(a2dp->seq_num++);
>> +    header->ssrc = htonl(1); /* should in theory be random */
>
> Why should it? And why is it not? What are the consequences of this
> choice?

In the RTP spec, it is recommended that the ssrc be random for each
session for additional security. In the BT case, this is really not
that important.

>>                      if (audio_sent <= time_passed) {
>>                          pa_usec_t audio_to_send = time_passed - audio_sent;
>>
>>                          /* Never try to catch up for more than 100ms */
>> -                        if (u->write_index > 0 && audio_to_send > MAX_PLAYBACK_CATCH_UP_USEC) {
>> +                        if (u->s_write_index > 0 && audio_to_send > MAX_PLAYBACK_CATCH_UP_USEC) {
>>                              pa_usec_t skip_usec;
>>                              uint64_t skip_bytes;
>>
>>                              skip_usec = audio_to_send - MAX_PLAYBACK_CATCH_UP_USEC;
>>                              skip_bytes = pa_usec_to_bytes(skip_usec, &u->sample_spec);
>>
>> +                            /* FIXME: this doesn't work for PASSTHROUGH */
>
> Any ideas how to fix this? Would moving u->started_at forward by
> skip_bytes make sense for passthrough?

I am not sure why this is needed. Luiz should really comment why this
code was there in the first place.