Skip to content

fix: nack timer not work as expected - #1076

Closed
o-u-p wants to merge 1 commit into
versatica:v3from
o-u-p:fix_nack_timer
Closed

fix: nack timer not work as expected#1076
o-u-p wants to merge 1 commit into
versatica:v3from
o-u-p:fix_nack_timer

Conversation

@o-u-p

@o-u-p o-u-p commented May 6, 2023

Copy link
Copy Markdown
Contributor

No description provided.

ibc
ibc reviewed May 6, 2023
View reviewed changes

@ibc ibc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you explain rationale of these changes? Was what wrong before them?

@o-u-p

o-u-p commented May 6, 2023

Copy link
Copy Markdown
Contributor Author

Can you explain rationale of these changes? Was what wrong before them?

if not check 0 rtt
if (filter == NackFilter::TIME && (nackInfo.sentAtMs == 0 || nowMs - nackInfo.sentAtMs >= this->rtt))
will send nack too quickly.
if not stop nack timer, when nack list is empty, it will never restart, if stop the timer, it will get started here

if (!this->timer->IsActive())
		MayRunTimer();

ibc
ibc reviewed May 18, 2023
View reviewed changes
Comment thread worker/src/RTC/RtpStreamRecv.cpp
// Tell it to the NackGenerator.
if (this->params.useNack)
{
this->nackGenerator->UpdateRtt(static_cast<uint32_t>(this->rtt));

@ibc ibc May 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are you just updating current RTT value in NackGenerator only if > 0.0f? This is wrong. It means that if negative then NackGenerator will keep using its previous given RTT value.

@ibc

ibc commented May 18, 2023

Copy link
Copy Markdown
Member

if not stop nack timer, when nack list is empty, it will never restart, if stop the timer, it will get started here

if (!this->timer->IsActive())
		MayRunTimer();

Makes sense.

@ibc

ibc commented May 18, 2023

Copy link
Copy Markdown
Member

I'm including some valid changes of this PR into another one. Thanks @o-u-p

ibc added a commit that referenced this pull request May 18, 2023
@ibc
worker: Fix NACK timer and avoid negative RTT
6d78d66 
Based on PR #1076 of @o-u-p

### Details

- Stop NACK timer when NACK list is empty as explained in #1076 (comment).
- Avoid RTT becoming negative in `RtpStreamSend` and `RtpStreamRecv`. If so, assign it with 0.0f.
- And if 0.0f, use `DefaultRtt` (100 ms) in `NackGenerator`.
- Remove useless `RtpStream::hasRtt` and check `RtpStream::rtt > 0.0f` instead.
- Add some brackets in condition blocks.
@ibc ibc mentioned this pull request May 18, 2023
Merged
@ibc

ibc commented May 18, 2023

Copy link
Copy Markdown
Member

Closing this PR in favour of #1082

@ibc ibc closed this May 18, 2023
ibc added a commit that referenced this pull request May 18, 2023
@ibc
worker: Fix NACK timer and avoid negative RTT (#1082)
75c11ee 
- Stop NACK timer when NACK list is empty as explained in #1076 (comment).
- Avoid RTT becoming negative in `RtpStreamSend` and `RtpStreamRecv`. If so, assign it with 0.0f.
- And if 0.0f, use `DefaultRtt` (100 ms) in `NackGenerator`.
- Remove useless `RtpStream::hasRtt` and check `RtpStream::rtt > 0.0f` instead.
- Add some brackets in condition blocks.
@ibc

ibc commented May 18, 2023

Copy link
Copy Markdown
Member

Released in mediasoup 3.11.26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ibc ibc ibc left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@o-u-p @ibc