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

[IMPL] PulseAudio provider implementation #26

Merged
merged 25 commits into from Oct 10, 2019

Conversation

@FiniteReality
Copy link
Contributor

commented Sep 14, 2019

Some simple functional tests have been included in the TerraFX.Samples project.

Some things I've noticed while implementing this:

  • Mutable IAudioDevice objects actually really don't make much sense outside of one use-case where they're likely going to fail anyway (requesting a device with specific properties)
  • We should probably support some sort of "request default device" given an AudioDeviceType, to return the default device for the given type or null if none was configured by the underlying provider.
  • Requesting a device by name/ID seems like a sensible API to support. Due to the non-portable nature of device names/IDs though, it would probably fit most on the individual provider implementations.
@FiniteReality FiniteReality force-pushed the FiniteReality:feature/pulseaudio-sample branch from e76cd05 to bda0ec9 Sep 17, 2019
Directory.Build.targets Outdated Show resolved Hide resolved
@FiniteReality FiniteReality force-pushed the FiniteReality:feature/pulseaudio-sample branch from 1290db6 to 70b9af8 Sep 17, 2019
public int Channels { get; set; }

/// <inhertidoc />
public bool IsBigEndian { get; set; }

This comment has been minimized.

Copy link
@tannergooding

tannergooding Sep 17, 2019

Collaborator

Hmmm.... Was there an explicit reason why IsBigEndian vs IsLittleEndian... based on the common system and the "true" path being the predicted branch (except for when throwing exceptions directly), I think IsLittleEndian might be better (not needed for this PR though).

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Sep 17, 2019

Author Contributor

Big endian is usually the exception from what I've seen, and usually I special-case the exceptional case to make it clearer, so it made sense to me.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Sep 17, 2019

Collaborator

Yeah, I think that makes sense conceptually. The unfortunate side is that isn't how the JIT (or even C++ compilers) emit code.

Compilers actually put the "true" case as the CPU default predicated branch, which can mean the common path isn't predicted for something like if (IsBigEndian)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Sep 17, 2019

Collaborator

I wonder how the JIT reports if you have... [AggressiveInlining] IsBigEndian => !_isLittleEndian....

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Oct 8, 2019

Author Contributor

I've done a few tests, and from what I can tell it doesn't seem to inline, and it just seems to make a call to the property's getter.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 10, 2019

Collaborator

ah, it is likely because the implementation isn't sealed. Could you handle that in a follow up PR?

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Oct 10, 2019

Author Contributor

Sure

@FiniteReality FiniteReality force-pushed the FiniteReality:feature/pulseaudio-sample branch from 4e37326 to a160524 Sep 17, 2019
@tannergooding

This comment has been minimized.

Copy link
Collaborator

commented Sep 17, 2019

Overall this is looking good and I think I'm happy with it 😄

I can sign-off/etc when you agree and mark it as not being a draft.

@FiniteReality

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

I forgot to comment earlier, but I'd rather at least playback support being implemented before I mark this as ready.

@FiniteReality FiniteReality force-pushed the FiniteReality:feature/pulseaudio-sample branch from efe08c0 to b036ca2 Oct 4, 2019
@tannergooding

This comment has been minimized.

Copy link
Collaborator

commented Oct 5, 2019

The PNG files are huge. Currently sitting at 1.07MB and 3.66MB, compared to 37KB as the biggest existing image (which I believe is only that big because of the transparency effect on Ubuntu terminals).

It would be good to get that fixed (with a history rewrite to avoid binary blob bloat) -- If needed, I can save them on Windows, which seems to do much better compression by default.

@tannergooding

This comment has been minimized.

Copy link
Collaborator

commented Oct 5, 2019

How long is the wav sample?

Also, you should add the wav extension explicitly as a bin file to gitattributes. If there is also some kind of built-in diffing support (I doubt it), those attributes would be preferred (I apparently messed this up for png files, as diffs are supposed to be displayable for them).

@FiniteReality

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2019

Ah, sorry about the large PNG files - I didn't notice that the file size was that large. 😅 The wav file is roughly 1 second long (just under) and it should loop perfectly. I'll add the attributes and work on compressing the PNG files.

buffer[x] = value;
buffer[x + 1] = value;
// Allow overflows to prevent exceptions. This may cause artifacting, but it's unlikely we'll be played for this long enough to matter
unchecked { samplePosition += 1; }

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 5, 2019

Collaborator

What is the position that this loops at?

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Oct 5, 2019

Author Contributor

It should loop roughly every 48.7 seconds, if my maths is correct.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 10, 2019

Collaborator

Not going to block the PR on this, but it would be good to just wrap this when samplePosition hits the end (much like rotations reset to 0 after 360). It helps avoid floating-point error.

@FiniteReality FiniteReality force-pushed the FiniteReality:feature/pulseaudio-sample branch from 30b88dc to fee3884 Oct 5, 2019
@tannergooding tannergooding merged commit 1aef7e1 into terrafx:master Oct 10, 2019
10 checks passed
10 checks passed
license/cla Contributor License Agreement is signed.
Details
terrafx.terrafx-pr Build #20191005.5 succeeded
Details
terrafx.terrafx-pr (macos_debug_x64) macos_debug_x64 succeeded
Details
terrafx.terrafx-pr (macos_release_x64) macos_release_x64 succeeded
Details
terrafx.terrafx-pr (ubuntu_debug_x64) ubuntu_debug_x64 succeeded
Details
terrafx.terrafx-pr (ubuntu_release_x64) ubuntu_release_x64 succeeded
Details
terrafx.terrafx-pr (windows_debug_x64) windows_debug_x64 succeeded
Details
terrafx.terrafx-pr (windows_debug_x86) windows_debug_x86 succeeded
Details
terrafx.terrafx-pr (windows_release_x64) windows_release_x64 succeeded
Details
terrafx.terrafx-pr (windows_release_x86) windows_release_x86 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.