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

Added DPL2 decoder to XAudio2 and XAudio2_7 backends. #6171

Merged
merged 2 commits into from Mar 18, 2019

Conversation

6 participants
@LAGonauta
Copy link
Contributor

LAGonauta commented Nov 5, 2017

I cherry picked this commit from PR #5235, as suggested by @ligfx.

Tested on Windows 8.1 x64, works fine. I will test later on Windows 10 x64, but it would be great if someone could test it on Windows 7.

Stop();
return false;
if (FAILED(
hr = m_xaudio2->CreateMasteringVoice(&m_mastering_voice, 6, m_mixer->GetSampleRate())))

This comment has been minimized.

Copy link
@Warepire

Warepire Nov 5, 2017

This code-block can be compacted with either a ternary if or by assigning the 6 (or 2), to a temporary variable based on SConfig::GetInstance().bDPL2Decoder.

Additionally, you define names for these "magic" numbers higher up in the file. Use those names.

This comment has been minimized.

Copy link
@LAGonauta

LAGonauta Nov 6, 2017

Author Contributor

Done.


if (m_use_surround)
{
xaudio_buffer = std::unique_ptr<BYTE[]>(new BYTE[NUM_BUFFERS * BUFFER_SIZE_BYTES_SURROUND]);

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 6, 2017

Member

this can be simplified to:

xaudio_buffer = std::unique_ptr<BYTE[]>(NUM_BUFFERS * BUFFER_SIZE_BYTES_SURROUND);

ditto for the other one.

This comment has been minimized.

Copy link
@LAGonauta

LAGonauta Nov 6, 2017

Author Contributor

I tried changing to that but I couldn't, it won't compile.

IntelliSense error:

Error (active) E0289 no instance of constructor "std::unique_ptr<_Ty [], _Dx>::unique_ptr [with _Ty=BYTE, _Dx=std::default_delete<BYTE []>]" matches the argument list

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 6, 2017

Member

Oh sorry, I had meant to type std::make_unique, not std::unique_ptr. So:

xaudio_buffer = std::make_unique<BYTE[]>(NUM_BUFFERS * BUFFER_SIZE_BYTES_SURROUND);

though, if using the solution in my other comment with regards to std::variant, this would involve using float[] and short[] instead of BYTE[]

m_mixer->Mix(static_cast<short*>(context), SAMPLES_PER_BUFFER);
if (m_use_surround)
{
m_mixer->MixSurround(static_cast<float*>(context), SAMPLES_PER_BUFFER_SURROUND);

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 6, 2017

Member

This cast, and the other one below it are technically undefined behavior, since it's effectively reinterpreting a BYTE buffer, which breaks alignment requirements.

This comment has been minimized.

Copy link
@LAGonauta

LAGonauta Nov 6, 2017

Author Contributor

Understood. I guess I could change it from static_cast to reinterpret_cast so it can show what it is really doing? I suppose that won't correct the alignment breakage, right?

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 6, 2017

Member

I suppose that won't correct the alignment breakage, right?

It wouldn't. One of the possible solutions is to use a std::variant such as:

std::variant<std::unique_ptr<short[]>, std::unique_ptr<float[]>> xaudio_buffer;

which would preserve the type of buffer. As opposed to just being a BYTE buffer (these are allowed to be reinterpret_casted to BYTE (aka unsigned char) without breaking alignment requirements.

This comment has been minimized.

Copy link
@LAGonauta

LAGonauta Nov 6, 2017

Author Contributor

Is it correct now, or should I change anything else?

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 6, 2017

Member

Yup mostly good. Just cosmetic stuff: the reinterpret_casts in Mix and MixSurround calls can remain static_casts. However the bodies of statements that travel more than one line should be braced.

e.g.

for (int i = 0; i != NUM_BUFFERS; ++i)
  SubmitBuffer(
      reinterpret_cast<BYTE*>(std::get<std::unique_ptr<float[]>>(xaudio_buffer).get()) +
      (i * BUFFER_SIZE_BYTES_SURROUND));

should be:

for (int i = 0; i != NUM_BUFFERS; ++i)
{
  SubmitBuffer(
      reinterpret_cast<BYTE*>(std::get<std::unique_ptr<float[]>>(xaudio_buffer).get()) +
      (i * BUFFER_SIZE_BYTES_SURROUND));
}

@LAGonauta LAGonauta force-pushed the LAGonauta:xaudio2-surround branch from 6ed39c9 to 78040c1 Nov 6, 2017


wfx.Format.nChannels = 6;
wfx.Format.wBitsPerSample = 32;
wfx.Format.cbSize = 22;

This comment has been minimized.

Copy link
@Warepire

Warepire Nov 6, 2017

This number is rather magical, can you show how you derived it?

This comment has been minimized.

Copy link
@LAGonauta

LAGonauta Nov 6, 2017

Author Contributor

Sure. I got them from here:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd390971(v=vs.85).aspx

Maybe just linking to this page is enough?

This comment has been minimized.

Copy link
@Warepire

Warepire Nov 6, 2017

I meant that you should calculate it using something similar to this line: wfx.Format.cbSize = sizeof(WAVEFORMATEXTENSIBLE) - sizeof(WAVEFORMATEX);

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 6, 2017

Member
sizeof(wfx.Samples) + sizeof(wfx.dwChannelFormat) + sizeof(wfx.SubFormat);
//        2                          4                          16

would be the equivalent.

doing math here in my head, since I'm not at my Windows PC to actually check this, but iirc

sizeof(WAVEFORMATEXTENSIBLE) - sizeof(WAVEFORMATEX);

would be 24 including padding. I don't know if that matters in practice though, since the docs just say it needs to be at least 22.

This comment has been minimized.

Copy link
@Warepire

Warepire Nov 6, 2017

And unless Windows does something less than great with the packing of that struct (I haven't checked in the header), the padding will occur between Samples and dwChannelMask if I am not mistaken in my alignment math.

@LAGonauta LAGonauta force-pushed the LAGonauta:xaudio2-surround branch 3 times, most recently from c4226be to 65db6d8 Nov 6, 2017

@LAGonauta

This comment has been minimized.

Copy link
Contributor Author

LAGonauta commented Mar 14, 2018

Anything I can do to make this PR move forward for merge?

@fantesykikachu

This comment has been minimized.

Copy link

fantesykikachu commented Mar 31, 2018

tested with windows 7 and it sounds great, however the system was not actually using surround, but i did try with it forced into surround, 4.1, and 7.1. (so i can't tell if the positions are correct)

@LAGonauta

This comment has been minimized.

Copy link
Contributor Author

LAGonauta commented Apr 1, 2018

Thanks for testing on Windows 7.
This PR alone won't let you have the full surround experience as the current decoder is more of a ProLogic decoder (not ProLogic II). As long as you heard something in your surround speaker should mean it is working, a great software to test the surround decoder is Mario Kart Double Dash and its surround test."Super" should be heard moving from front left to front right, "Mario" should be heard on the right rear speaker, and "Kart" on the left rear speaker.
The full surround experience will be available after #5235 is merged.

@LAGonauta LAGonauta force-pushed the LAGonauta:xaudio2-surround branch from 65db6d8 to 99a3556 Mar 18, 2019

@LAGonauta

This comment has been minimized.

Copy link
Contributor Author

LAGonauta commented Mar 18, 2019

As #5235 is now merged, anything I can do to make this PR move forward? I just rebased and added a commit to be able to enable DPLII with XAudio2.

@shuffle2 shuffle2 merged commit c547108 into dolphin-emu:master Mar 18, 2019

9 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@zellfy

This comment was marked as off-topic.

Copy link

zellfy commented Mar 28, 2019

@shuffle2 hello, have you seen what I type on your nx2elf topic? Please help me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.