[pulseaudio-discuss] [PATCH] bluetooth: MP3 passthrough over A2DP

[pl bossart]

Thanks Tanu,
couple of comments on your review:
- I did spent a lot of time correcting these spaces, but there's
always some that go through. If anyone had a indent configuration,
it'd be simpler to review patches having the same style... That's what
gstreamer does. I'' fix those by hand but it's a pain.

> Should this be put a few lines earlier, before "if (codec->configured &&
> seid == 0)"? If we return codec->seid, this function will be called
> again once, but is it possible that also the second invocation will
> return codec->seid? If it's possible, then u->a2dp.has_mpeg is left as
> FALSE, which is probably wrong.

No the code is called twice and the second time with an seid different
from zero. Only when the second call succeeds do you want to record
this endpoint as a valid one.

>> +            pa_log("setup_a2dp: Trying to set-up A2DP Passthrough configuration but no MPEG endpoint available");
>> +            return -1; /* this should not happen */
>
> Please use assertions for stuff that should not happen. That makes it
> much easier to reason about the code - if there's no assertion, the
> reader has to assume that it actually is possible that this code is
> executed in some situation, and if the reader wants to know what
> situation that would be, it may take a significant amount of work to
> figure out that in the end the code can not really be ever executed. The
> assertion is a promise that somebody else has already done that
> checking. Having just a "this should not happen" comment looks like the
> author of the code isn't quite sure whether proper error handling is
> required or not.

I guess the comment is wrong. no need for an assert.

> Time to revisit the sample vs frame discussion... I'm sorry I never
> replied to your last mail concerning this. I'll reply now.
>
> You wrote:
> "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."
>
> I'm fairly sure "pcm frame" means something else to you than to me. If
> the sampling rate of a pcm stream is 48000 Hz and the stream has two
> channels, then one second of that stream contains 48000 pcm frames and
> 96000 samples when using my definition of terms "pcm frame" and
> "sample". I guess you would say that one second of such stream contains
> 48000 samples and some smaller number of pcm frames - I don't have an
> idea what the definition of "pcm frame" would be in this case.

- on the samples/frames discussion, this is just a matter of wording.
People with a DSP background will use samples because that's a concept
they can relate to. There's no such thing as a framing frequency...
Frames are used by people with a CS background who don't understand
what the sampling theorem is.


> There already are functions pa_sample_size() and pa_frame_size() that
> follow the terminology I'm advocating. The audio APIs that I'm familiar
> with use the same terminology: alsa has types called snd_pcm_uframes_t
> and snd_pcm_sframes_t and jack has jack_nframes_t.

That's valid for PCM only.

> It's possible that there was no misunderstaning, and you're really
> claiming that compressed data doesn't care about pcm frames, but I very
> much doubt that...
>
> So, how about renaming this array to pcm_frames_per_mpeg_frame?

If that makes you happy I'll do the change, but this is clearly a
wrong interpretation. You want to infer the length of a frame in ms by
dividing the sampling rate by the number of sample points...


> Fix spaces. Also, you use constant index 1 - that's wrong for < 32kHz
> streams, isn't it?

I have to check this one, can't remember why this was done this way.

>> +        if (preambles == 0x72F81F4E) { /* little endian assumed *

> Where is this magic number specified? What document should I read in
> order to understand and verify this code?

IEC958 standard.