Date: Tue, 18 Jun 2019 17:29:11 +0300
Hi Peter,
Thanks for the great feedback! This is really useful stuff. My replies below.
> On 18 Jun 2019, at 12:13, Peter Sommerlad (CPP) <peter.cpp_at_[hidden] <mailto:peter.cpp_at_[hidden]>> wrote:
> 1. why is the inconsistency in represening counts/sizes between audio_buffer and audio_device? size_t vs. int, naming of member functions.
You mean you don’t like that audio_device::num_input/output_channels() returns an int? Yeah, I guess this should be an unsigned integer. However, should we really use 64-bit size_t considering that there won’t ever be an audio device with millions of audio channels? That’s a serious question. What’s the guidance here?
>
> 2. in 6.5:
> is the iterator return type of audio_device_list define somewhere else? if not, it should at least be a typedef in the list type and specify, that its operator* returns an audio_device (or an optional<audio_device> to make code safer.)
Yes it totally should be. This is not formal wording yet. Note this paragraph in section 6.2:
Note that the API description below is intentionally incomplete so that the signal does not get overwhelmed by the noise. For example, we have left out constructors, destructors, and thorough application of const and noexcept to name but a few items. These functions, as well as more formal wording to specify them, will come in future revisions of this document. Furthermore, all names in this proposal are placeholder names subject to future bikeshedding.
>
> 3. why are the setter functions not noexcept? They return a bool to denote success. I consider the many bool returning functions a kind of design smell. Has anybody considered to employ enums for that? (I understand that often requires to negate logic).
Setting parameters like a buffer size and sample rate can fail. What we need here is a proper error handling mechanism. You are right, those should be noexcept and they should not be returning bools. My current thinking is to rewrite this for R3 to use std::error_code, like std::filesystem does. Does that sound better?
> Also a type with getters and setters is always suspect for me, since it leaks abstractions. Would sample rate and bufffer size frames somehow be related and just a missing single abstraction, or other combinations?
Yes, please note section 9.2 which acknowledges the lack of a proper settings negotiation API. I imagine that this will replace the simple getter/setter interface (hopefully for R3). And yes, sample rate and buffer size may depend on each other. If you use large buffer sizes, you might not be able to simultaneously use large sample rates on some devices.
>
> 4. There seem to be deeper portability issues with the implementation defined types leaking and providing setters for them. What kind of operations would be possible on such a type, how could it be set to a different value than one previously received in a portable way. Why not provide a strong type wrapper (e.g., for sample_rate_t and buffer_size_t) with defined operations instead, so that it is clear, what are supported. Are set/get really the right API, or is a "Change/AdjustBy" a better API. I do not know the domain, so that makes me unsure, but obtaining a value, doing arbitrary arithmetic, then setting a value seems not a good abstraction.
See above. And no, change/adjustBy is not an appropriate API here. The way you set up audio devices is like this:
- you acquire a handle to it
- you negotiate what settings you can use
- then you “start” the device, at which time it starts asynchronously processing stuff. it’s “running” now.
- you can’t change any settings anymore on a “running” device. To do that, it has to be stopped and restarted.
>
> 5. bool queries: is_running(), is_input(), supports_sample_type<T>(), ... -> it seems to me there is a state machine lurking may be, or at least a state space that the device can represent. I think this should be modeled more explicitly, it might provide insights to a better abstraction than a boolean sea.
Yes, see above. It is literally a state machine. It has “idle” and “running” states, and you can do different things during the different states.
How would you suggest to model it? The way we model it is quite similar to existing audio APIs which is why we did it that way.
> 6. 6.8: Why is audio_device_io not a struct. closing angle brackets are missing.
Correct, thanks for spotting! Will fix.
>
> 7. 7.1/7.3 the while(true); is ihmo UB. Could we make it while(device.has_unprocessed_io()); or similarly check the state of it.
Also correct. Will fix.
Cheers,
Timur
Thanks for the great feedback! This is really useful stuff. My replies below.
> On 18 Jun 2019, at 12:13, Peter Sommerlad (CPP) <peter.cpp_at_[hidden] <mailto:peter.cpp_at_[hidden]>> wrote:
> 1. why is the inconsistency in represening counts/sizes between audio_buffer and audio_device? size_t vs. int, naming of member functions.
You mean you don’t like that audio_device::num_input/output_channels() returns an int? Yeah, I guess this should be an unsigned integer. However, should we really use 64-bit size_t considering that there won’t ever be an audio device with millions of audio channels? That’s a serious question. What’s the guidance here?
>
> 2. in 6.5:
> is the iterator return type of audio_device_list define somewhere else? if not, it should at least be a typedef in the list type and specify, that its operator* returns an audio_device (or an optional<audio_device> to make code safer.)
Yes it totally should be. This is not formal wording yet. Note this paragraph in section 6.2:
Note that the API description below is intentionally incomplete so that the signal does not get overwhelmed by the noise. For example, we have left out constructors, destructors, and thorough application of const and noexcept to name but a few items. These functions, as well as more formal wording to specify them, will come in future revisions of this document. Furthermore, all names in this proposal are placeholder names subject to future bikeshedding.
>
> 3. why are the setter functions not noexcept? They return a bool to denote success. I consider the many bool returning functions a kind of design smell. Has anybody considered to employ enums for that? (I understand that often requires to negate logic).
Setting parameters like a buffer size and sample rate can fail. What we need here is a proper error handling mechanism. You are right, those should be noexcept and they should not be returning bools. My current thinking is to rewrite this for R3 to use std::error_code, like std::filesystem does. Does that sound better?
> Also a type with getters and setters is always suspect for me, since it leaks abstractions. Would sample rate and bufffer size frames somehow be related and just a missing single abstraction, or other combinations?
Yes, please note section 9.2 which acknowledges the lack of a proper settings negotiation API. I imagine that this will replace the simple getter/setter interface (hopefully for R3). And yes, sample rate and buffer size may depend on each other. If you use large buffer sizes, you might not be able to simultaneously use large sample rates on some devices.
>
> 4. There seem to be deeper portability issues with the implementation defined types leaking and providing setters for them. What kind of operations would be possible on such a type, how could it be set to a different value than one previously received in a portable way. Why not provide a strong type wrapper (e.g., for sample_rate_t and buffer_size_t) with defined operations instead, so that it is clear, what are supported. Are set/get really the right API, or is a "Change/AdjustBy" a better API. I do not know the domain, so that makes me unsure, but obtaining a value, doing arbitrary arithmetic, then setting a value seems not a good abstraction.
See above. And no, change/adjustBy is not an appropriate API here. The way you set up audio devices is like this:
- you acquire a handle to it
- you negotiate what settings you can use
- then you “start” the device, at which time it starts asynchronously processing stuff. it’s “running” now.
- you can’t change any settings anymore on a “running” device. To do that, it has to be stopped and restarted.
>
> 5. bool queries: is_running(), is_input(), supports_sample_type<T>(), ... -> it seems to me there is a state machine lurking may be, or at least a state space that the device can represent. I think this should be modeled more explicitly, it might provide insights to a better abstraction than a boolean sea.
Yes, see above. It is literally a state machine. It has “idle” and “running” states, and you can do different things during the different states.
How would you suggest to model it? The way we model it is quite similar to existing audio APIs which is why we did it that way.
> 6. 6.8: Why is audio_device_io not a struct. closing angle brackets are missing.
Correct, thanks for spotting! Will fix.
>
> 7. 7.1/7.3 the while(true); is ihmo UB. Could we make it while(device.has_unprocessed_io()); or similarly check the state of it.
Also correct. Will fix.
Cheers,
Timur
Received on 2019-06-18 09:31:05