Allow case where parity shard count is greater than data shard count #4

Merged

cgutman merged 2 commits from ps_limit into master

2023-01-12 14:59:32 -06:00

cgutman commented

2023-01-11 23:06:50 -06:00

(Migrated from github.com)

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.

I'm working on replacing the Reed-Solomon implementation in [Sunshine](https://github.com/LizardByte/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.

🚀 1

sleepybishop commented

2023-01-12 14:56:10 -06:00

(Migrated from github.com)

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.

Thank you for the PR! One of the optimizations I made was to keep allocations to a minimum, so `reed_solomon_new()` [preallocates](https://github.com/sleepybishop/nanors/blob/master/rs.c#L87) the estimated space it will need for [matrix inversion](https://github.com/sleepybishop/nanors/blob/master/rs.c#L113). 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.

👍 2