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

[core] Use setDataPacketTS to timestamp data packets. #2489

Merged
merged 2 commits into from
Oct 17, 2022
Merged

[core] Use setDataPacketTS to timestamp data packets. #2489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb394af
[core] Use setDataPacketTS to timestamp data packets.
maxsharabayko Oct 10, 2022
069d577
IF_HEAVY_LOGGING(reason)
maxsharabayko Oct 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 54 additions & 53 deletions srtcore/core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9006,7 +9006,7 @@ void srt::CUDT::updateAfterSrtHandshake(int hsv)
}
}

int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origintime)
int srt::CUDT::packLostData(CPacket& w_packet)
{
// protect m_iSndLastDataAck from updating by ACK processing
UniqueLock ackguard(m_RecvAckLock);
Expand Down Expand Up @@ -9061,8 +9061,8 @@ int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origi
}

int msglen;

const int payload = m_pSndBuffer->readData(offset, (w_packet), (w_origintime), (msglen));
steady_clock::time_point tsOrigin;
const int payload = m_pSndBuffer->readData(offset, (w_packet), (tsOrigin), (msglen));
if (payload == -1)
{
int32_t seqpair[2];
Expand Down Expand Up @@ -9101,6 +9101,7 @@ int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origi
{
w_packet.m_iMsgNo |= PACKET_SND_REXMIT;
}
setDataPacketTS(w_packet, tsOrigin);

return payload;
}
Expand Down Expand Up @@ -9194,6 +9195,42 @@ class snd_logger
snd_logger g_snd_logger;
#endif // SRT_DEBUG_TRACE_SND

void srt::CUDT::setPacketTS(CPacket& p, const time_point& ts)
{
enterCS(m_StatsLock);
const time_point tsStart = m_stats.tsStartTime;
leaveCS(m_StatsLock);
p.m_iTimeStamp = makeTS(ts, tsStart);
}

void srt::CUDT::setDataPacketTS(CPacket& p, const time_point& ts)
{
enterCS(m_StatsLock);
const time_point tsStart = m_stats.tsStartTime;
leaveCS(m_StatsLock);

if (!m_bPeerTsbPd)
{
// If TSBPD is disabled, use the current time as the source (timestamp using the sending time).
p.m_iTimeStamp = makeTS(steady_clock::now(), tsStart);
return;
}

// TODO: Might be better for performance to ensure this condition is always false, and just use SRT_ASSERT here.
if (ts < tsStart)
{
p.m_iTimeStamp = makeTS(steady_clock::now(), tsStart);
LOGC(qslog.Warn,
log << CONID() << "setPacketTS: reference time=" << FormatTime(ts)
<< " is in the past towards start time=" << FormatTime(tsStart)
<< " - setting NOW as reference time for the data packet");
return;
}

// Use the provided source time for the timestamp.
p.m_iTimeStamp = makeTS(ts, tsStart);
}

