Skip to content

Conversation

cgutman
Copy link
Contributor

@cgutman cgutman commented Jan 12, 2023

I'm working on replacing the Reed-Solomon implementation in Sunshine, an NVIDIA GameStream host, with nanors (with great performance results). For the most part it was a drop-in replacement, however there is one case which nanors doesn't currently support: ps > ds

There is a configurable attribute in the SDP used by GameStream that allows the client to specify a minimum parity shard count per frame. NVIDIA's official clients and Moonlight both set this to 2, which means we can sometimes encounter a case where we have a small frame with only 1 data shard that will be generating 2 parity shards, which fails the check here.

This check appears to be superfluous, so I've removed it and updated the tests and benchmark to exercise that case. The tests required a bit more work than would be otherwise necessarily since it had assumptions that 2 * K >= K + N in various places that needed to be adjusted. I also updated them to ensure that a failure of reed_solomon_new() would result in a FAILED output, rather than just outputting nothing.

I locally ran make check and make ubsan to verify this change.

NVIDIA GameStream can request a minimum number of parity shards
and this can sometimes exceed the number of data shards.

This case is already handled properly, so there is no need to
fail reed_solomon_new() if ps > ds.
@sleepybishop
Copy link
Owner

sleepybishop commented Jan 12, 2023

Thank you for the PR!

One of the optimizations I made was to keep allocations to a minimum, so reed_solomon_new() preallocates the estimated space it will need for matrix inversion. I remember there was an issue where if there was more parity shards than data shards then the inversion workspace would get overrun. I believe I fixed that and just forgot to remove the guard.

Thank you for patching it up along with the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants