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

Streaming Wav and Voc audio. #14043

Merged
merged 2 commits into from Sep 24, 2017

Conversation

Projects
None yet
6 participants
@RoosterDragon
Member

RoosterDragon commented Sep 17, 2017

Followup to #13521.

This allows the Wav and Voc classes to provide actual streaming data for their audio, rather than having to load it into memory and return a MemoryStream. This means we can leave the file sitting on disk, rather than it sitting in memory. Mods using these audio types will therefore require less memory.

Tested Wavs successfully in RA2. I haven't tested Voc but hopefully the changes can be verified by inspection, they're not too bad.

stream = SegmentStream.CreateWithoutOwningStream(cloneFrom.stream, 0, (int)cloneFrom.stream.Length);
blocks = cloneFrom.blocks;
totalSamples = cloneFrom.totalSamples;
Rewind();
}

This comment has been minimized.

@RoosterDragon

RoosterDragon Sep 17, 2017

Member

VocFormat already implemented the hard part of providing a stream, all we really need to do is to be able to clone it so we can hand out as many as needed on demand.

The cloning is all done here, the main bit is that we create a clone of the source stream, so we don't mess with the original.

The rest of the changes in here are just designed to make it more straightforward to see the cloning is correct. For example, but marking some of the fields as readonly - these represent the fixed data that doesn't change. The three remaining non-readonly fields represent the stream position, and are all reset in the Rewind method.

@RoosterDragon

RoosterDragon Sep 17, 2017

Member

VocFormat already implemented the hard part of providing a stream, all we really need to do is to be able to clone it so we can hand out as many as needed on demand.

The cloning is all done here, the main bit is that we create a clone of the source stream, so we don't mess with the original.

The rest of the changes in here are just designed to make it more straightforward to see the cloning is correct. For example, but marking some of the fields as readonly - these represent the fixed data that doesn't change. The three remaining non-readonly fields represent the stream position, and are all reset in the Rewind method.

