Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cloned Playback Volume data model module into Capture module #175

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

DragRedSim
Copy link
Contributor

-Adds default capture device to the data model
-Works in the same way as (and reuses the code of) the playback device
-Works on default capture device (eg. microphone, line-in, stereo mix)

I created this because one of the RGB buttons on my laptop defaults to muting and unmuting the microphone, and I wanted to have the mute status reflected in the button colour; to do this, I needed to expose the capture device in the data model, in the same way that the playback device is exposed. It is simply a copy/paste, with appropriate identifiers find/replaced, and the default capture device being referenced in the initialisation rather than the default playback device.

-Adds default capture device to the data model
-Works in the same way as (and reuses the code of) the playback device
-Works on default capture device (eg. microphone, line-in, stereo mix)
-Done to match behaviour of my setup, where mic mute key affects that device specifically
@RobertBeekman
Copy link
Member

Hello,

Sorry for not getting back to you sooner. I went through it and I think this is a nice addition.
The only problem I have is that it's basically an exact copy, like you mentioned. If we have a bug in one of them, we have to fix it twice. This breaks the DRY-principle.

To avoid this issue, could you refactor this to use an abstract class instead? Functionally, the only real difference is in SetPlaybackDevice so you can either make that take a role data flow and role as arguments.

I propose the following class signatures:
AudioEndpointVolumeModule : Module<AudioEndpointVolumeDataModel>
CaptureVolumeModule : AudioEndpointVolumeModule
PlaybackVolumeModule : AudioEndpointVolumeModule

This allows you to put all shared logic in AudioEndpointVolumeModule which can also just reuse a single data model class, AudioEndpointVolumeDataModel.

Thank you!

@DragRedSim
Copy link
Contributor Author

Happy to do that, just need to figure out the logic; I’m not the most experienced with classes and that, so I went for the most basic possible solution that produced the effect I was wanting at the time. I do understand it’s not the best for code maintainability, but as a proof of concept it worked. I’ll take a look at it shortly.

Another benefit of the requested refactor is to allow more than just the specified devices to be exposed to the data model, if someone puts together a suitable interface for users to select devices; rather than the current two options of default playback device and default communication capture device. More reason to go ahead with the refactor.

@diogotr7
Copy link
Member

You could have additional devices available as dynamic data models but I'm not sure how useful this is

@DragRedSim
Copy link
Contributor Author

Okay, I've given it a crack at refactoring. A couple of things I wasn't exactly happy with:

  • The plugin now throws an exception on startup, specifically for attempting to instantiate the AudioEndpointVolumeModule as its own module. I didn't figure out how to get that ignored. It attempts to instantiate all of AudioEndpoint, Capture and Playback modules.
  • The individual modules use default values of the individual roles as default, since I couldn't figure out how to pass in appropriate values. It needs to be done at the time of instantiation. The DataFlow was also hardcoded, but that is less of an issue since the point was to have the individual classes handle capture and render, respectively.

@diogotr7
Copy link
Member

Artemis trying to load an abstract class as a feature seems like an oversight to me, it will be fixed in Core. I'm doing quite large changes to this plugin for linux compatibility, i'll see if I can get these changes in there as well.


private bool SetAudioEndpointDevice()
{
_audioDevice = _naudioDeviceEnumerationService.GetDefaultAudioEndpoint(DataFlow.Render, Role.Console);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_audioDevice = _naudioDeviceEnumerationService.GetDefaultAudioEndpoint(_flow, _role);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way at least the module will correctly capture the mic. Unfortunately, the volume doesn't seem to update unless i also have the native windows mixer open:

Artemis.UI.Windows_e0ZRENbMP9.mp4

I'm not sure if we need to listen for some event or call some update function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, missed that one (_flow/_role). That's a major issue. Fix implemented.

In regards to the volume not updating, I can confirm this issue occurs on my system. My suspicion is that nAudio is registering the device and pulling in data - but not actually opening access to the device itself, in regards to the security context. If you note the icon in the system tray for "Apps using your microphone", it will appear when the Windows recording device panel is enabled, and disappear when it is not. The same occurs if you use the Settings sound panel (the "replacement" control panel), or (for another example) if you have a Discord call open. I'll do a little more digging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, we do have to initialise a recording access in order for the volume data to be updated. This was outside my use case, as I only wanted the mute state or otherwise; but better to do it right the first time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience users don't like it when apps listen to the mic 24/7 so I would be careful with how you implement this. Maybe separate volume and actual sounds into separate modules or add an option to enable mic capture or something? It's not a trivial feature to add imo.

@DragRedSim
Copy link
Contributor Author

In my experience users don't like it when apps listen to the mic 24/7 so I would be careful with how you implement this. Maybe separate volume and actual sounds into separate modules or add an option to enable mic capture or something? It's not a trivial feature to add imo.

I've implemented a capture feature; on by default (for consistency with the render variant of the plugin), but with an option in the plugin settings to disable it. Description text could probably use some improvement, but it's there as the second option in the list.

@DragRedSim
Copy link
Contributor Author

One other thing I noticed; if you disable the capture option while the data model debugger is open, it will appear to stop updating; this is only a visual bug, the data model is updating in the background. A quick way to test is using this profile I quickly put together (change the file extension to JSON), and then try toggling the enable/disable on and off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants