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

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Wed, Nov 17, 2010 at 11:02:11PM +0100, Max Reitz wrote:
> 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.

True. But then Kanal is just as wrong.

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

But you don't have a driver for the microphone but for the sound card.

Anyway, I think this is a point where the text need to be clearer: Matt
seems to be talking about multiple interfaces that address different
output devices whereas you are talking about hardware mixing.

Maybe Matt's point that you can have multiple output devices attached to
only one card means that treating each output/input device (such as
microphones or speakers) as a different cdi_audio_device actually makes
most sense.

In this case, let's just keep what you have to support hardware mixing.


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

Don't think that a union would help. If we keep input/output devices
separate, it's best to leave it as it is.

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

Well, you never know. And even if it converts it to a linear buffer this
might mean that we can save an additional copy in the OS.

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

Assume stream->buffer_size = 42 and I provide buffer = 1337. What
happens? Does it copy 42 bytes (this is what your documentation
suggests) or does it fail immediately?

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

You'll always want to provide some data in advance. The shorter your
buffers are, the higher are the chances that you get dropouts. So the
application can't make it too short. On the other, a stop button that
works only with a delay isn't really acceptable either.

Kevin