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

Implement a Cubeb audio sink #3776

Merged
merged 5 commits into from Jun 1, 2018

Conversation

Projects
None yet
9 participants
@darkf
Contributor

darkf commented May 25, 2018

This PR implements a cubeb audio sink, as an alternative to the existing SDL2 audio sink. Cubeb is also used by Dolphin and noted for its low latency.

The implementation here is based partially off of Dolphin's and the existing SDL2 audio sink, although the actual output buffering is new.


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented May 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-05-25T06:49:06Z: 45aa950...darkf:e773b38f6b0d412c12f7f7ff20fee479a8f11e52

@j-selby

Minor nits, but otherwise looks good!

@@ -0,0 +1,150 @@
// Copyright 2016 Citra Emulator Project

This comment has been minimized.

@j-selby

j-selby May 25, 2018

Contributor

2016?

This comment has been minimized.

@darkf

darkf May 25, 2018

Contributor

I copied parts of it (well, the interface, mainly) from the SDL2 sink, which was written in 2016, it's just a min date.

.gitmodules Outdated
@@ -31,3 +31,6 @@
[submodule "libressl"]
path = externals/libressl
url = https://github.com/citra-emu/ext-libressl-portable.git
[submodule "externals/cubeb"]

This comment has been minimized.

@j-selby

j-selby May 25, 2018

Contributor

For consistency with other submodules, might want to strip out externals from the name of the submodule.

if(ENABLE_CUBEB)
target_link_libraries(audio_core PRIVATE cubeb)
add_definitions(-DHAVE_CUBEB=1)
endif()

This comment has been minimized.

@j-selby

j-selby May 25, 2018

Contributor

Newline.

This comment has been minimized.

@darkf

darkf May 25, 2018

Contributor

@j-selby Addressed, thanks.

@darkf darkf force-pushed the darkf:cubeb-sink2 branch from e773b38 to 7cb6bcf May 25, 2018

@cluezbot

This comment has been minimized.

cluezbot commented May 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-05-25T07:07:14Z: 45aa950...darkf:7cb6bcffb2ab4d16407963649e4767c058914136

@cluezbot

This comment has been minimized.

cluezbot commented May 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-05-25T07:29:47Z: 45aa950...darkf:c80c0a00e80fdab0699b59758fb78a563d150372

@darkf darkf force-pushed the darkf:cubeb-sink2 branch from 7cb6bcf to c80c0a0 May 25, 2018

@cluezbot

This comment has been minimized.

cluezbot commented May 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-05-25T08:39:40Z: 45aa950...darkf:4f8016535e7a2aed7f7f32dcef6f421b62d1d323

@darkf darkf force-pushed the darkf:cubeb-sink2 branch from 4f80165 to 92d1f7b May 25, 2018

@cluezbot

This comment has been minimized.

cluezbot commented May 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-05-25T08:46:23Z: 45aa950...darkf:92d1f7be8b4244d422fa83290839e681ca130d1d

impl->queue.erase(impl->queue.begin(), impl->queue.begin() + frames_to_write * 2);
if (frames_to_write < num_frames) {
// Fill the rest of the frames with silence

This comment has been minimized.

@MerryMage

MerryMage May 25, 2018

Member

It's better to fill the frame with the last available sample (prevents click noises when this happens due to the high frequences in a sudden cut-off).

Should do that to the sdl2 backend too.

Edit: Thinking about this I'll do that after this PR is merged.

This comment has been minimized.

@adityaruplaha

adityaruplaha May 26, 2018

Contributor

Will it be possible to do a slight fadeout? I feel sounds stopping abruptly don't sound well.

This comment has been minimized.

@darkf

darkf May 26, 2018

Contributor

That's something merry would have to explain, but I don't think that would help, it happens really fast and should be hardly noticeable. Being slightly less abrupt is the point of using the last available samples (maintains the wave instead of a sharp drop to 0.) But she's the expert here. :)

@MerryMage

lgtm otherwise

@BreadFish64

This comment has been minimized.

Contributor

BreadFish64 commented May 28, 2018

When the cubeb audio backend is used, the volume in Window's volume mixer is always reset when Citra is restarted: https://redd.it/8mkj5l

// Licensed under GPLv2 or any later version
// Refer to the license.txt file included.
#include <iostream>

This comment has been minimized.

@lioncash

lioncash May 28, 2018

Member

Is <iostream> intended to be left in? This'll inject a static constructor into the translation unit, even if nothing uses it.

private:
struct Impl;
std::unique_ptr<Impl> impl;
int device_id;

This comment has been minimized.

@lioncash

lioncash May 28, 2018

Member

device_id doesn't seem to be used (this also likely belongs in Impl if it's intended to be used)

long CubebSink::Impl::DataCallback(cubeb_stream* stream, void* user_data, const void* input_buffer,
void* output_buffer, long num_frames) {
Impl* impl = reinterpret_cast<Impl*>(user_data);

This comment has been minimized.

@lioncash

lioncash May 28, 2018

Member

This can be a static_cast

return 0;
long frames_written = 0; // Number of frames written to the output buffer
long frames_to_write = std::min(impl->queue.size() / 2, static_cast<size_t>(num_frames));

This comment has been minimized.

@lioncash

lioncash May 28, 2018

Member

long is 32-bit on windows and 64-bit on LP64 systems. Are these usages intentional? (instead of using long long or size_t)

This comment has been minimized.

@darkf

darkf May 28, 2018

Contributor

Oops, I used to return a long computed from this. Good catch!

struct Impl;
std::unique_ptr<Impl> impl;
int device_id;
std::vector<std::string> device_list;

This comment has been minimized.

@lioncash

lioncash May 28, 2018

Member

This should be within Impl (given the point of the idiom is to hide implementation details).

This comment has been minimized.

@darkf

darkf May 28, 2018

Contributor

I had copied that interface from sdl2_sink, fixed.

@darkf

This comment has been minimized.

Contributor

darkf commented May 28, 2018

@BreadFish64 As mentioned on Discord, it seems that it is the fault of upstream to me, as it appears to always set 100% volume on device reset/init and does not bother to query the existing volume upon opening it (even though WASAPI should remember it.)

You mentioned this is reproducible with Dolphin, so an issue should be raised on the cubeb repo regarding it.

@darkf darkf force-pushed the darkf:cubeb-sink2 branch from 92d1f7b to ab49fbe May 28, 2018

@cluezbot

This comment has been minimized.

cluezbot commented May 28, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-05-28T23:08:42Z: 65f5bc7...darkf:ab49fbe75b7509e5cf6b4aedbc769206a48fd455

@darkf

This comment has been minimized.

Contributor

darkf commented May 28, 2018

@lioncash Thanks for your review! I've addressed all of your comments.

@cluezbot

This comment has been minimized.

cluezbot commented Jun 1, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-01T02:03:22Z: 72f9142...darkf:04139e26bdcf8e6a181a9d9487954fdfc99bfe2c

@darkf darkf force-pushed the darkf:cubeb-sink2 branch from ab49fbe to 04139e2 Jun 1, 2018

@jroweboy jroweboy merged commit 04a9145 into citra-emu:master Jun 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jbeich jbeich referenced this pull request Jun 2, 2018

Merged

Unbreak install #3805

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment