Re: [cdi-devel] [PATCH] CDI.audio

[Max Reitz <max@xxxxxxxxxx>]

Kevin Wolf wrote:

>> called streams (if anyone knows a better name, I'd appreciate it).
> 
> The German comment says "Kanal", so what about "channel"?

Well, I use the word channel when talking about mono (one channel), stereo (two channels) and so on, so I didn't know if this would be appropriate.

>> +struct cdi_audio_device {
>> +    int record;
> 
> Hm, shouldn't that be a property of a stream/channel? It seems odd to
> have two devices appearing in the system for one physical sound card,
> just to have one for playback and one for recording. It's like having
> a different devices for reading from a disk than for writing to it.

Well, actually I wanted to use multiple streams per device only if there's a hardware mixer, thus I thought it to be more appropriate here.
And, as far as I'm concerned, I do think a microphone and loud speakers actually are different devices. ;-)

>> +    cdi_list_t streams;
> 
> If you want to keep the restriction enforced and still have only one
> device for everything, you could have a cdi_list_t playback_streams and
> a struct cdi_audio_stream* record_stream, which may be NULL.

Hm, yes, maybe as a union (because you know whether a device records or plays, if record stays in this struct)... But on the other hand, it's more uniform this way.

>> +struct cdi_audio_stream {
>> +    size_t buffers;
> 
> num_buffers?

Well, why not?


>> +struct cdi_audio_driver {
>> +    int (*transfer_data)(struct cdi_audio_stream* stream, size_t buffer,
>> +        void* memory);
> 
> Would it make sense to use a cdi_mem_area instead of size_t/void*?

May be true, though I don't think it's a big difference whether the driver receives the structure or just the vaddr, because I don't think any driver will use this buffer directly without copying its content.

> How is success/error defined? You say "The amount of data transmitted
> equals the buffer's size" (I assume this refers to stream->buffer_size).
> So is buffer != stream->buffer_size an error or would it result in
> something like a short write?

I guess it would be considered an error (though I don't know why it should partially fail).

> Also, what's the state after a transfer_data() call? I suppose no data
> has been queued in that case?

If there was an error, that buffer retains its old state (if there was a partial fail, well...).

> Is there a way to update the data later? I'm thinking of a mixer that
> sent 30 seconds in advance to keep the CPU load low, but then the user
> hit the stop button.

The stop button is handled by the application, thus it must make sure the mixer won't get the data 30 seconds in advance (I think; I've never written such an application).

>> +    int (*change_device_status)(struct cdi_audio_device* device, int status);
> 
> Better use 0/1 and leave the rest undefined/reserved. You never know what you
> want to add later.

Sounds good.

>> +    int (*set_sample_rate)(struct cdi_audio_stream* stream, int sample_rate);
> 
> Unit? (Okay, it's obvious, but belongs here anyway)

See above (fixed_sample_rate). *g*

> General direction looks good to me.

Fine. :-)


Max