Refactor Android audio code to clarify use of communication devices [chromium/src : main]
Maya Jelonkiewicz (Gerrit)
Message from Maya Jelonkiewicz![]( "Open in Gerrit")
Set Ready For Review
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
I'm a bit concerned this is muddying the details around input-only device selection -- though it's possible that doesn't work on Android today.
Java_AudioManagerAndroid_getCommunicationDeviceNames(Is it not possible to have an input only device on Android?
Related details
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 1 comment![]( "Open in Gerrit")
// When using streams with communication usage set, the output device is
// automatically selected by the Android framework based on the input device.
// Thus, there is no reason to expose an output device selection.Mirroring Dale's question, how should a user select between multiple output-only devices of the same type? I think we allow 2 BT devices to be connected at once, and some of these don't have microphones.
I know this is an edge-case and might not need to be addressed now (we currently don't), but just something to keep in mind.
Related details
- Maya Jelonkiewicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 3 comments![]( "Open in Gerrit")
I'm a bit concerned this is muddying the details around input-only device selection -- though it's possible that doesn't work on Android today.
Could you elaborate on that? Unless the other comment thread covers this.
Java_AudioManagerAndroid_getCommunicationDeviceNames(Is it not possible to have an input only device on Android?
It is possible, but it won't be listed as a communication device (meaning it's only usable in Clank via the "default device" route).
// When using streams with communication usage set, the output device is
// automatically selected by the Android framework based on the input device.
// Thus, there is no reason to expose an output device selection.Mirroring Dale's question, how should a user select between multiple output-only devices of the same type? I think we allow 2 BT devices to be connected at once, and some of these don't have microphones.
I know this is an edge-case and might not need to be addressed now (we currently don't), but just something to keep in mind.
If the device is output-only, it's not selectable at all, as there won't be a corresponding communication device for it.
Android does allow multiple BT devices to be connected at once, but only one audio device ends up available for use by apps. That also goes for other types like USB. You are right though that this could change. We could definitely enhance the communication device-based audio management scheme to account for this case.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 4 comments![]( "Open in Gerrit")
Maya JelonkiewiczI'm a bit concerned this is muddying the details around input-only device selection -- though it's possible that doesn't work on Android today.
Could you elaborate on that? Unless the other comment thread covers this.
On other platforms communications devices specifically means a paired input/output device. It's possible to have standalone input and output devices. I'm trying to understand if this CL is diverging from that pattern or just bowing to Android's limitations. If the latter it needs a lot more comments on why we want to put communications in the name of things that should be more generic.
Java_AudioManagerAndroid_getCommunicationDeviceNames(Maya JelonkiewiczIs it not possible to have an input only device on Android?
It is possible, but it won't be listed as a communication device (meaning it's only usable in Clank via the "default device" route).
I mean on other platforms this list is generally the list of all selectable devices, not just paired input/output communications devices. Given your other comments it sounds like these aren't selectable / listable though?
// When using streams with communication usage set, the output device is
// automatically selected by the Android framework based on the input device.
// Thus, there is no reason to expose an output device selection.Maya JelonkiewiczMirroring Dale's question, how should a user select between multiple output-only devices of the same type? I think we allow 2 BT devices to be connected at once, and some of these don't have microphones.
I know this is an edge-case and might not need to be addressed now (we currently don't), but just something to keep in mind.
If the device is output-only, it's not selectable at all, as there won't be a corresponding communication device for it.
Android does allow multiple BT devices to be connected at once, but only one audio device ends up available for use by apps. That also goes for other types like USB. You are right though that this could change. We could definitely enhance the communication device-based audio management scheme to account for this case.
On windows the default and the default-communications device are separate things:
https://source.chromium.org/chromium/chromium/src/+/main:media/audio/win/audio_manager_win.cc;l=176;drc=b4dcc60457a349c18efddcd6347cc4e8d27174ab
Is there a reason to diverge here?
if (!SetCommunicationDevice(device_id)) {I believe on Windows we'd only do this if the device_id == default_communications.
Related details
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 4 comments![]( "Open in Gerrit")
Maya JelonkiewiczI'm a bit concerned this is muddying the details around input-only device selection -- though it's possible that doesn't work on Android today.
Dale CurtisCould you elaborate on that? Unless the other comment thread covers this.
On other platforms communications devices specifically means a paired input/output device. It's possible to have standalone input and output devices. I'm trying to understand if this CL is diverging from that pattern or just bowing to Android's limitations. If the latter it needs a lot more comments on why we want to put communications in the name of things that should be more generic.
I should underline first and foremost that this CL doesn't introduce behavior changes, just a refactor. I am following the existing implementation.
We are indeed bowing to Android's limitations here. For one, on SDK < 29 we fallback to OpenSL ES, which doesn't expose per-stream device selection, so managing the system communication device is the only option to influence the device used by a stream. AFAIK Clank targets SDK 26+, and AAudio is also 26+, so we could perhaps drop OpenSL ES support, but there is also another reason - vendor devices may or may not respect per-stream device selection (e.g. they may route all streams with the same usage to one device, or exhibit clashes when using two seperate audio devices, and this might even happen silently). On the other hand, there is good support for the communication usage path, to allow for device selection in voice/video call apps. This is why the communication device-based approach for device selection is relevant and will need to stay around, even with per-stream device selection implemented.
Java_AudioManagerAndroid_getCommunicationDeviceNames(Maya JelonkiewiczIs it not possible to have an input only device on Android?
Dale CurtisIt is possible, but it won't be listed as a communication device (meaning it's only usable in Clank via the "default device" route).
I mean on other platforms this list is generally the list of all selectable devices, not just paired input/output communications devices. Given your other comments it sounds like these aren't selectable / listable though?
Yes, specifically, they are not currently selectable in Clank; they are in general selectable for Android apps, but with the caveats I listed in another comment. I'll be introducing selection of all devices in upcoming changes.
// When using streams with communication usage set, the output device is
// automatically selected by the Android framework based on the input device.
// Thus, there is no reason to expose an output device selection.Maya JelonkiewiczMirroring Dale's question, how should a user select between multiple output-only devices of the same type? I think we allow 2 BT devices to be connected at once, and some of these don't have microphones.
I know this is an edge-case and might not need to be addressed now (we currently don't), but just something to keep in mind.
Dale CurtisIf the device is output-only, it's not selectable at all, as there won't be a corresponding communication device for it.
Android does allow multiple BT devices to be connected at once, but only one audio device ends up available for use by apps. That also goes for other types like USB. You are right though that this could change. We could definitely enhance the communication device-based audio management scheme to account for this case.
On windows the default and the default-communications device are separate things:
https://source.chromium.org/chromium/chromium/src/+/main:media/audio/win/audio_manager_win.cc;l=176;drc=b4dcc60457a349c18efddcd6347cc4e8d27174abIs there a reason to diverge here?
This is a decision made 10 years ago. I wouldn't be opposed to changing it - possibly in a seperate CL?
I assume you mean having a device with ID "communications" *instead of* one with ID "default"? My only potential concern would be if there is some part of Chrome that assumes there always is a "default" device [if the list is not empty]? Otherwise, that seems reasonable to me.
if (!SetCommunicationDevice(device_id)) {I believe on Windows we'd only do this if the device_id == default_communications.
Sorry, not sure if I understand. Do you mean !=, i.e. we wouldn't call this for a default device? If so, it is because in that case we have custom logic to pick a default device (AudioDeviceSelector#selectDefaultDevice). This could also be worth considering changing.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 4 comments![]( "Open in Gerrit")
Maya JelonkiewiczI'm a bit concerned this is muddying the details around input-only device selection -- though it's possible that doesn't work on Android today.
Dale CurtisCould you elaborate on that? Unless the other comment thread covers this.
Maya JelonkiewiczOn other platforms communications devices specifically means a paired input/output device. It's possible to have standalone input and output devices. I'm trying to understand if this CL is diverging from that pattern or just bowing to Android's limitations. If the latter it needs a lot more comments on why we want to put communications in the name of things that should be more generic.
I should underline first and foremost that this CL doesn't introduce behavior changes, just a refactor. I am following the existing implementation.
We are indeed bowing to Android's limitations here. For one, on SDK < 29 we fallback to OpenSL ES, which doesn't expose per-stream device selection, so managing the system communication device is the only option to influence the device used by a stream. AFAIK Clank targets SDK 26+, and AAudio is also 26+, so we could perhaps drop OpenSL ES support, but there is also another reason - vendor devices may or may not respect per-stream device selection (e.g. they may route all streams with the same usage to one device, or exhibit clashes when using two seperate audio devices, and this might even happen silently). On the other hand, there is good support for the communication usage path, to allow for device selection in voice/video call apps. This is why the communication device-based approach for device selection is relevant and will need to stay around, even with per-stream device selection implemented.
It sounds like you're saying this should work with AAudio and compliant devices; of which I'd expect AL to be one?
If that's the case, I don't think we should rename the classes, but I support renaming methods to make the limitations clearer relative to our ideals. I.e., somehwere down the line AudioDeviceSelector would work for non-communication devices -- which will be a long tail requirement for AL I'd expect.
I'm a bit unclear on how this interacts with whatever value we pass to AAudio though. Is it the case that for a working AAudio impl we shouldn't call this at all and should only pass the device id to AAudio? I'd be more supportive of renaming the classes in this case since this is effectively just the fallback path for broken per-stream selection.
Java_AudioManagerAndroid_getCommunicationDeviceNames(Maya JelonkiewiczIs it not possible to have an input only device on Android?
Dale CurtisIt is possible, but it won't be listed as a communication device (meaning it's only usable in Clank via the "default device" route).
Maya JelonkiewiczI mean on other platforms this list is generally the list of all selectable devices, not just paired input/output communications devices. Given your other comments it sounds like these aren't selectable / listable though?
Yes, specifically, they are not currently selectable in Clank; they are in general selectable for Android apps, but with the caveats I listed in another comment. I'll be introducing selection of all devices in upcoming changes.
Acknowledged
// When using streams with communication usage set, the output device is
// automatically selected by the Android framework based on the input device.
// Thus, there is no reason to expose an output device selection.Maya JelonkiewiczMirroring Dale's question, how should a user select between multiple output-only devices of the same type? I think we allow 2 BT devices to be connected at once, and some of these don't have microphones.
I know this is an edge-case and might not need to be addressed now (we currently don't), but just something to keep in mind.
Dale CurtisIf the device is output-only, it's not selectable at all, as there won't be a corresponding communication device for it.
Android does allow multiple BT devices to be connected at once, but only one audio device ends up available for use by apps. That also goes for other types like USB. You are right though that this could change. We could definitely enhance the communication device-based audio management scheme to account for this case.
Maya JelonkiewiczOn windows the default and the default-communications device are separate things:
https://source.chromium.org/chromium/chromium/src/+/main:media/audio/win/audio_manager_win.cc;l=176;drc=b4dcc60457a349c18efddcd6347cc4e8d27174abIs there a reason to diverge here?
This is a decision made 10 years ago. I wouldn't be opposed to changing it - possibly in a seperate CL?
I assume you mean having a device with ID "communications" *instead of* one with ID "default"? My only potential concern would be if there is some part of Chrome that assumes there always is a "default" device [if the list is not empty]? Otherwise, that seems reasonable to me.
I'm inclined not to change it without strong reasons, since it'll result in surprises for developers / users.
I think we always expect the default output device to work even if the list is entry, but I haven't audited all the managers to ensure they add a device there.
if (!SetCommunicationDevice(device_id)) {Maya JelonkiewiczI believe on Windows we'd only do this if the device_id == default_communications.
Sorry, not sure if I understand. Do you mean !=, i.e. we wouldn't call this for a default device? If so, it is because in that case we have custom logic to pick a default device (AudioDeviceSelector#selectDefaultDevice). This could also be worth considering changing.
Yeah for the default device I don't think we'd call anything, just let the output stream select whatever happens to be default. I think I misspoke though, on Windows we'd just tell the OS to create the default stream for role type "communications" when the device_id == default_communications.
For explicit device_id we'd want to always set this as needed.
Related details
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 2 comments![]( "Open in Gerrit")
Maya JelonkiewiczI'm a bit concerned this is muddying the details around input-only device selection -- though it's possible that doesn't work on Android today.
Dale CurtisCould you elaborate on that? Unless the other comment thread covers this.
Maya JelonkiewiczOn other platforms communications devices specifically means a paired input/output device. It's possible to have standalone input and output devices. I'm trying to understand if this CL is diverging from that pattern or just bowing to Android's limitations. If the latter it needs a lot more comments on why we want to put communications in the name of things that should be more generic.
Dale CurtisI should underline first and foremost that this CL doesn't introduce behavior changes, just a refactor. I am following the existing implementation.
We are indeed bowing to Android's limitations here. For one, on SDK < 29 we fallback to OpenSL ES, which doesn't expose per-stream device selection, so managing the system communication device is the only option to influence the device used by a stream. AFAIK Clank targets SDK 26+, and AAudio is also 26+, so we could perhaps drop OpenSL ES support, but there is also another reason - vendor devices may or may not respect per-stream device selection (e.g. they may route all streams with the same usage to one device, or exhibit clashes when using two seperate audio devices, and this might even happen silently). On the other hand, there is good support for the communication usage path, to allow for device selection in voice/video call apps. This is why the communication device-based approach for device selection is relevant and will need to stay around, even with per-stream device selection implemented.
It sounds like you're saying this should work with AAudio and compliant devices; of which I'd expect AL to be one?
If that's the case, I don't think we should rename the classes, but I support renaming methods to make the limitations clearer relative to our ideals. I.e., somehwere down the line AudioDeviceSelector would work for non-communication devices -- which will be a long tail requirement for AL I'd expect.
I'm a bit unclear on how this interacts with whatever value we pass to AAudio though. Is it the case that for a working AAudio impl we shouldn't call this at all and should only pass the device id to AAudio? I'd be more supportive of renaming the classes in this case since this is effectively just the fallback path for broken per-stream selection.
I do expect that we won't be using the renamed classes for "proper" per-stream device selection. It is indeed mostly be a matter of passing the device ID to AAudio.
The AudioDeviceListener I think is best replaced by a solution using AudioManager#registerAudioDeviceCallback (like https://crrev.com/c/6309250) - this automatically scales across various types of devices (incl. HDMI, which we don't even currently listen to changes for) and removes a dependency on the BT permission.
The AudioDeviceSelector has logic relating to communication device management that I expect we shouldn't need (https://crrev.com/c/6299052). It also has inconvenient baggage from needing to support the older communication device APIs used in AudioDeviceSelectorPreS.
(BTW: go/android-desktop-code?)
if (!SetCommunicationDevice(device_id)) {Maya JelonkiewiczI believe on Windows we'd only do this if the device_id == default_communications.
Dale CurtisSorry, not sure if I understand. Do you mean !=, i.e. we wouldn't call this for a default device? If so, it is because in that case we have custom logic to pick a default device (AudioDeviceSelector#selectDefaultDevice). This could also be worth considering changing.
Yeah for the default device I don't think we'd call anything, just let the output stream select whatever happens to be default. I think I misspoke though, on Windows we'd just tell the OS to create the default stream for role type "communications" when the device_id == default_communications.
For explicit device_id we'd want to always set this as needed.
What you describe does seem more logical. I don't think there is an explicit need to specify a communication device on Android; we should be able to use whatever the system decides by default just as on Windows.
However, if you are concerned that changing how the default device is listed could be a surprising change, it could similarly be argued that changing this could be surprising - users may be accustomed to the selectDefaultDevice logic. IMO this is a matter to consider separately from this CL, if at all, though I can document the current behavior better.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
3 comments
Maya JelonkiewiczI'm a bit concerned this is muddying the details around input-only device selection -- though it's possible that doesn't work on Android today.
Dale CurtisCould you elaborate on that? Unless the other comment thread covers this.
Maya JelonkiewiczOn other platforms communications devices specifically means a paired input/output device. It's possible to have standalone input and output devices. I'm trying to understand if this CL is diverging from that pattern or just bowing to Android's limitations. If the latter it needs a lot more comments on why we want to put communications in the name of things that should be more generic.
Dale CurtisI should underline first and foremost that this CL doesn't introduce behavior changes, just a refactor. I am following the existing implementation.
We are indeed bowing to Android's limitations here. For one, on SDK < 29 we fallback to OpenSL ES, which doesn't expose per-stream device selection, so managing the system communication device is the only option to influence the device used by a stream. AFAIK Clank targets SDK 26+, and AAudio is also 26+, so we could perhaps drop OpenSL ES support, but there is also another reason - vendor devices may or may not respect per-stream device selection (e.g. they may route all streams with the same usage to one device, or exhibit clashes when using two seperate audio devices, and this might even happen silently). On the other hand, there is good support for the communication usage path, to allow for device selection in voice/video call apps. This is why the communication device-based approach for device selection is relevant and will need to stay around, even with per-stream device selection implemented.
Maya JelonkiewiczIt sounds like you're saying this should work with AAudio and compliant devices; of which I'd expect AL to be one?
If that's the case, I don't think we should rename the classes, but I support renaming methods to make the limitations clearer relative to our ideals. I.e., somehwere down the line AudioDeviceSelector would work for non-communication devices -- which will be a long tail requirement for AL I'd expect.
I'm a bit unclear on how this interacts with whatever value we pass to AAudio though. Is it the case that for a working AAudio impl we shouldn't call this at all and should only pass the device id to AAudio? I'd be more supportive of renaming the classes in this case since this is effectively just the fallback path for broken per-stream selection.
I do expect that we won't be using the renamed classes for "proper" per-stream device selection. It is indeed mostly be a matter of passing the device ID to AAudio.
The AudioDeviceListener I think is best replaced by a solution using AudioManager#registerAudioDeviceCallback (like https://crrev.com/c/6309250) - this automatically scales across various types of devices (incl. HDMI, which we don't even currently listen to changes for) and removes a dependency on the BT permission.
The AudioDeviceSelector has logic relating to communication device management that I expect we shouldn't need (https://crrev.com/c/6299052). It also has inconvenient baggage from needing to support the older communication device APIs used in AudioDeviceSelectorPreS.
(BTW: go/android-desktop-code?)
Okay, if this class is a workaround for cases where we can't properly set the device id, naming it around this sgtm. Thanks for the details.
Rename lgtm
if (!SetCommunicationDevice(device_id)) {Maya JelonkiewiczI believe on Windows we'd only do this if the device_id == default_communications.
Dale CurtisSorry, not sure if I understand. Do you mean !=, i.e. we wouldn't call this for a default device? If so, it is because in that case we have custom logic to pick a default device (AudioDeviceSelector#selectDefaultDevice). This could also be worth considering changing.
Maya JelonkiewiczYeah for the default device I don't think we'd call anything, just let the output stream select whatever happens to be default. I think I misspoke though, on Windows we'd just tell the OS to create the default stream for role type "communications" when the device_id == default_communications.
For explicit device_id we'd want to always set this as needed.
What you describe does seem more logical. I don't think there is an explicit need to specify a communication device on Android; we should be able to use whatever the system decides by default just as on Windows.
However, if you are concerned that changing how the default device is listed could be a surprising change, it could similarly be argued that changing this could be surprising - users may be accustomed to the selectDefaultDevice logic. IMO this is a matter to consider separately from this CL, if at all, though I can document the current behavior better.
Changing how we list the devices could be an issue, but it's always hard to tell, especially on Android which doesn't have as large a tapestry of mweb RTC applications.
Related details
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 2 comments![]( "Open in Gerrit")
// When using streams with communication usage set, the output device is
// automatically selected by the Android framework based on the input device.
// Thus, there is no reason to expose an output device selection.Maya JelonkiewiczMirroring Dale's question, how should a user select between multiple output-only devices of the same type? I think we allow 2 BT devices to be connected at once, and some of these don't have microphones.
I know this is an edge-case and might not need to be addressed now (we currently don't), but just something to keep in mind.
Dale CurtisIf the device is output-only, it's not selectable at all, as there won't be a corresponding communication device for it.
Android does allow multiple BT devices to be connected at once, but only one audio device ends up available for use by apps. That also goes for other types like USB. You are right though that this could change. We could definitely enhance the communication device-based audio management scheme to account for this case.
Maya JelonkiewiczOn windows the default and the default-communications device are separate things:
https://source.chromium.org/chromium/chromium/src/+/main:media/audio/win/audio_manager_win.cc;l=176;drc=b4dcc60457a349c18efddcd6347cc4e8d27174abIs there a reason to diverge here?
Dale CurtisThis is a decision made 10 years ago. I wouldn't be opposed to changing it - possibly in a seperate CL?
I assume you mean having a device with ID "communications" *instead of* one with ID "default"? My only potential concern would be if there is some part of Chrome that assumes there always is a "default" device [if the list is not empty]? Otherwise, that seems reasonable to me.
I'm inclined not to change it without strong reasons, since it'll result in surprises for developers / users.
I think we always expect the default output device to work even if the list is entry, but I haven't audited all the managers to ensure they add a device there.
I reworded the comment to be a bit more clear, but other than that, I suppose we are not changing the behavior here? LMK if there is anything more to do here.
if (!SetCommunicationDevice(device_id)) {Maya JelonkiewiczI believe on Windows we'd only do this if the device_id == default_communications.
Dale CurtisSorry, not sure if I understand. Do you mean !=, i.e. we wouldn't call this for a default device? If so, it is because in that case we have custom logic to pick a default device (AudioDeviceSelector#selectDefaultDevice). This could also be worth considering changing.
Maya JelonkiewiczYeah for the default device I don't think we'd call anything, just let the output stream select whatever happens to be default. I think I misspoke though, on Windows we'd just tell the OS to create the default stream for role type "communications" when the device_id == default_communications.
For explicit device_id we'd want to always set this as needed.
Dale CurtisWhat you describe does seem more logical. I don't think there is an explicit need to specify a communication device on Android; we should be able to use whatever the system decides by default just as on Windows.
However, if you are concerned that changing how the default device is listed could be a surprising change, it could similarly be argued that changing this could be surprising - users may be accustomed to the selectDefaultDevice logic. IMO this is a matter to consider separately from this CL, if at all, though I can document the current behavior better.
Changing how we list the devices could be an issue, but it's always hard to tell, especially on Android which doesn't have as large a tapestry of mweb RTC applications.
In this case it would be the semantics of what the default device means that would change. But I assume the same applies.
I clarified the default device behavior in the comment. LMK if there is anything more to do here.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 1 comment![]( "Open in Gerrit")
I see I have in fact made a mistake and flipped the semantics of communication devices - they are output devices for which an input device is automatically selected, rather than the other way around. I've been going along with how in Clank we expose communication devices as input devices, and did not correctly verify this fact. My apologies.
This means that us listing communication devices as inputs and just providing a "default" for outputs is actually quite peculiar, and it should really be the other way around. I don't know if this is something worth changing at this point; while this is "backwards" internally, it's probably meaningless for the end user.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 1 comment![]( "Open in Gerrit")
I see I have in fact made a mistake and flipped the semantics of communication devices - they are output devices for which an input device is automatically selected, rather than the other way around. I've been going along with how in Clank we expose communication devices as input devices, and did not correctly verify this fact. My apologies.
This means that us listing communication devices as inputs and just providing a "default" for outputs is actually quite peculiar, and it should really be the other way around. I don't know if this is something worth changing at this point; while this is "backwards" internally, it's probably meaningless for the end user.
Marking unresolved.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 1 comment![]( "Open in Gerrit")
Maya JelonkiewiczI see I have in fact made a mistake and flipped the semantics of communication devices - they are output devices for which an input device is automatically selected, rather than the other way around. I've been going along with how in Clank we expose communication devices as input devices, and did not correctly verify this fact. My apologies.
This means that us listing communication devices as inputs and just providing a "default" for outputs is actually quite peculiar, and it should really be the other way around. I don't know if this is something worth changing at this point; while this is "backwards" internally, it's probably meaningless for the end user.
Marking unresolved.
Patchset 3 addresses this, for now with the assumption that we won't change the behavior.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 3 comments![]( "Open in Gerrit")
// meant to return input devices, this is inverted for compatibility withI think this would be better phrased as a
```
// TODO(bug): Currently this returns the list of output communication devices
// since selecting individual devices doesn't work on many Android devices. We
// are only able to reliably select communications devices.
```
Since IIUC your follow up patch sets will fix this?
// When using streams with communication usage set, the output device is
// automatically selected by the Android framework based on the input device.
// Thus, there is no reason to expose an output device selection.Maya JelonkiewiczMirroring Dale's question, how should a user select between multiple output-only devices of the same type? I think we allow 2 BT devices to be connected at once, and some of these don't have microphones.
I know this is an edge-case and might not need to be addressed now (we currently don't), but just something to keep in mind.
Dale CurtisIf the device is output-only, it's not selectable at all, as there won't be a corresponding communication device for it.
Android does allow multiple BT devices to be connected at once, but only one audio device ends up available for use by apps. That also goes for other types like USB. You are right though that this could change. We could definitely enhance the communication device-based audio management scheme to account for this case.
Maya JelonkiewiczOn windows the default and the default-communications device are separate things:
https://source.chromium.org/chromium/chromium/src/+/main:media/audio/win/audio_manager_win.cc;l=176;drc=b4dcc60457a349c18efddcd6347cc4e8d27174abIs there a reason to diverge here?
Dale CurtisThis is a decision made 10 years ago. I wouldn't be opposed to changing it - possibly in a seperate CL?
I assume you mean having a device with ID "communications" *instead of* one with ID "default"? My only potential concern would be if there is some part of Chrome that assumes there always is a "default" device [if the list is not empty]? Otherwise, that seems reasonable to me.
Maya JelonkiewiczI'm inclined not to change it without strong reasons, since it'll result in surprises for developers / users.
I think we always expect the default output device to work even if the list is entry, but I haven't audited all the managers to ensure they add a device there.
I reworded the comment to be a bit more clear, but other than that, I suppose we are not changing the behavior here? LMK if there is anything more to do here.
So long as we don't change behavior in this CL I don't have any issues with naming to make things more clear.
// selection to be made for the input device. Additionally, in order toAs above, lets drop the preserving legacy behavior (which I don't think we want to preserve in cases where selecting devices actually works) and rephrase as a TODO(): Populate output device list.
Related details
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 1 comment![]( "Open in Gerrit")
// meant to return input devices, this is inverted for compatibility withI think this would be better phrased as a
```
// TODO(bug): Currently this returns the list of output communication devices
// since selecting individual devices doesn't work on many Android devices. We
// are only able to reliably select communications devices.
```Since IIUC your follow up patch sets will fix this?
Per-stream device selection will change this, but we will need to preserve the current implementation (or at least some version of it), because of a) the OpenSL ES fallback for SDK < 29, and b) the lack of general support across all Android devices for arbitrarily routing to audio devices, which may cause surprises like no sound output or a different device being used than expected.
I'm not saying we would *permanently* have this divergent implementation, but IMO we can't confidently get rid of communication stream based routing without a lot more work (at minimum testing across a multitude of devices).
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
// meant to return input devices, this is inverted for compatibility withMaya JelonkiewiczI think this would be better phrased as a
```
// TODO(bug): Currently this returns the list of output communication devices
// since selecting individual devices doesn't work on many Android devices. We
// are only able to reliably select communications devices.
```Since IIUC your follow up patch sets will fix this?
Per-stream device selection will change this, but we will need to preserve the current implementation (or at least some version of it), because of a) the OpenSL ES fallback for SDK < 29, and b) the lack of general support across all Android devices for arbitrarily routing to audio devices, which may cause surprises like no sound output or a different device being used than expected.
I'm not saying we would *permanently* have this divergent implementation, but IMO we can't confidently get rid of communication stream based routing without a lot more work (at minimum testing across a multitude of devices).
Do you think we could say that after a given Android version things would be expected to work properly?
Related details
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 1 comment![]( "Open in Gerrit")
// meant to return input devices, this is inverted for compatibility withMaya JelonkiewiczI think this would be better phrased as a
```
// TODO(bug): Currently this returns the list of output communication devices
// since selecting individual devices doesn't work on many Android devices. We
// are only able to reliably select communications devices.
```Since IIUC your follow up patch sets will fix this?
Dale CurtisPer-stream device selection will change this, but we will need to preserve the current implementation (or at least some version of it), because of a) the OpenSL ES fallback for SDK < 29, and b) the lack of general support across all Android devices for arbitrarily routing to audio devices, which may cause surprises like no sound output or a different device being used than expected.
I'm not saying we would *permanently* have this divergent implementation, but IMO we can't confidently get rid of communication stream based routing without a lot more work (at minimum testing across a multitude of devices).
Do you think we could say that after a given Android version things would be expected to work properly?
Maybe in the future, but I don't think we could point to such a version right now. The Android team is leaning towards the opposite if anything - they discourage apps from selecting devices as opposed to just specifying a usage, even despite the existence of APIs to specify a device ID. At the same time that is counter to what we want for desktop 😕.
I wouldn't be 100% opposed to launching this change on all Android devices, but erring on the side of caution, I'd have concerns that on some devices it would be an overall regression. The Android audio TL was pessimistic here, noting vendors may introduce limits on simultaneous device use, and that AAudio device selection can silently fail. How much of a problem this is in practice, I don't have good data on. We can discuss this over GVC.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
// meant to return input devices, this is inverted for compatibility withMaya JelonkiewiczI think this would be better phrased as a
```
// TODO(bug): Currently this returns the list of output communication devices
// since selecting individual devices doesn't work on many Android devices. We
// are only able to reliably select communications devices.
```Since IIUC your follow up patch sets will fix this?
Dale CurtisPer-stream device selection will change this, but we will need to preserve the current implementation (or at least some version of it), because of a) the OpenSL ES fallback for SDK < 29, and b) the lack of general support across all Android devices for arbitrarily routing to audio devices, which may cause surprises like no sound output or a different device being used than expected.
I'm not saying we would *permanently* have this divergent implementation, but IMO we can't confidently get rid of communication stream based routing without a lot more work (at minimum testing across a multitude of devices).
Maya JelonkiewiczDo you think we could say that after a given Android version things would be expected to work properly?
Maybe in the future, but I don't think we could point to such a version right now. The Android team is leaning towards the opposite if anything - they discourage apps from selecting devices as opposed to just specifying a usage, even despite the existence of APIs to specify a device ID. At the same time that is counter to what we want for desktop 😕.
I wouldn't be 100% opposed to launching this change on all Android devices, but erring on the side of caution, I'd have concerns that on some devices it would be an overall regression. The Android audio TL was pessimistic here, noting vendors may introduce limits on simultaneous device use, and that AAudio device selection can silently fail. How much of a problem this is in practice, I don't have good data on. We can discuss this over GVC.
I chatted with Thomas and he seemed to think we are able to set the communications device previously. I'm not as familiar with this code, so am deferring to his knowledge here. I've setup a meeting for us all to sync tomorrow.
Related details
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
// meant to return input devices, this is inverted for compatibility withMaya JelonkiewiczI think this would be better phrased as a
```
// TODO(bug): Currently this returns the list of output communication devices
// since selecting individual devices doesn't work on many Android devices. We
// are only able to reliably select communications devices.
```Since IIUC your follow up patch sets will fix this?
Dale CurtisPer-stream device selection will change this, but we will need to preserve the current implementation (or at least some version of it), because of a) the OpenSL ES fallback for SDK < 29, and b) the lack of general support across all Android devices for arbitrarily routing to audio devices, which may cause surprises like no sound output or a different device being used than expected.
I'm not saying we would *permanently* have this divergent implementation, but IMO we can't confidently get rid of communication stream based routing without a lot more work (at minimum testing across a multitude of devices).
Maya JelonkiewiczDo you think we could say that after a given Android version things would be expected to work properly?
Dale CurtisMaybe in the future, but I don't think we could point to such a version right now. The Android team is leaning towards the opposite if anything - they discourage apps from selecting devices as opposed to just specifying a usage, even despite the existence of APIs to specify a device ID. At the same time that is counter to what we want for desktop 😕.
I wouldn't be 100% opposed to launching this change on all Android devices, but erring on the side of caution, I'd have concerns that on some devices it would be an overall regression. The Android audio TL was pessimistic here, noting vendors may introduce limits on simultaneous device use, and that AAudio device selection can silently fail. How much of a problem this is in practice, I don't have good data on. We can discuss this over GVC.
I chatted with Thomas and he seemed to think we are able to set the communications device previously. I'm not as familiar with this code, so am deferring to his knowledge here. I've setup a meeting for us all to sync tomorrow.
Sorry that should read: "set non-communications devices"
Related details
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 1 comment![]( "Open in Gerrit")
// meant to return input devices, this is inverted for compatibility withMaya JelonkiewiczI think this would be better phrased as a
```
// TODO(bug): Currently this returns the list of output communication devices
// since selecting individual devices doesn't work on many Android devices. We
// are only able to reliably select communications devices.
```Since IIUC your follow up patch sets will fix this?
Dale CurtisPer-stream device selection will change this, but we will need to preserve the current implementation (or at least some version of it), because of a) the OpenSL ES fallback for SDK < 29, and b) the lack of general support across all Android devices for arbitrarily routing to audio devices, which may cause surprises like no sound output or a different device being used than expected.
I'm not saying we would *permanently* have this divergent implementation, but IMO we can't confidently get rid of communication stream based routing without a lot more work (at minimum testing across a multitude of devices).
Maya JelonkiewiczDo you think we could say that after a given Android version things would be expected to work properly?
Dale CurtisMaybe in the future, but I don't think we could point to such a version right now. The Android team is leaning towards the opposite if anything - they discourage apps from selecting devices as opposed to just specifying a usage, even despite the existence of APIs to specify a device ID. At the same time that is counter to what we want for desktop 😕.
I wouldn't be 100% opposed to launching this change on all Android devices, but erring on the side of caution, I'd have concerns that on some devices it would be an overall regression. The Android audio TL was pessimistic here, noting vendors may introduce limits on simultaneous device use, and that AAudio device selection can silently fail. How much of a problem this is in practice, I don't have good data on. We can discuss this over GVC.
Dale CurtisI chatted with Thomas and he seemed to think we are able to set the communications device previously. I'm not as familiar with this code, so am deferring to his knowledge here. I've setup a meeting for us all to sync tomorrow.
Sorry that should read: "set non-communications devices"
Pre-S, I think we only explicitly set BT and speakerphone, and use the default device for the rest, since [this code](https://source.chromium.org/chromium/chromium/src/+/main:media/base/android/java/src/org/chromium/media/AudioDeviceSelectorPreS.java;l=220;drc=f2db5409f4065feeb5525a59fea56cc48298d9db) doesn't do much. The default device is selected by the OS based on some internal scheme, which matches the [priorities outlined here](https://source.chromium.org/chromium/chromium/src/+/main:media/base/android/java/src/org/chromium/media/AudioDeviceSelectorPreS.java;l=95;drc=f2db5409f4065feeb5525a59fea56cc48298d9db) (I think...). I don't know if the Pre-S OS distinguishes whether these devices have a microphone (and are "communication devices") or are just outputs, and whether the "default" is re-selected when we turn communication mode on or off.
Related details
- Maya Jelonkiewicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 1 comment![]( "Open in Gerrit")
// meant to return input devices, this is inverted for compatibility withMaya JelonkiewiczI think this would be better phrased as a
```
// TODO(bug): Currently this returns the list of output communication devices
// since selecting individual devices doesn't work on many Android devices. We
// are only able to reliably select communications devices.
```Since IIUC your follow up patch sets will fix this?
Dale CurtisPer-stream device selection will change this, but we will need to preserve the current implementation (or at least some version of it), because of a) the OpenSL ES fallback for SDK < 29, and b) the lack of general support across all Android devices for arbitrarily routing to audio devices, which may cause surprises like no sound output or a different device being used than expected.
I'm not saying we would *permanently* have this divergent implementation, but IMO we can't confidently get rid of communication stream based routing without a lot more work (at minimum testing across a multitude of devices).
Maya JelonkiewiczDo you think we could say that after a given Android version things would be expected to work properly?
Dale CurtisMaybe in the future, but I don't think we could point to such a version right now. The Android team is leaning towards the opposite if anything - they discourage apps from selecting devices as opposed to just specifying a usage, even despite the existence of APIs to specify a device ID. At the same time that is counter to what we want for desktop 😕.
I wouldn't be 100% opposed to launching this change on all Android devices, but erring on the side of caution, I'd have concerns that on some devices it would be an overall regression. The Android audio TL was pessimistic here, noting vendors may introduce limits on simultaneous device use, and that AAudio device selection can silently fail. How much of a problem this is in practice, I don't have good data on. We can discuss this over GVC.
Dale CurtisI chatted with Thomas and he seemed to think we are able to set the communications device previously. I'm not as familiar with this code, so am deferring to his knowledge here. I've setup a meeting for us all to sync tomorrow.
Thomas GuilbertSorry that should read: "set non-communications devices"
Pre-S, I think we only explicitly set BT and speakerphone, and use the default device for the rest, since [this code](https://source.chromium.org/chromium/chromium/src/+/main:media/base/android/java/src/org/chromium/media/AudioDeviceSelectorPreS.java;l=220;drc=f2db5409f4065feeb5525a59fea56cc48298d9db) doesn't do much. The default device is selected by the OS based on some internal scheme, which matches the [priorities outlined here](https://source.chromium.org/chromium/chromium/src/+/main:media/base/android/java/src/org/chromium/media/AudioDeviceSelectorPreS.java;l=95;drc=f2db5409f4065feeb5525a59fea56cc48298d9db) (I think...). I don't know if the Pre-S OS distinguishes whether these devices have a microphone (and are "communication devices") or are just outputs, and whether the "default" is re-selected when we turn communication mode on or off.
I'm also not sure fully sure how the deprecated calls on pre-S behave, but they don't seem to be providing anything more than setCommunicationDevice.
Following my correction about communication devices being output devices, I've been able to find an output-only device and find that it *is* possible to select an output device without an associated mic via setCommunicationDevice. In this case the system selects some other mic for input communication streams. But standalone mics or combinations like builtin mic in + headset out are not selectable for communication streams.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 7 comments![]( "Open in Gerrit")
Thanks again for meeting earlier. Here's some proposed changes, based on my current understanding (which has been greatly updated over the course of this CL and this discussion), so please push-back on anything that doesn't seem right.
I propose the following comment:
```
Android doesn't allow control over setting an individual input and output
device, and forces us to set a single device with an input/output pair
(a.k.a. a "communication device").
The names we receive below are enums abstracting the exact model names and
device IDs (e.g. "BT Headset" instead of "FooBuds Pro 2.0").
On Android S+, these should be proper communication devices, with an
associated input.
On Android R-, these devices could *potentially* be output only, but it's
not clear wether this is a real issue, considering how long this code has
been around for...
TODO(bug): Expose specific model names here, and allow per-stream device input selection.
```
// selection to be made for the input device. Additionally, in order toAs above, lets drop the preserving legacy behavior (which I don't think we want to preserve in cases where selecting devices actually works) and rephrase as a TODO(): Populate output device list.
Proposed comment:
```
Android doesn't allow control over setting an individual input and output
device, and forces us to set a single device with an input/output pair
(a.k.a. a "communication device").
We've only returned "default" here for quite some time, forcing output
device selection to happen through selecting the system-wide input
device (see `GetAudioInputDeviceNames()`).
Setting the output via the input device is backwards, and could be updated.
However, only returning "default" here has prevented confusion for users:
populating this list would've given users the option to set a different input
and output devices, which would've failed.
TODO(bug): populate `device_names` with the real list of output devices
and allow per-stream device selection.
```
private AudioDeviceName @Nullable [] getCommunicationDeviceNames() {This returns the "synthetic" names, which might not be "communication" devices on Pre-S. I would consider renaming this function to `getConnectedDeviceNames()`, `getAvailableDeviceNames()`, or `getGenericDeviceNames()`, then also mention that the specific device model names are replaced with a generic name for that category.
class CommunicationDeviceListener {I would advocate for leaving this class as the AudioDeviceListener, since it can listen for output-only devices IIUC.
If `OnCommunicationDeviceChangedListener()` is used for S+, we could simplify the S+ code to no longer depend on this class, or then create a "proper" `CommunicationDeviceListener`.
abstract class CommunicationDeviceSelector {Could you add a comment mentioning that a communication device is a device with paired input/ouput.
class CommunicationDeviceSelectorPreS extends CommunicationDeviceSelector {Could you add a comment mentioning that Pre-S, there isn't the concept of "communication device", but device selection effectively behaves the same way, and is emulated by this class? E.g., selecting a device forces the OS to select both the input and the output at once.
Related details
- Dale Curtis
- Maya Jelonkiewicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
1 comment
- Maya Jelonkiewicz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 1 comment![]( "Open in Gerrit")
private AudioDeviceName @Nullable [] getCommunicationDeviceNames() {This returns the "synthetic" names, which might not be "communication" devices on Pre-S. I would consider renaming this function to `getConnectedDeviceNames()`, `getAvailableDeviceNames()`, or `getGenericDeviceNames()`, then also mention that the specific device model names are replaced with a generic name for that category.
Hmm, I'd be concerned that introducing a new name on top of this may be redundant complexity. From one perspective, pre-S is "simulating" post-S communication device management, and in the end, both sets of APIs are controlling which device is meant to be used for communication-usage streams.
Alternatively, if we change it here, I guess we'd want to change it everywhere else that is relevant to match? "Generic device" seems fair to me, or maybe indeed "synthetic device".
The "Name(s)" part is misleading, for sure. AudioDeviceName wraps a name and an ID, and for per-stream device selection we will probably also store other device metadata there, like the device type. This probably stemmed from `media::AudioDeviceName`, which is also a container for name & ID.
Related details
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 4 comments![]( "Open in Gerrit")
if (!SetCommunicationDevice(device_id)) {Maya JelonkiewiczI believe on Windows we'd only do this if the device_id == default_communications.
Dale CurtisSorry, not sure if I understand. Do you mean !=, i.e. we wouldn't call this for a default device? If so, it is because in that case we have custom logic to pick a default device (AudioDeviceSelector#selectDefaultDevice). This could also be worth considering changing.
Maya JelonkiewiczYeah for the default device I don't think we'd call anything, just let the output stream select whatever happens to be default. I think I misspoke though, on Windows we'd just tell the OS to create the default stream for role type "communications" when the device_id == default_communications.
For explicit device_id we'd want to always set this as needed.
Dale CurtisWhat you describe does seem more logical. I don't think there is an explicit need to specify a communication device on Android; we should be able to use whatever the system decides by default just as on Windows.
However, if you are concerned that changing how the default device is listed could be a surprising change, it could similarly be argued that changing this could be surprising - users may be accustomed to the selectDefaultDevice logic. IMO this is a matter to consider separately from this CL, if at all, though I can document the current behavior better.
Maya JelonkiewiczChanging how we list the devices could be an issue, but it's always hard to tell, especially on Android which doesn't have as large a tapestry of mweb RTC applications.
In this case it would be the semantics of what the default device means that would change. But I assume the same applies.
Maya JelonkiewiczI clarified the default device behavior in the comment. LMK if there is anything more to do here.
Done
class CommunicationDeviceListener {I would advocate for leaving this class as the AudioDeviceListener, since it can listen for output-only devices IIUC.
If `OnCommunicationDeviceChangedListener()` is used for S+, we could simplify the S+ code to no longer depend on this class, or then create a "proper" `CommunicationDeviceListener`.
I do believe this attempts to restrict to communication devices (e.g. for BT we check for a device with a "HEADSET" profile, but not "A2DP"). This class is also only ever used by CommunicationDeviceSelector(AudioDeviceSelector), with the sole purpose of keeping mDeviceStates up to date.
Could you add a comment mentioning that a communication device is a device with paired input/ouput.
Done
class CommunicationDeviceSelectorPreS extends CommunicationDeviceSelector {Could you add a comment mentioning that Pre-S, there isn't the concept of "communication device", but device selection effectively behaves the same way, and is emulated by this class? E.g., selecting a device forces the OS to select both the input and the output at once.
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 3 comments![]( "Open in Gerrit")
Maya JelonkiewiczI see I have in fact made a mistake and flipped the semantics of communication devices - they are output devices for which an input device is automatically selected, rather than the other way around. I've been going along with how in Clank we expose communication devices as input devices, and did not correctly verify this fact. My apologies.
This means that us listing communication devices as inputs and just providing a "default" for outputs is actually quite peculiar, and it should really be the other way around. I don't know if this is something worth changing at this point; while this is "backwards" internally, it's probably meaningless for the end user.
Maya JelonkiewiczMarking unresolved.
Patchset 3 addresses this, for now with the assumption that we won't change the behavior.
Done
// meant to return input devices, this is inverted for compatibility withChanged based on suggestion.
// selection to be made for the input device. Additionally, in order toThomas GuilbertAs above, lets drop the preserving legacy behavior (which I don't think we want to preserve in cases where selecting devices actually works) and rephrase as a TODO(): Populate output device list.
Proposed comment:
```
Android doesn't allow control over setting an individual input and output
device, and forces us to set a single device with an input/output pair
(a.k.a. a "communication device").
We've only returned "default" here for quite some time, forcing output
device selection to happen through selecting the system-wide input
device (see `GetAudioInputDeviceNames()`).
Setting the output via the input device is backwards, and could be updated.
However, only returning "default" here has prevented confusion for users:
populating this list would've given users the option to set a different input
and output devices, which would've failed.
TODO(bug): populate `device_names` with the real list of output devices
and allow per-stream device selection.
```
Changed based on suggestion.
Related details
- Dale Curtis
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert voted and added 5 comments![]( "Open in Gerrit")
Votes added by Thomas Guilbert
| Code-Review | +1 |
5 comments
LGTM! Thank you for your patience, and apologies for the bikeshedding... Future reviews should be more streamlined 🙏
// meant to return input devices, this is inverted for compatibility withThanks!
// selection to be made for the input device. Additionally, in order toThomas GuilbertAs above, lets drop the preserving legacy behavior (which I don't think we want to preserve in cases where selecting devices actually works) and rephrase as a TODO(): Populate output device list.
Maya JelonkiewiczProposed comment:
```
Android doesn't allow control over setting an individual input and output
device, and forces us to set a single device with an input/output pair
(a.k.a. a "communication device").
We've only returned "default" here for quite some time, forcing output
device selection to happen through selecting the system-wide input
device (see `GetAudioInputDeviceNames()`).
Setting the output via the input device is backwards, and could be updated.
However, only returning "default" here has prevented confusion for users:
populating this list would've given users the option to set a different input
and output devices, which would've failed.
TODO(bug): populate `device_names` with the real list of output devices
and allow per-stream device selection.
```
Changed based on suggestion.
Thanks!
private AudioDeviceName @Nullable [] getCommunicationDeviceNames() {Maya JelonkiewiczThis returns the "synthetic" names, which might not be "communication" devices on Pre-S. I would consider renaming this function to `getConnectedDeviceNames()`, `getAvailableDeviceNames()`, or `getGenericDeviceNames()`, then also mention that the specific device model names are replaced with a generic name for that category.
Hmm, I'd be concerned that introducing a new name on top of this may be redundant complexity. From one perspective, pre-S is "simulating" post-S communication device management, and in the end, both sets of APIs are controlling which device is meant to be used for communication-usage streams.
Alternatively, if we change it here, I guess we'd want to change it everywhere else that is relevant to match? "Generic device" seems fair to me, or maybe indeed "synthetic device".
The "Name(s)" part is misleading, for sure. AudioDeviceName wraps a name and an ID, and for per-stream device selection we will probably also store other device metadata there, like the device type. This probably stemmed from `media::AudioDeviceName`, which is also a container for name & ID.
Ok for not introducing a new term, and using communication device across the board. Dropping the "name" also SGTM.
class CommunicationDeviceListener {Maya JelonkiewiczI would advocate for leaving this class as the AudioDeviceListener, since it can listen for output-only devices IIUC.
If `OnCommunicationDeviceChangedListener()` is used for S+, we could simplify the S+ code to no longer depend on this class, or then create a "proper" `CommunicationDeviceListener`.
I do believe this attempts to restrict to communication devices (e.g. for BT we check for a device with a "HEADSET" profile, but not "A2DP"). This class is also only ever used by CommunicationDeviceSelector(AudioDeviceSelector), with the sole purpose of keeping mDeviceStates up to date.
Ok for leaving it as is then.
Related details
- Maya Jelonkiewicz
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 1 comment![]( "Open in Gerrit")
LGTM! Thank you for your patience, and apologies for the bikeshedding... Future reviews should be more streamlined 🙏
No worries, if there are issues worth discussing let's not brush them aside.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Refactor Android audio code to clarify use of communication devices
This change renames classes and methods to make it clearer when they use
and/or manage the system communication device. This should make the
current implementation more transparent and prepares the code for a
future modification that allows for per-stream device selection as
opposed to using the current communication device for streams.
- M media/audio/android/audio_manager_android.cc
- M media/audio/android/audio_manager_android.h
- M media/base/android/BUILD.gn
- M media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
- R media/base/android/java/src/org/chromium/media/CommunicationDeviceListener.java
- R media/base/android/java/src/org/chromium/media/CommunicationDeviceSelector.java
- R media/base/android/java/src/org/chromium/media/CommunicationDeviceSelectorPostS.java
- R media/base/android/java/src/org/chromium/media/CommunicationDeviceSelectorPreS.java
- R media/base/android/java/src/test/org/chromium/media/CommunicationDeviceListenerTest.java
Code-Review: +1 by Thomas Guilbert, +1 by Dale Curtis
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |