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

UI: Add Xbox Live Communicator to UI #1873

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

Conversation

faha223
Copy link
Contributor

@faha223 faha223 commented Jan 16, 2025

Added UI for attaching XBLC to expansion port
Refactored XBLC to use SDL2 Audio instead of QEMU Audio so that audio devices can be selected

Converted Audio to use SDL2 Audio instead of QEMU Audio so that users can choose which audio devices to use
@faha223
Copy link
Contributor Author

faha223 commented Jan 16, 2025

It still needs a bit more testing, and I need to recreate the quick-and-dirty XBLC image as a proper SVG

@MasonT8198
Copy link
Contributor

Looks good so far!

Just checking, does this support XBLC with an XMU attached (port 1/port 2) etc

@faha223
Copy link
Contributor Author

faha223 commented Jan 16, 2025

Looks good so far!

Just checking, does this support XBLC with an XMU attached (port 1/port 2) etc

Yes. Each controller has 2 Expansion slots. The XBLC can only go in Slot A but you can have an XBLC in Slot A and an XMU in Slot B, just like on a real controller

Recreated the xblc_mask.png from the SVG
…ched when switching input device while keeping the output device static. It seems to go away if you reinitialize both input and output voices whenever changing either device.

Removed the code in xblc.c that was supposed to increase the volume when volume > SDL_MIX_MAXVOLUME. This code didn't work and has already been removed in the UI
@faha223 faha223 marked this pull request as ready for review January 20, 2025 01:01
Copy link
Member

@mborgerson mborgerson left a comment

Choose a reason for hiding this comment

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

Thanks for the work. Some initial feedback

hw/xbox/xblc.c Outdated
USBXBLCState *s = (USBXBLCState *)userdata;

s->in.average_volume = s->in.volume *
calc_average_amplitude((int16_t*)stream, len / 2) / 128;
Copy link
Member

