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

SoundEffect fixes and tests #4469

Merged
merged 6 commits into from Apr 11, 2016

Conversation

Projects
None yet
5 participants
@tomspilman
Copy link
Member

commented Jan 26, 2016

This PR starts to add some unit tests for SoundEffect while fixing bugs. I also tried to cleanup platform initialization limiting it to one path for PCM data and another for non-PCM formats.

I cleaned up the #if mess in SoundEffectReader by moving the work into SoundEffect platform methods.

This needs further testing before merging.

tomspilman added some commits Jan 26, 2016

Fixed public SoundEffect ctors to do exhaustive parameter checking.
Added internal SoundEffect ctor for format based initialization.
Lots of fixes to match XNA unit tests.

@tomspilman tomspilman added this to the 3.6 Release milestone Jan 26, 2016

#endif
// Make our own copy of the data.
_data = new byte[count];
Array.Copy(buffer, offset, _data, 0, count);

This comment has been minimized.

Copy link
@tomspilman

tomspilman Jan 26, 2016

Author Member

This wasn't here before, but it seems dangerous to not copy the passed in sound buffer as the caller could alter it after the fact. In fact I expect under XNA it does something similar copying the passed in buffer into the native code.

This comment has been minimized.

Copy link
@KonajuGames

KonajuGames Jan 26, 2016

Contributor

We need to make sure we're not duplicating some of these buffers. If the buffer is owned by SoundEffect, we shouldn't need to duplicate it here. If it is the buffer passed to SoundEffect ctor, I would expect SoundEffect to copy it first and it owns that copy.


#endregion

#region Public Constructors

/// <param name="buffer">Buffer containing PCM wave data.</param>
/// <param name="buffer">Buffer containing 16bit PCM wave data.</param>

This comment has been minimized.

Copy link
@tomspilman

tomspilman Jan 26, 2016

Author Member

There seemed to be some confusion in the code base about this, but the SoundEffect public ctors in
XNA only support 16bit PCM.

This comment has been minimized.

Copy link
@KonajuGames

KonajuGames Jan 26, 2016

Contributor

It would be good to extend this for the platforms that can support higher fidelity, such as 24-bit PCM and floating point. It has been brought up in comments before.

This comment has been minimized.

Copy link
@tomspilman

tomspilman Jan 26, 2016

Author Member

It would be good to extend this for the platforms that can support higher fidelity

Seems like that could be fit in with some additional bitdepth passed to these ctors and helper functions.

@mgbot

This comment has been minimized.

Copy link
Member

commented on 8a70a73 Jan 26, 2016

TeamCity MonoGame :: Develop (Win) Build 3.5.0.1241 outcome was FAILURE
Summary: Snapshot dependency failed: MonoGame :: Develop (Mac) Build time: 00:00:05


if (format != 1)
throw new NotSupportedException("Unsupported wave format!");

This comment has been minimized.

Copy link
@tomspilman

tomspilman Jan 26, 2016

Author Member

Does the OpenAL code on iOS/MacOSX support anything other than PCM? The pipeline suggests it supports IMA ADPCM...

https://github.com/mono/MonoGame/blob/develop/MonoGame.Framework.Content.Pipeline/Processors/SoundEffectProcessor.cs#L48

Or is this broken?

This comment has been minimized.

Copy link
@KonajuGames

KonajuGames Jan 26, 2016

Contributor

I've seen some blogs that show how to decode IMA ADPCM for playback through OpenAL on iOS. I believe the higher level audio functions support ADPCM, but the OpenAL layer may not. OpenAL Soft on Android does support ADPCM (IMA and MS), but I'm not sure if it has been fully tested.

This comment has been minimized.

Copy link
@tomspilman

tomspilman Jan 26, 2016

Author Member

I guess that means the pipeline is generating compressed audio that currently doesn't work on iOS/MacOS. Can you verify this @dellis1972 ?

This comment has been minimized.

Copy link
@dellis1972

dellis1972 Jan 27, 2016

Contributor

@tomspilman with the current pipeline tool I can play audio fine on iOS and Mac as well. No idea what its generating though

This comment has been minimized.

Copy link
@tomspilman

tomspilman Jan 27, 2016

Author Member

@dellis1972 - If you set the quality on the sound in the pipeline to medium or low does it still work?

This comment has been minimized.

Copy link
@dellis1972

dellis1972 Jan 27, 2016

Contributor

