A bunch of refactoring before changes in the API #110
Conversation
| @@ -972,11 +1018,25 @@ bool CRcvBuffer::getRcvFirstMsg(ref_t<uint64_t> tsbpdtime, ref_t<bool> passack, | |||
| * Tell 1st valid packet seqno so caller can skip (drop) the missing packets. | |||
| */ | |||
| skipseqno = m_pUnit[i]->m_Packet.m_iSeqNo; | |||
| if ( pppkt ) | |||
| *pppkt = &m_pUnit[i]->m_Packet; | |||
There was a problem hiding this comment.
This does not look like a "refactoring" change. If this represents a fix that can affect logic elsewhere the change needs to be moved into a separate pull request.
There was a problem hiding this comment.
Actually this pppkt plays no logical role here. The use of it is only for logging. This function simply has to return some data that need to be processed further, but the packet itself is of no use to it. I've added extracting the packet just to be able to make the calling function look into its internals, in particular, extract the sequence number. Check core.cpp around line 3766. Previously happened that this number could be not always extracted, so the log was displaying the fallback value 0; this makes the log displaying better.
There was a problem hiding this comment.
Ok, thank you for explanation! As agreed, there will be a separate PR that will clean up declaration and usage of the packet tracing argument, changing it either to const or seq depending on the usage.
srtcore/buffer.cpp
Outdated
| @@ -1389,7 +1449,7 @@ void CRcvBuffer::printDriftOffset(int tsbPdOffset, int tsbPdDriftAvg) | |||
| } | |||
| #endif /* SRT_DEBUG_TSBPD_DRIFT */ | |||
|
|
|||
| void CRcvBuffer::addRcvTsbPdDriftSample(uint32_t timestamp) | |||
| void CRcvBuffer::addRcvTsbPdDriftSample(uint32_t timestamp, pthread_mutex_t& mutex_to_lock) | |||
There was a problem hiding this comment.
I don't think this is a refactoring change because it will change logic elsewhere. It should be possible to move this (and corresponding call changes) out to "Fix X locking mechanism" or something along those lines.
There was a problem hiding this comment.
Agreed, goes to a separate PR.
srtcore/core.cpp
Outdated
| // inaccurate. Additionally it won't lock if TSBPD mode is off, and | ||
| // won't update anything. Note that if you set TSBPD mode and use | ||
| // srt_recvfile (which doesn't make any sense), you'll have e deadlock. | ||
| m_pRcvBuffer->addRcvTsbPdDriftSample(ctrlpkt.getMsgTimeStamp(), m_RecvLock); |
There was a problem hiding this comment.
This is a significant change/fix, especially if lock is now not taken under certain conditions. Please move to a separate pull request.
|
Thank you @ethouris ! |
As per Issue #108