for (var i = 0; i < decoded.Length; i += 2)
{
var outOffsetSample = outOffsetChannel + i;
if (outOffsetSample >= outputSize)
return output;

This comment has been minimized.

@RoosterDragon

RoosterDragon Sep 17, 2017

Member

Removing this was intentional, we no longer bail out whilst in the middle of decoding each channel.

@RoosterDragon

RoosterDragon Sep 17, 2017

Member

Removing this was intentional, we no longer bail out whilst in the middle of decoding each channel.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 17, 2017

Contributor

OpenRA.Mods.Common/FileFormats/WavReader.cs(34,8): error CS0219: Warning as Error: The variable 'fileSize' is assigned but its value is never used
OpenRA.Mods.Common/FileFormats/WavReader.cs(62,11): error CS0219: Warning as Error: The variable 'byteRate' is assigned but its value is never used

Contributor

reaperrr commented Sep 17, 2017

OpenRA.Mods.Common/FileFormats/WavReader.cs(34,8): error CS0219: Warning as Error: The variable 'fileSize' is assigned but its value is never used
OpenRA.Mods.Common/FileFormats/WavReader.cs(62,11): error CS0219: Warning as Error: The variable 'byteRate' is assigned but its value is never used

var outputRemaining = outputSize - outOffset;
var toCopy = Math.Min(outputRemaining, interleaveBuffer.Length);
for (var i = 0; i < toCopy; i++)
data.Enqueue(interleaveBuffer[i]);

This comment has been minimized.

@RoosterDragon

RoosterDragon Sep 17, 2017

Member

Instead of bailing out mid-decoding of channels, we decode them all, but then only return as much data as expected.

This represents a bug-fix: Previously the early bail out would mean some bytes right at the end would not be decoded and would be left with their default value of 0. Now we decode these and set their values correctly.

Both the old and new methods output the same number of bytes, it's just the left few bytes in the old method would often be mistakenly zeroed.

@RoosterDragon

RoosterDragon Sep 17, 2017

Member

Instead of bailing out mid-decoding of channels, we decode them all, but then only return as much data as expected.

This represents a bug-fix: Previously the early bail out would mean some bytes right at the end would not be decoded and would be left with their default value of 0. Now we decode these and set their values correctly.

Both the old and new methods output the same number of bytes, it's just the left few bytes in the old method would often be mistakenly zeroed.

Providing streaming WavFormat data.
WavFormat.GetPCMInputStream now returns data that is streamed, rather than a MemoryStream.
@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Sep 17, 2017

Member

Odd that the compiler on Travis picks that up, but neither Appveyor nor VS2017 seem to mind. Fixed nonetheless.

Member

RoosterDragon commented Sep 17, 2017

Odd that the compiler on Travis picks that up, but neither Appveyor nor VS2017 seem to mind. Fixed nonetheless.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Sep 17, 2017

Contributor

I could never get Windows to care about CS0219.

Contributor

GraionDilach commented Sep 17, 2017

I could never get Windows to care about CS0219.

Allow multiple VocFormat streams.
VocFormat.GetPCMInputStream now returns independent streams, allowing multiple instances of the same source to be streamed.
@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Sep 18, 2017

Member

Sound effects in d2k still play. Sadly I don't know enough to verify the code.

Member

Mailaender commented Sep 18, 2017

Sound effects in d2k still play. Sadly I don't know enough to verify the code.

@pchote

pchote approved these changes Sep 24, 2017

The code changes look reasonable, and confirmed the fixes for Wav format.

@pchote pchote merged commit 7160c8a into OpenRA:bleed Sep 24, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 24, 2017

Member

The d2 mod currently requires a customized version of release-20161019, so we don't have any use or test cases for the VocLoader code. This is a problem. We recently moved the audio loaders from engine code to mod code, so IMO we should remove VocLoader and let the d2 mod adopt it in their mod dll whenever they get around to upgrading.

Member

pchote commented Sep 24, 2017

The d2 mod currently requires a customized version of release-20161019, so we don't have any use or test cases for the VocLoader code. This is a problem. We recently moved the audio loaders from engine code to mod code, so IMO we should remove VocLoader and let the d2 mod adopt it in their mod dll whenever they get around to upgrading.

@RoosterDragon RoosterDragon deleted the RoosterDragon:moreAudioStreaming branch Sep 26, 2017

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Mar 26, 2018

Member

Tested Wavs successfully in RA2.

Sorry, something must have gone wrong there. After this commit those don't work anymore (i.e. reverting this 'fixes' sounds not working).

Member

abcdefg30 commented Mar 26, 2018

Tested Wavs successfully in RA2.

Sorry, something must have gone wrong there. After this commit those don't work anymore (i.e. reverting this 'fixes' sounds not working).

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Mar 26, 2018

Member

Same behaviour as #14153? Can you note anything new on there?

Member

RoosterDragon commented Mar 26, 2018

Same behaviour as #14153? Can you note anything new on there?

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Mar 26, 2018

Member

It just played the first audio file inside the .bag for every sound.

Member

abcdefg30 commented Mar 26, 2018

It just played the first audio file inside the .bag for every sound.

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Mar 29, 2018

Member

Can you create a new issue please? Also please include the exact commits, repro steps and OS. If this involves RA2, I need commits for the engine version you can repro this on.

Member

RoosterDragon commented Mar 29, 2018

Can you create a new issue please? Also please include the exact commits, repro steps and OS. If this involves RA2, I need commits for the engine version you can repro this on.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Mar 31, 2018

Member

Checkout this pull request OpenRA/ra2#459 and change mod.config to

-# HACK HACK HACK: Use a custom engine which reverts https://github.com/OpenRA/OpenRA/pull/14043
-ENGINE_VERSION="147557e"
+ENGINE_VERSION="release-20180307"
Member

Mailaender commented Mar 31, 2018

Checkout this pull request OpenRA/ra2#459 and change mod.config to

-# HACK HACK HACK: Use a custom engine which reverts https://github.com/OpenRA/OpenRA/pull/14043
-ENGINE_VERSION="147557e"
+ENGINE_VERSION="release-20180307"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment