Re: [RFC - AAF PCM plugin 3/5] aaf: Implement Playback mode support

["Guedes, Andre" <andre.guedes@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5496 bytes --]

Hi Pierre,

On Mon, 2018-08-20 at 22:37 -0500, Pierre-Louis Bossart wrote:
> > +#define CLOCK_REF		CLOCK_REALTIME
> 
> You should probably comment on why you use CLOCK_REALTIME for
> something 
> that is driven by a media clock that's completely unrelated to 
> CLOCK_REALTIME?

Sure, I'll add a comment here explaining why.

Just anticipating, the AAF plugin implements a media clock itself. To
achieve that it relies on a periodic timer, which requires a clock id.
CLOCK_REALTIME was chosen because we can synchronize it against the PTP
 clock (see instructions on doc/aaf.txt) and use it to get PTP time.
PTP time is the time reference in an AVTP system e.g. time reference
utilized to set presentation time.

> > +static unsigned int alsa_to_avtp_format(snd_pcm_format_t format)
> > +{
> > +	switch (format) {
> > +	case SND_PCM_FORMAT_S16_BE:
> 
> we usually use S16_LE? I can't recall when I last used S16_BE...

AVTP specifies that the byte order of the samples is network order
(i.e. big-endian). So, for simplicity, the plugin accepts only BE
samples (see aaf_hw_params) at the moment.

Later on, we can extend the plugin to convert LE to BE, or rely on
another plugin (e.g. linear) to do the conversion.

> > +		return AVTP_AAF_FORMAT_INT_16BIT;
> > +	case SND_PCM_FORMAT_S24_3BE:
> 
> this means 3 bytes without padding to 32-bit word, is this your 
> definition as well?

Yes, that's AVTP definition.

> > +static unsigned int alsa_to_avtp_rate(unsigned int rate)
> > +{
> > +	switch (rate) {
> > +	case 8000:
> > +		return AVTP_AAF_PCM_NSR_8KHZ;
> > +	case 16000:
> > +		return AVTP_AAF_PCM_NSR_16KHZ;
> > +	case 24000:
> > +		return AVTP_AAF_PCM_NSR_24KHZ;
> > +	case 32000:
> > +		return AVTP_AAF_PCM_NSR_32KHZ;
> > +	case 44100:
> > +		return AVTP_AAF_PCM_NSR_44_1KHZ;
> > +	case 48000:
> > +		return AVTP_AAF_PCM_NSR_48KHZ;
> > +	case 88200:
> > +		return AVTP_AAF_PCM_NSR_88_2KHZ;
> > +	case 96000:
> > +		return AVTP_AAF_PCM_NSR_96KHZ;
> > +	case 176400:
> > +		return AVTP_AAF_PCM_NSR_176_4KHZ;
> > +	case 192000:
> > +		return AVTP_AAF_PCM_NSR_192KHZ;
> 
> You should align avtp_aaf definitions with ALSA, you are missing
> quite a 
> few frequencies. e.g. 11.025, 64, 384kHz. If this is intentional add
> a 
> comment to explain the restrictions.

This is intentional. AVTP doesn't support all frequencies. I'll add a
comment as requested.

> > +static int aaf_init_areas(snd_pcm_aaf_t *aaf)
> > +{
> > +	snd_pcm_channel_area_t *audiobuf_areas, *payload_areas;
> > +	ssize_t sample_size, frame_size;
> > +	snd_pcm_ioplug_t *io = &aaf->io;
> > +
> > +	sample_size = snd_pcm_format_size(io->format, 1);
> > +	if (sample_size < 0)
> > +		return sample_size;
> > +
> > +	frame_size = sample_size * io->channels;
> > +
> > +	audiobuf_areas = calloc(io->channels,
> > sizeof(snd_pcm_channel_area_t));
> > +	if (!audiobuf_areas)
> > +		return -ENOMEM;
> > +
> > +	payload_areas = calloc(io->channels,
> > sizeof(snd_pcm_channel_area_t));
> > +	if (!payload_areas) {
> > +		free(audiobuf_areas);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (unsigned int i = 0; i < io->channels; i++) {
> > +		audiobuf_areas[i].addr = aaf->audiobuf;
> > +		audiobuf_areas[i].first = i * sample_size * 8;
> > +		audiobuf_areas[i].step = frame_size * 8;
> > +
> > +		payload_areas[i].addr = aaf->pdu->avtp_payload;
> > +		payload_areas[i].first = i * sample_size * 8;
> > +		payload_areas[i].step = frame_size * 8;
> 
> not sure what 8 represents here?

Number of bits per byte.

'sample_size' and 'frame_size' are in bytes. 'first' and 'step' fields
from 'snd_pcm_channel_area_t' are in bits.

> > +static int aaf_mclk_start_playback(snd_pcm_aaf_t *aaf)
> > +{
> > +	int res;
> > +	struct timespec now;
> > +	struct itimerspec itspec;
> > +	snd_pcm_ioplug_t *io = &aaf->io;
> > +
> > +	res = clock_gettime(CLOCK_REF, &now);
> > +	if (res < 0) {
> > +		SNDERR("Failed to get time from clock");
> > +		return -errno;
> > +	}
> > +
> > +	aaf->mclk_period = (NSEC_PER_SEC * aaf->frames_per_pkt) /
> > io->rate;
> 
> is this always an integer? If not, don't you have a systematic 
> arithmetic error?

NSEC_PER_SEC is 64-bit so I don't see an arithmetic error during
calculation (e.g. integer overflow). Not sure this was your concern,
though. Let me know otherwise.

> > +static int aaf_poll_revents(snd_pcm_ioplug_t *io, struct pollfd
> > *pfd,
> > +			    unsigned int nfds, unsigned short
> > *revents)
> > +{
> > +	int res;
> > +	snd_pcm_aaf_t *aaf = io->private_data;
> > +
> > +	if (nfds != FD_COUNT_PLAYBACK)
> > +		return -EINVAL;
> > +
> > +	if (pfd[0].revents & POLLIN) {
> > +		res = aaf_mclk_timeout_playback(aaf);
> > +		if (res < 0)
> > +			return res;
> > +
> > +		*revents = POLLIN;
> > +	}
> 
> I couldn't figure out how you use playback events and your timer. 

Every time aaf->timer_fd expires, the audio buffer is consumed by the
plugin, making some room available on the buffer. So here a POLLIN
event is returned so alsa-lib layer can copy more data into the audio
buffer.

> When there are two audio clock sources or timers that's usually where
> the fun begins.

Regarding scenarios with two audio clock sources or timers, the plugin
doesn't support them at the moment. This is something we should work on
once the basic functionality is pushed upstream.

Thank you for your feedback.

Regards,

Andre

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]