Skip to content
This repository has been archived by the owner on Aug 23, 2022. It is now read-only.

Fixes overflow panic in stream counters #12

Merged
merged 1 commit into from
Aug 6, 2022

Conversation

nullsauce
Copy link

@nullsauce nullsauce commented Aug 2, 2022

Hey. This should fix #7

I've implemented the wrapped adds for the two counters inside a dedicated struct so that the behavior is isolated and easy to test. Apart from running the test suite, is there anything else I should check ? I'm new to the project, any comments / directions are welcome.

I've put the tests in sender_test.rs for consistency but would have preferred putting them in sender_stream.rs to avoid having to make Counter pub(crate) visible and make Counter::mock a test helper function. What do you think ?

Edit: Tested this fix with webrtc-rs/webrtc 0.4.0 and I was able to display a 4k 30fps h264 stream in Firefox for more than 3 hours without any overflow panic happening.

@xnorpx
Copy link
Contributor

xnorpx commented Aug 4, 2022

@k0nserv would be nice to get this in.

@k0nserv
Copy link
Member

k0nserv commented Aug 4, 2022

On holiday atm, will be back on the 16th of August. Please ask someone else in with merge permissions if it's urgent

@rainliu rainliu merged commit 739abd5 into webrtc-rs:main Aug 6, 2022
@nullsauce nullsauce deleted the fix/counters-overflow branch August 6, 2022 06:08
@xnorpx
Copy link
Contributor

xnorpx commented Aug 6, 2022

@rainliu thanks for merge can we do patch release for this?

@nullsauce nullsauce restored the fix/counters-overflow branch August 6, 2022 17:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add with overflow panic
4 participants