Choose a reason for hiding this comment

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

  • If dividing by two to calculate number of samples from number of bytes, can you make it more explicit, e.g. (size_t num_samples = len / sizeof(int16_t);
  • I assume /128 is dividing through by SDL_MIX_MAXVOLUME?

hw/xbox/xblc.c Outdated
if (processed < max_len) return;
static int calc_average_amplitude(const int16_t *samples, int len) {
int max = 0;
for(int i = 0; i < len / 2; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Why divide by 2 here? If samples is given as a int16_t, len should be the number of samples not bytes.

hw/xbox/xblc.c Outdated
processed = AUD_write(s->out.voice, (void *)data, max_len);
avail -= processed;
if (processed < max_len) return;
static int calc_average_amplitude(const int16_t *samples, int len) {
Copy link
Member

Choose a reason for hiding this comment

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

This function doesn't calculate the average?

hw/xbox/xblc.c Outdated
uint32_t max_len = MIN(len, fifo8_num_free(&s->in.fifo));
if (max_len > 0) {
if(s->in.volume < SDL_MIX_MAXVOLUME) {
uint8_t *buffer = g_malloc(max_len);
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid memory allocation in this very frequently called 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.

I've changed this to use a function scope buffer of a fixed size, but would it be better to use a static global scope buffer?

SDL_AudioSpec desired_spec;
desired_spec.channels = 1;
desired_spec.freq = s->sample_rate;
desired_spec.format = AUDIO_S16LSB;
Copy link
Member

Choose a reason for hiding this comment

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

What if the actual format differs from the desired format? SDL apparently does conversion for you.

ui/xemu-input.c Outdated
@@ -489,10 +578,16 @@ void xemu_input_bind(int index, ControllerState *state, int save)
// Unbind any XMUs
Copy link
Member

Choose a reason for hiding this comment

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

Comment out of date

ui/xemu-input.c Outdated
PERIPHERAL_XMU)
xemu_input_unbind_xmu(index, i);
// If a peripheral was bound, unbind the peripheral
switch (bound_controllers[index]->peripheral_types[i]) {
Copy link
Member

Choose a reason for hiding this comment

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

How about just testing with PERIPHERAL_NONE?

ui/xemu-input.c Outdated
return g_strdup_printf("%d|%d|%s|%s", (int)(200 * xblc->output_device_volume), (int)(200 * xblc->input_device_volume), output_device_label, input_device_label);
}

XblcState *xemu_input_deserialize_xblc_settings(const char *param)
Copy link
Member

Choose a reason for hiding this comment

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

This custom config parsing is tricky and I'd strongly like to avoid it. I think we can simplify it to an array of structures with genconfig and avoid parsing altogether. We can also enhance genconfig if need be

float original_volume = *ui_volume;
const char *label = (is_capture == 0) ? "Speaker" : "Microphone";
char description[32];
sprintf(description, "Volume: %.2f%%", 100 * original_volume);
Copy link
Member

Choose a reason for hiding this comment

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

Don't need so much precision

for(int device_index = -1; device_index < numOutputDevices; device_index++) {
const char *device_name = default_device_name;
if(device_index >= 0)
device_name = SDL_GetAudioDeviceName(device_index, is_capture);
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to cache these device names and not query them on every frame

Changed naming of "average volume" to "peak volume" to more accurately describe what is being done/represented
Removed the divide by 2 in calc_peak_volume so that the second half of the samples would also be factored when calculating the peak volume in the most recent sample data
Changed references to 128 to use the SDL_MIX_MAXVOLUME preprocessor definition for clarity
Changed the line that caches the peak volume in input_callback to use sizeof(int16_t) instead of 2 for clarity
Changed input_callback to use a local buffer of size XBLC_FIFO_SIZE instead of allocating one at runtime
Changed the get/set volume functions in xblc.h and xblc.c to use floating point values on the interval [0, 1] instead of integers on the range [0, SDL_MIX_MAXVOLUME] because they're used that way everywhere else anyway
Changed the numOutputDevices variable in DrawAudioDeviceSelectComboBox function to num_devices to match the naming scheme and because this function doesn't only work with output devices.
Changed xemu_input_bind to use an if statement instead of a switch. The switch was left behind from before I added the xemu_input_unbind_peripheral function when I was calling xemu_input_unbind_xmu and xemu_input_unbind_xblc directly
…'t have to query them every frame.

The cache is invalidated whenever the number of devices changes. I put this outside ComboBox if statement so that if the available devices changes when the combo box isn't open the cache still gets updated.
@BarracaDoSeuZe
Copy link

i was trying to make an Portable .ini Setting for Xemu, and i found a problem while trying to automatically configure the XBLC into the Controller. if you set "[input.peripherals.port1] peripheral_type_0 = 2" without any "[input.bindings]" set in, the emulator will hard crash with any game that actually uses the XBLC. can this be somewhat improved?

@MasonT8198
Copy link
Contributor

MasonT8198 commented Mar 15, 2025 via email

@faha223
Copy link
Contributor Author

faha223 commented Mar 15, 2025

i was trying to make an Portable .ini Setting for Xemu, and i found a problem while trying to automatically configure the XBLC into the Controller. if you set "[input.peripherals.port1] peripheral_type_0 = 2" without any "[input.bindings]" set in, the emulator will hard crash with any game that actually uses the XBLC. can this be somewhat improved?

It doesn't make sense to have an [input.peripherals.port1] section in your ini if there is no controller connected to port 1. You can't attach an XBLC to the Console if the controller it connects through does not exist.

It shouldn't cause a crash the emulator though. I'm going to investigate this and see if I can fix it. Thanks for finding this!

@BarracaDoSeuZe
Copy link

BarracaDoSeuZe commented Mar 15, 2025

It doesn't make sense to have an [input.peripherals.port1] section in your ini if there is no controller connected to port 1. You can't attach an XBLC to the Console if the controller it connects through does not exist.

This was a thing i was testing, while using the Auto Bind controller option, the emulator would automatically setup the xbcl into the Controller Port ( since i want everything to organize itself automatically, without me having to configure anything ). at least doing this actually works, the only problem is the crash .-

…using issues with Halo 2 when the XBLC is connected during startup. I moved it so that it's only used for the XBLC Microphone endpoint and that seems to have resolved the issue
@faha223
Copy link
Contributor Author

faha223 commented Mar 16, 2025

It doesn't make sense to have an [input.peripherals.port1] section in your ini if there is no controller connected to port 1. You can't attach an XBLC to the Console if the controller it connects through does not exist.

This was a thing i was testing, while using the Auto Bind controller option, the emulator would automatically setup the xbcl into the Controller Port ( since i want everything to organize itself automatically, without me having to configure anything ). at least doing this actually works, the only problem is the crash .-

I've made a change that I think should fix this issue. I was able to easily replicate it with Halo 2. After the build is finished, can you check if the issue you're seeing still exists?

@BarracaDoSeuZe
Copy link

I've made a change that I think should fix this issue. I was able to easily replicate it with Halo 2. After the build is finished, can you check if the issue you're seeing still exists?

Tested it here, and now it does not Crash on Startup ;). but i encountered a weird glitch, but I don't know if that would catch your attention.
After letting the emulator automatically pickup the Controller, it doens't seem to know what Audio Device he should be getting. i thought it would use the "Default" option, but it get this random text.
xblcglitch

@faha223
Copy link
Contributor Author

faha223 commented Mar 16, 2025

I've made a change that I think should fix this issue. I was able to easily replicate it with Halo 2. After the build is finished, can you check if the issue you're seeing still exists?

Tested it here, and now it does not Crash on Startup ;). but i encountered a weird glitch, but I don't know if that would catch your attention. After letting the emulator automatically pickup the Controller, it doens't seem to know what Audio Device he should be getting. i thought it would use the "Default" option, but it get this random text. xblcglitch

I saw this exactly once, but I haven't been able to reproduce it

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.

4 participants