I seem to remember allot of noise/static when you do that. Best works fine
though.

On 27 January 2016 at 14:45, Tom Spilman notifications@github.com wrote:

In MonoGame.Framework/Audio/SoundEffect.OpenAL.cs
#4469 (comment):

  •        if (format != 1)
    
  •            throw new NotSupportedException("Unsupported wave format!");
    

@dellis1972 https://github.com/dellis1972 - If you set the quality on
the sound in the pipeline to medium or low does it still work?


Reply to this email directly or view it on GitHub
https://github.com/mono/MonoGame/pull/4469/files#r50992041.

This comment has been minimized.

Copy link
@tomspilman

tomspilman Jan 27, 2016

Author Member

I seem to remember allot of noise/static when you do that. Best works fine
though.

That means we have a bug there. The pipeline is specifically generating compressed audio for medium/lowest... it makes sense you are getting static because we've made no attempt to support that compressed format in the OpenAL code.

Anyway... i'm not introducing any new bugs here then. We need to deal with this in a separate issue. See #4470.

@mgbot

This comment has been minimized.

Copy link
Member

commented on 31824e6 Jan 26, 2016

TeamCity MonoGame :: Develop (Win) Build 3.5.0.1242 outcome was FAILURE
Summary: Snapshot dependency failed: MonoGame :: Develop (Mac) Build time: 00:00:00

@mgbot

This comment has been minimized.

Copy link
Member

commented on 01dc102 Jan 26, 2016

TeamCity MonoGame :: Develop (Win) Build 3.5.0.1243 is now running

This comment has been minimized.

Copy link
Member

replied Jan 26, 2016

TeamCity MonoGame :: Develop (Win) Build 3.5.0.1243 outcome was FAILURE
Summary: Exit code 1 (new) Build time: 00:12:44

@mgbot

This comment has been minimized.

Copy link
Member

commented on ef8e050 Jan 26, 2016

TeamCity MonoGame :: Develop (Win) Build 3.5.0.1244 is now running

This comment has been minimized.

Copy link
Member

replied Jan 26, 2016

TeamCity MonoGame :: Develop (Win) Build 3.5.0.1244 outcome was FAILURE
Summary: Tests failed: 1 (1 new), passed: 588, ignored: 6 Build time: 00:26:30

Failed tests

MonoGameTests.exe: MonoGame.Tests.Visual.SpriteBatchTest.Draw_without_blend: <no details avaliable>

This comment has been minimized.

Copy link
Member

replied Jan 26, 2016

TeamCity MonoGame :: Develop (Win) Build 3.5.0.1245 is now running

This comment has been minimized.

Copy link
Member

replied Jan 26, 2016

TeamCity MonoGame :: Develop (Win) Build 3.5.0.1245 outcome was SUCCESS
Summary: Tests passed: 589, ignored: 6 Build time: 00:25:01

@tomspilman

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2016

I tested the desktop DX and GL paths which seem to be working great. Just need to be sure I didn't break iOS or Android somehow.

@tomspilman

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2016

Retesting this now on Windows... be good to have someone test on Mac/Linux and ideally iOS and Android.

@tomspilman

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2016

Actually new plan.

@KonajuGames - After this is merged I will follow it up with set of unit tests for SoundEffectInstance following the pattern @polsys defined in #4715. My time is better spent making a good test rather than manually testing yet another time.

@tomspilman

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2016

@mrhelmut - Maybe you should look this over once too?

@mrhelmut

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2016

@tomspilman works just fine on DesktopGL.

@KonajuGames KonajuGames merged commit dfbbdaf into MonoGame:develop Apr 11, 2016

5 checks passed

Build Mac, iOS, and Linux Finished TeamCity Build MonoGame :: Build Mac : Running
Details
Build Windows, Web, Android, and OUYA Finished TeamCity Build MonoGame :: Build Windows : Running
Details
Package Mac and Linux Finished TeamCity Build MonoGame :: Package Mac and Linux : Running
Details
Package Windows SDK Finished TeamCity Build MonoGame :: Package Windows : Running
Details
Test Windows Finished TeamCity Build MonoGame :: Test Windows : Tests passed: 612, ignored: 6
Details
@KonajuGames

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2016

Merged.

@tomspilman tomspilman changed the title SoundEffect Fixes and Tests SoundEffect fixes and tests Jun 15, 2016

@tomspilman tomspilman deleted the SickheadGames:sfxtest branch Jan 4, 2017

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.