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

[Tanu Kaskinen]

On Sun, 2010-11-07 at 18:37 -0600, pl bossart wrote:
> 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.

I would imagine that if you start from the beginning of the review mail
and go through the comments one by one, not skipping around (perhaps
making notes for the comments that you don't agree with and skipping
those in the first round), you would end up fixing every issue that was
pointed out.

> 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.

I agree, basic formatting should be automated. I'll look into making an
indent configuration.

> > 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.

Can you add a comment for

    if (codec->configured && seid == 0)
        return codec->seid;

in parse_mpeg_caps()? I don't know what codec->configured means and I
don't know what seid or codec->seid means, so I have really no clue what
this code means (otherwise the code is nicely self-explanatory!).

Anyway, since the caller can't distinguish between a successful call
(return value zero) and the case where codec->seid is zero, I think you
should also add the following check before the code pasted above:

    if (codec->seid == 0) {
        pa_log("Oh no!");
        return -1;
    }

And, your comment above ("only when the second call succeeds do you want
to record this endpoint as a valid one") indicates that the second
invocation of parse_mpeg_caps() really should return zero, otherwise the
endpoint is not valid, so I think the ret check after the second call
should be

    if (ret != 0)
        return -1;

instead of

    if (ret < 0)
        return -1;

While figuring this out, I also noticed that there's a redundant
u->profile == PROFILE_A2DP_PASSTHROUGH check in parse_mpeg_caps():

    if ((u->profile == PROFILE_A2DP_PASSTHROUGH) && (codec->transport != BT_CAPABILITIES_TRANSPORT_A2DP))

It's guaranteed (the function even begins with an assertion) that the
profile is PROFILE_A2DP_PASSTHROUGH.

> >> +            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.

setup_a2dp() is called after get_caps(), and get_caps() should fail if
the requested profile is PROFILE_A2DP_PASSTHROUGH but the device doesn't
support mpeg, so an assert is correct here (and in set_conf()).

> > 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...

Yes, but "number of samples" doesn't unambiguously refer to "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.
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at mail.0pointer.de
> https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss