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.

Sign up for GitHub

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

Jump to bottom

RTC: Fix NACK negotiation bug for Firefox. #2373

Merged
merged 1 commit into from
Jul 24, 2021
Merged

RTC: Fix NACK negotiation bug for Firefox. #2373

merged 1 commit into from
Jul 24, 2021

Conversation

duiniuluantanqin
Copy link
Member

@duiniuluantanqin duiniuluantanqin commented May 24, 2021 •
edited by winlinvip
Loading

Reason

  1. The answer sdp in SRS reply does not have the rtcp-fb nack field.
  2. Chrome enables nack by default, so even if SRS's answer does not enable NACK, it is not a problem.
  3. Firefox disables nack by default, so if SRS's answer does not enable NACK, it will cause packet loss and stuttering.

Conclusion

During network jitter, Firefox does not send nack, resulting in video stuttering.

Solution

Adding the rtcp-fb nack field is equivalent to manually enabling Firefox's nack function, which resolves stuttering caused by packet loss.

@see #2371

TRANS_BY_GPT3

@duiniuluantanqin duiniuluantanqin changed the title explicitly enable nack, for firefox explicitly enable nack, for firefox. //@ see https://github.com/ossrs/srs/issues/2371 May 24, 2021
@duiniuluantanqin duiniuluantanqin changed the title explicitly enable nack, for firefox. //@ see https://github.com/ossrs/srs/issues/2371 explicitly enable nack, for firefox. // @see https://github.com/ossrs/srs/issues/2371 May 24, 2021
@duiniuluantanqin duiniuluantanqin changed the title explicitly enable nack, for firefox. // @see https://github.com/ossrs/srs/issues/2371 explicitly enable nack, for firefox May 24, 2021
@duiniuluantanqin duiniuluantanqin mentioned this pull request May 24, 2021
Closed
@winlinvip winlinvip linked an issue May 25, 2021 that may be closed by this pull request
RTMP streaming with WebRTC playback, works fine on Chrome but experiences packet loss and buffering on Firefox. #2371
Closed
@winlinvip
Copy link
Member

winlinvip commented May 28, 2021 •
edited
Loading

I confirmed that this is indeed a bug and can be reproduced.

I need to supplement regression testing to prevent similar issues from occurring again in the future.

TRANS_BY_GPT3

@winlinvip winlinvip changed the title explicitly enable nack, for firefox RTC: Fix NACK negotiation bug for Firefox. May 28, 2021
@winlinvip
Copy link
Member

winlinvip commented Jun 30, 2021 •
edited
Loading

Scenarios to be added for regression testing:

  1. If there is a nack in the client's offer and the SRS configuration enables nack, then the answer must have a nack.
  2. If there is no nack in the client's offer or the SRS configuration disables nack, then the answer must not have a nack.

TRANS_BY_GPT3

@winlinvip winlinvip merged commit fb73d42 into ossrs:develop Jul 24, 2021
winlinvip pushed a commit to winlinvip/srs that referenced this pull request Jul 24, 2021
@duiniuluantanqin @winlinvip
explicitly enable nack, for firefox (ossrs#2373)
43bb966
winlinvip added a commit that referenced this pull request Jul 24, 2021
@winlinvip @duiniuluantanqin
For #2371: Add regression test for SDP nack (#2487)
8c7ba05 
* explicitly enable nack, for firefox (#2373)

* For #2371: Add regression test for SDP nack

1. Refine API to change defaule decodes.
2. Add test for publish SDP nack.
3. Add test for play SDP nack.

Co-authored-by: Haibo Chen <495810242@qq.com>
zhouxiaojun2008 added a commit to zhouxiaojun2008/srs that referenced this pull request Sep 18, 2022
@zhouxiaojun2008
Revert "explicitly enable nack, for firefox (ossrs#2373)"
465e3e3 
This reverts commit fb73d42.
@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
TransByAI Translated by AI/GPT.
Projects
None yet
Milestone
No milestone
2 participants
@duiniuluantanqin @winlinvip