bool srt::CUDT::isRetransmissionAllowed(const time_point& tnow SRT_ATR_UNUSED)
{
// Prioritization of original packets only applies to Live CC.
Expand Down Expand Up @@ -9237,9 +9274,7 @@ std::pair<bool, steady_clock::time_point> srt::CUDT::packData(CPacket& w_packet)
{
int payload = 0;
bool probe = false;
steady_clock::time_point origintime;
bool new_packet_packed = false;
bool filter_ctl_pkt = false;

const steady_clock::time_point enter_time = steady_clock::now();

Expand All @@ -9248,8 +9283,6 @@ std::pair<bool, steady_clock::time_point> srt::CUDT::packData(CPacket& w_packet)
m_tdSendTimeDiff = m_tdSendTimeDiff.load() + (enter_time - m_tsNextSendTime);
}

string reason = "reXmit";

ScopedLock connectguard(m_ConnectionLock);
// If a closing action is done simultaneously, then
// m_bOpened should already be false, and it's set
Expand All @@ -9262,28 +9295,28 @@ std::pair<bool, steady_clock::time_point> srt::CUDT::packData(CPacket& w_packet)
return std::make_pair(false, enter_time);

payload = isRetransmissionAllowed(enter_time)
? packLostData((w_packet), (origintime))
? packLostData((w_packet))
: 0;

IF_HEAVY_LOGGING(const char* reason); // The source of the data packet (normal/rexmit/filter)
if (payload > 0)
{
reason = "reXmit";
IF_HEAVY_LOGGING(reason = "reXmit");
}
else if (m_PacketFilter &&
m_PacketFilter.packControlPacket(m_iSndCurrSeqNo, m_pCryptoControl->getSndCryptoFlags(), (w_packet)))
{
HLOGC(qslog.Debug, log << CONID() << "filter: filter/CTL packet ready - packing instead of data.");
payload = (int) w_packet.getLength();
reason = "filter";
filter_ctl_pkt = true; // Mark that this packet ALREADY HAS timestamp field and it should not be set
IF_HEAVY_LOGGING(reason = "filter");

// Stats
ScopedLock lg(m_StatsLock);
m_stats.sndr.sentFilterExtra.count(1);
}
else
{
if (!packUniqueData(w_packet, origintime))
if (!packUniqueData(w_packet))
{
m_tsNextSendTime = steady_clock::time_point();
m_tdSendTimeDiff = steady_clock::duration();
Expand All @@ -9296,46 +9329,10 @@ std::pair<bool, steady_clock::time_point> srt::CUDT::packData(CPacket& w_packet)
probe = true;

payload = (int) w_packet.getLength();
reason = "normal";
}

// Normally packet.m_iTimeStamp field is set exactly here,
// usually as taken from m_stats.tsStartTime and current time, unless live
// mode in which case it is based on 'origintime' as set during scheduling.
// In case when this is a filter control packet, the m_iTimeStamp field already
// contains the exactly needed value, and it's a timestamp clip, not a real
// timestamp.
if (!filter_ctl_pkt)
{
if (m_bPeerTsbPd)
{
/*
* When timestamp is carried over in this sending stream from a received stream,
* it may be older than the session start time causing a negative packet time
* that may block the receiver's Timestamp-based Packet Delivery.
* XXX Isn't it then better to not decrease it by m_stats.tsStartTime? As long as it
* doesn't screw up the start time on the other side.
*/
if (origintime >= m_stats.tsStartTime)
{
setPacketTS(w_packet, origintime);
}
else
{
setPacketTS(w_packet, steady_clock::now());
LOGC(qslog.Warn,
log << CONID() << "packData: reference time=" << FormatTime(origintime)
<< " is in the past towards start time=" << FormatTime(m_stats.tsStartTime)
<< " - setting NOW as reference time for the data packet");
}
}
else
{
setPacketTS(w_packet, steady_clock::now());
}
IF_HEAVY_LOGGING(reason = "normal");
}

w_packet.m_iID = m_PeerID;
w_packet.m_iID = m_PeerID; // Set the destination SRT socket ID.

if (new_packet_packed && m_PacketFilter)
{
Expand Down Expand Up @@ -9408,7 +9405,7 @@ std::pair<bool, steady_clock::time_point> srt::CUDT::packData(CPacket& w_packet)
return std::make_pair(payload >= 0, m_tsNextSendTime);
}

bool srt::CUDT::packUniqueData(CPacket& w_packet, time_point& w_origintime)
bool srt::CUDT::packUniqueData(CPacket& w_packet)
{
// Check the congestion/flow window limit
const int cwnd = std::min(int(m_iFlowWindowSize), int(m_dCongestionWindow));
Expand All @@ -9428,7 +9425,8 @@ bool srt::CUDT::packUniqueData(CPacket& w_packet, time_point& w_origintime)
// isn't a useless redundant state copy. If it is, then taking the flags here can be removed.
const int kflg = m_pCryptoControl->getSndCryptoFlags();
int pktskipseqno = 0;
const int pld_size = m_pSndBuffer->readData((w_packet), (w_origintime), kflg, (pktskipseqno));
time_point tsOrigin;
const int pld_size = m_pSndBuffer->readData((w_packet), (tsOrigin), kflg, (pktskipseqno));
if (pktskipseqno)
{
// Some packets were skipped due to TTL expiry.
Expand Down Expand Up @@ -9520,7 +9518,10 @@ bool srt::CUDT::packUniqueData(CPacket& w_packet, time_point& w_origintime)
w_packet.m_iSeqNo = m_iSndCurrSeqNo;
}

// Encrypt if 1st time this packet is sent and crypto is enabled
// Set missing fields before encrypting the packet, because those fields might be used for encryption.
w_packet.m_iID = m_PeerID; // Destination SRT Socket ID
setDataPacketTS(w_packet, tsOrigin);

if (kflg != EK_NOENC)
{
// Note that the packet header must have a valid seqno set, as it is used as a counter for encryption.
Expand Down
36 changes: 17 additions & 19 deletions srtcore/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,22 +376,29 @@ class CUDT
return m_config.bMessageAPI ? (len+ps-1)/ps : 1;
}

int32_t makeTS(const time_point& from_time) const
static int32_t makeTS(const time_point& from_time, const time_point& tsStartTime)
{
// NOTE:
// - This calculates first the time difference towards start time.
// - This difference value is also CUT OFF THE SEGMENT information
// (a multiple of MAX_TIMESTAMP+1)
// So, this can be simply defined as: TS = (RTS - STS) % (MAX_TIMESTAMP+1)
// XXX Would be nice to check if local_time > m_tsStartTime,
// otherwise it may go unnoticed with clock skew.
return (int32_t) sync::count_microseconds(from_time - m_stats.tsStartTime);
SRT_ASSERT(from_time >= tsStartTime);
return (int32_t) sync::count_microseconds(from_time - tsStartTime);
}

void setPacketTS(CPacket& p, const time_point& local_time)
{
p.m_iTimeStamp = makeTS(local_time);
}
/// @brief Set the timestamp field of the packet using the provided value (no check)
/// @param p the packet structure to set the timestamp on.
/// @param ts timestamp to use as a source for packet timestamp.
SRT_ATTR_EXCLUDES(m_StatsLock)
void setPacketTS(CPacket& p, const time_point& ts);

/// @brief Set the timestamp field of the packet according the TSBPD mode.
/// Also checks the connection start time (m_tsStartTime).
/// @param p the packet structure to set the timestamp on.
/// @param ts timestamp to use as a source for packet timestamp. Ignored if m_bPeerTsbPd is false.
SRT_ATTR_EXCLUDES(m_StatsLock)
void setDataPacketTS(CPacket& p, const time_point& ts);

// Utility used for closing a listening socket
// immediately to free the socket
Expand Down Expand Up @@ -437,16 +444,13 @@ class CUDT

private:
/// initialize a UDT entity and bind to a local address.

void open();

/// Start listening to any connection request.

void setListenState();

/// Connect to a UDT entity listening at address "peer".
/// @param peer [in] The address of the listening UDT entity.

void startConnect(const sockaddr_any& peer, int32_t forced_isn);

/// Process the response handshake packet. Failure reasons can be:
Expand All @@ -457,7 +461,6 @@ class CUDT
/// @retval 0 Connection successful
/// @retval 1 Connection in progress (m_ConnReq turned into RESPONSE)
/// @retval -1 Connection failed

SRT_ATR_NODISCARD SRT_ATTR_REQUIRES(m_ConnectionLock)
EConnectStatus processConnectResponse(const CPacket& pkt, CUDTException* eout) ATR_NOEXCEPT;

Expand Down Expand Up @@ -1029,20 +1032,15 @@ class CUDT
void updateSndLossListOnACK(int32_t ackdata_seqno);

/// Pack a packet from a list of lost packets.
///
/// @param packet [in, out] a packet structure to fill
/// @param origintime [in, out] origin timestamp of the packet
///
/// @return payload size on success, <=0 on failure
int packLostData(CPacket &packet, time_point &origintime);
int packLostData(CPacket &packet);

/// Pack a unique data packet (never sent so far) in CPacket for sending.
///
/// @param packet [in, out] a CPacket structure to fill.
/// @param origintime [in, out] origin timestamp of the packet.
///
/// @return true if a packet has been packets; false otherwise.
bool packUniqueData(CPacket& packet, time_point& origintime);
bool packUniqueData(CPacket& packet);

/// Pack in CPacket the next data to be send.
///
Expand Down