This method is a bit weird and I think should be moved to the one caller as a private helper.

• If you pass in a MemoryStream, we don't advance the stream position.
• You're calling this to try and avoid an alloc/copy, but if you don't have a MemoryStream we're going to alloc a new array. A caller might prefer a TryGet... method instead, in case it could reuse an existing buffer in the non-MemoryStream case.

buf.AsSpan().Slice(offset, len)

here you truncate, but below you don't.

below it's already the right size

To avoid polluting the existing classes with extra API surface here, perhaps instead the Stream could detect a ReadAllBytes call by checking if a call to Read will consume the entire Stream - when it does it can run the fast-path scenario internally.

Do you mean codegen?

No, I mean in the Read method of the Stream you could do something conceptually akin to:

public override int Read(Span<byte> buffer)
{
  if (position == 0 && buffer.Length == totalLengthOfFile)
  {
    // This is a request to read the whole stream in one go - use the fast-path here.
  }

  // This is a request to read a portion of the file, use the slow-path.
}

This means we can have a fast-path and slow-path - but they're both exposed via the existing Stream abstraction, so we don't need a new API surface for getting a byte array directly.

Or if that's still too late in the process - then copy things into a MemoryStream ahead of time and return that instead.

above you have the same code for a diff audio format.

feels like this function should be in a base class. or a helper.

it's here just a s place holder. Ideally every format would implement their own fast path

If I recall from the spec these files can have a form of compression applied, so they might be larger once uncompressed?

I see the commit is saying the chunk for the uncompressed size is often not reliable - but even if we're ignoring what the file claims will be the final size, I'm not seing how we're dealing with it here?

we are calculating the total uncompressed size (unrelated: in many ways this is the bound check)

ImaAdpcm format is the compression, it reduces file size to 1/4th the size and the headers do remain uncompressed

We'll need to validate the channel count is reasonable - wouldn't want to stack allocate if the file claimed to have short.MaxValue channels for example.

it should always be 1 or 2 channels. We could of course reject if there are more

The reason it's important is that the process can't recover from a stack overflow - so a malformed or malicious file will crash the process if we stackalloc too large a value. For other parsing errors we could try-catch and just not play the audio - but a stack overflow kills our process outright.

Unsafe code for an operation as dangerous as file parsing to avoid a few bounds checks is insane to me.

we are working on already bound checked data, removing bound checks in hot loops did improve the perf by 3-4%

But the bounds check you've calculated is based on input controlled by the file. You can't trust the file. If the file is malformed or malicious, then we won't have the correct bounds.

We appear to have exactly those sorts of bugs here, we'll read input of length dataSize - which is the chunk size the file claims to have. And we write output based on the calculation of the output size. This is based on other inputs from the file such as blockAlign and channels.

A well-formed file will have everything match up so we consume the right amount of input and produce the right amount of output. A malformed or malicious file can make false claims where things don't line up, meaning we'll walk off the end of the input or output arrays.

If that happened in our existing parsers, we'd get an array out of bounds exception and could recover and keep running. By using unsafe, we've elevated the bug into either an unrecoverable crash if we read/write out of our process memory - or worse a security issue if the file is controlled by an attacker and the bad accesses can be manipulated in a manner advantageous to them.

If we used safe code, then at least bugs like this have reduced impact potiential - by dropping to unsafe we've signed ourselves up to much worse outcomes if the code contains any bugs.

If I had a magical lever to downgrade the impact of bugs I wrote from "security issue/denial-of-service" to "mostly recoverable" and it only cost me 3-4% perf, I'd pull that lever most times (in this case, the lever is "don't use unsafe code to skip bounds checks")

Also, if we really are sensitive to 3-4% perf then the better solution is to load those files up-front behind the load screen and keep them in ram!

I would put this init block in a separate function. Assuming you don't reference too many local vars.

There's so little code it's using all local variables

We're decoding in a batch of 8 at a time, I assume as a form of loop unrolling?

Is there a guarentee that chunks have a multiple of 8 in them? If not, we're need another loop to catch the leftovers or we're going to miss out some samples.

it is guaranteed, if there's an incomplete group at the end of the file we should just skip it

though, the fast path will just fail and decode garbage