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

Rtcp sr and rr for rtc play #3748

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Conversation

runner365
Copy link
Contributor

@runner365 runner365 commented Jul 30, 2023

For WebRTC downlink, add:

  • Sending RTCP SR.
  • Receiving and processing RTCP RR to calculate RTT.
  • Retransmission of NACK based on RTT.

The main functionality is to calculate RTT in the downlink, which prevents unnecessary retransmissions within a time period less than RTT, avoiding unnecessary retransmissions.


TRANS_BY_GPT3

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Jul 30, 2023
@runner365 runner365 force-pushed the rtcp_update branch 2 times, most recently from 51c4d56 to a627c54 Compare August 1, 2023 13:52
@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Aug 2, 2023
@@ -448,6 +450,9 @@ SrsRtcPlayStream::~SrsRtcPlayStream()

_srs_config->unsubscribe(this);

if (timer_rtcp_) {
srs_freep(timer_rtcp_);
Copy link
Member

@winlinvip winlinvip Aug 3, 2023

Choose a reason for hiding this comment

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

You can simply release the object directly, srs_freep will check if it is empty.

srs_freep(timer_rtcp_);

TRANS_BY_GPT3

@@ -681,6 +686,29 @@ srs_error_t SrsRtcPlayStream::cycle()
}
}

srs_error_t SrsRtcPlayStream::send_rtcp_sr(int64_t now_ms) {
Copy link
Member

@winlinvip winlinvip Aug 3, 2023

Choose a reason for hiding this comment

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

Please keep the coding style consistent, and the opening brace of member functions should be on a new line.

srs_error_t SrsRtcPlayStream::send_rtcp_sr(int64_t now_ms) 
{

TRANS_BY_GPT3

srs_error_t err = srs_success;
for(std::map<uint32_t, SrsRtcVideoSendTrack*>::iterator iter = video_tracks_.begin();
iter != video_tracks_.end();
iter++) {
Copy link
Member

@winlinvip winlinvip Aug 3, 2023

Choose a reason for hiding this comment

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

These three lines can be written in one line, now the screen and editor are both very long, and can tolerate longer lines. Please change it to:

    for(std::map<uint32_t, SrsRtcVideoSendTrack*>::iterator iter = video_tracks_.begin(); iter != video_tracks_.end(); iter++) {

TRANS_BY_GPT3

sr->set_rtp_send_packets(send_count_);
sr->set_rtp_send_bytes(send_bytes_);

char data[1500];
Copy link
Member

@winlinvip winlinvip Aug 3, 2023

Choose a reason for hiding this comment

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

There should not be a magic number 1500, but instead use existing constants or macro definitions. You can refer to the serialization of other RTC objects, I remember there will be a buffer cache and a method to specify the corresponding length.

TRANS_BY_GPT3

char data[1500];
SrsBuffer buffer(data, sr->nb_bytes());
sr->encode(&buffer);
delete sr;
Copy link
Member

@winlinvip winlinvip Aug 3, 2023

Choose a reason for hiding this comment

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

SrsAutoFree should be used instead of naked delete to automatically release memory upon creation, in order to avoid forgetting to delete sr and causing memory leaks.

SrsRtcpSR* sr = new SrsRtcpSR();
SrsAutoFree(SrsRtcpSR, sr);

TRANS_BY_GPT3

send_count_++;
send_bytes_ += len;
last_rtp_pkt_ts_ = rtp_ts;
last_rtp_ms_ = srs_update_system_time() / 1000;//ms
Copy link
Member

@winlinvip winlinvip Aug 3, 2023

Choose a reason for hiding this comment

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

Do not directly use int64_t and ms, instead use srs_utime_t and srsu2ms conversion to avoid unit errors and make it easier to understand.

TRANS_BY_GPT3

return video_iter->second->handle_rtcp_rr(rb, now_ms);
}
}
srs_warn("rtcp rr find to find track by ssrc:%u", ssrc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

void update_rtp_static(int64_t len, uint32_t rtp_ts);
public:
srs_error_t handle_rtcp_rr(const SrsRtcpRB& rb, int64_t now_ms);
protected:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Directly assigning values to member variables in a C++ class seems to be a syntax introduced in C++11.

interval = (interval > 10) ? (interval - 10) : interval;//for resend interval Residual
if (diff_t < (int64_t)interval) {
return NULL;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should limit the count of retrans packet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return err;
}

srs_utime_t now_ms = srs_update_system_time() / 1000;
Copy link
Member

@winlinvip winlinvip Aug 11, 2023

Choose a reason for hiding this comment

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

You cannot divide by 1000 here, because this function returns srs_utime_t, and the variable cannot have a unit, because this type already has a unit.

It should be changed to:

srs_utime_t now = srs_update_system_time();

if ((err = p_->send_rtcp_sr(now)) != srs_success) {

Your send_rtcp_sr should not be in milliseconds or seconds, it should be srs_utime_t, because it has a unit, which is a time unit. Don't convert it to milliseconds or seconds, only convert it to milliseconds using srsu2ms when it is finally used.

TRANS_BY_GPT4

@@ -2710,6 +2743,70 @@ void SrsRtcSendTrack::rebuild_packet(SrsRtpPacket* pkt)
srs_info("RTC: Correct %s seq=%u/%u, ts=%u/%u", track_desc_->type_.c_str(), seq, pkt->header.get_sequence(), ts, pkt->header.get_timestamp());
}

srs_error_t SrsRtcSendTrack::send_rtcp_sr(srs_utime_t now_ms) {
Copy link
Member

@winlinvip winlinvip Aug 11, 2023

Choose a reason for hiding this comment

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

This srs_utime_t is utime, which is in microseconds (us), so the variable should not carry the unit, or the variable should be now_us. However, this type is already in microseconds, so there is no need to carry the us unit. It should be changed to:

srs_error_t SrsRtcSendTrack::send_rtcp_sr(srs_utime_t now) {
    srs_utime_t diff = now_ms - last_rtp_ms_;
    srs_utime_t diff_ts = diff * track_desc_->media_->sample_ / 1000;
    srs_utime_t video_rtp_ts = last_rtp_pkt_ts_ + diff_xxx;

You should convert track_desc_->media_->sample_ to microseconds (us), so it can be directly added or subtracted with srs_utime_t.

TRANS_BY_GPT4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English. TransByAI Translated by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants