Skip to content

Commit

Permalink
[core] Use setDataPacketTS to timestamp data packets (#2489).
Browse files Browse the repository at this point in the history
  • Loading branch information
@maxsharabayko
maxsharabayko committed Oct 17, 2022
1 parent e0a25d0 commit f637035
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 72 deletions.
107 changes: 54 additions & 53 deletions srtcore/core.cpp
View file
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
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

0 comments on commit f637035

Please sign in to comment.