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

perf: Add RLE encoding #149

Merged
merged 13 commits into from
Aug 27, 2023
Merged

perf: Add RLE encoding #149

merged 13 commits into from
Aug 27, 2023

Conversation

ianshade
Copy link
Contributor

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Performance improvement for uploading stills and clips that benefit from RLE

  • What is the current behavior? (You can also link to an open issue here)
    Frames are sent uncompressed, which consistently takes about 600ms per 1920x1080 frame

  • What is the new behavior (if this is a feature change)?
    Encodes still and clip frames with Run-Length Encoding supported by ATEM switchers, making some frames (those that have lots of solid color areas) upload significantly faster (even down to 100ms).
    Also increases MAX_PACKETS_TO_SEND_PER_TICK to 50 which seems to be the sweet spot.

  • Other information:
    This currently also includes changes from fix: prevent 'Lost lock mid-transfer' error with multiple transfers #147

    On my 12th Gen Intel notebook CPU encoding takes about 30-40ms for a 1920x1080 frame, no matter how compressible it is. Should we try WASM with SIMD for the color space conversion and RLE to squeeze out some more performance? Should we make RLE optional? - I'm open for suggestions.

    The code is ready for review, but some help is needed to fix existing tests that I believe have wrong hashes in them.

…re queued

makes the locking more robust by:
unlocking only when queue is empty,
aborting transfers when an unexpected unlock occurs, but not when
a transfer is queued between sending the unlocking LockStateCommand
and receiving LockStateUpdateCommand back
@Julusian
Copy link
Member

Julusian commented Jul 24, 2023

Nice, this is one of things that I have been wondering about, but the benefit was too much of an unknown for me to be able to motivate.

I'll give this and your other PR a proper read next week.

Should we make RLE optional?

I think it would be good to make it opt out. Perhaps always disabled when running in single-threaded mode, as that 30-40ms of cpu time might be long enough for the connection to drop?
Making it opt-out gives a nice escape hatch in case it does cause connection issues, or for when you know that the RLE will do nothing (eg, source is from a jpeg).
I think this should be specified with an options object, so that we can add some other options later, like specifying the input format or output colourspace. (we currently assume its a RGBA buffer, and converting to 709)

Should we try WASM with SIMD for the color space conversion and RLE to squeeze out some more performance?

If we were to move the RGB->YUV into that too then WASM will probably be beneficial. The problem with WASM is that the pixels will need to be copied from a nodejs buffer into a WASM buffer, and the inverse on the way out, so if not careful it could be more costly.
I prototyped a similar conversion in my streamdeck library, which did halve the time of the buffer operations, but the numbers (for 72x72 pixels) were small enough to make the benefit questionable

Copy link
Member

@Julusian Julusian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could make it opt-out as described in #149 (comment), then I am happy.

I am open to doing some wasm, that might be better as a follow up PR? I don't mind. (I am tempted to implement that myself)

src/dataTransfer/dataTransferUploadBuffer.ts Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2023

Codecov Report

Patch coverage: 90.47% and project coverage change: +0.05% 🎉

Comparison is base (3a331ff) 86.23% compared to head (bb9a1dc) 86.28%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   86.23%   86.28%   +0.05%     
==========================================
  Files         177      180       +3     
  Lines        5709     5813     +104     
  Branches      941      957      +16     
==========================================
+ Hits         4923     5016      +93     
- Misses        764      775      +11     
  Partials       22       22              
Files Changed Coverage Δ
src/atem.ts 20.24% <0.00%> (-0.01%) ⬇️
src/index.ts 100.00% <ø> (ø)
src/dataTransfer/dataTransferUploadBuffer.ts 75.00% <88.88%> (+2.65%) ⬆️
src/lib/atemUtil.ts 63.01% <95.00%> (+12.07%) ⬆️
src/dataTransfer/dataTransferUploadAudio.ts 100.00% <100.00%> (ø)
src/dataTransfer/dataTransferUploadClip.ts 89.85% <100.00%> (+0.30%) ⬆️
src/dataTransfer/dataTransferUploadMacro.ts 100.00% <100.00%> (ø)
...dataTransfer/dataTransferUploadMultiViewerLabel.ts 100.00% <100.00%> (ø)
src/dataTransfer/dataTransferUploadStill.ts 100.00% <100.00%> (ø)
src/dataTransfer/index.ts 89.91% <100.00%> (+1.12%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Julusian Julusian merged commit 4c06389 into nrkno:master Aug 27, 2023
6 of 7 checks passed
@Julusian
Copy link
Member

I have made this opt-out, and this is now released.

I also started playing around with doing the RGBA->YUVA conversion in a native library in #152, we could look at moving the RLE to that too later on, once/if that gets merged

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.

3 participants