refactor: rtp packetization refactor and unit test #386
Conversation
yezhan10
changed the title
include/rtp/RTPPacketizer.h
Outdated
| @@ -25,6 +132,53 @@ class RTPPacketizer | |||
| protected: | |||
| MediaFrame::Type mediaType; | |||
| DWORD codec; | |||
| private: | |||
| static size_t CalculateAggregationEndIndex(const std::shared_ptr<MediaFrame>& frame, size_t startIdx, uint32_t maxSize) | |||
There was a problem hiding this comment.
use const MediaFrame::RtpPacketizationInfo& info instead of frame as input
There was a problem hiding this comment.
if we are going to extract this function, let's move it to an h264xxx.h file instead
There was a problem hiding this comment.
Not sure if I prefer to move it to h264Packetizer.h since CalculateAggregationEndIndex() and AggregatePayloads() are not h264 particular. The function would be the same regardless of the codec type, so it makes less sense to me to move them to a file that pertaining to a specific codec.
There was a problem hiding this comment.
they make no sense for codecs other than h264
There was a problem hiding this comment.
ok, i thought that stap-a can be applied to other codec as well. We just support stapA for h264 now. If this is the case, I will move it to h264packetizer instead
include/rtp/RTPPacketizer.h
Outdated
| return last; | ||
| } | ||
|
|
||
| static void AggregatePayloads(const std::shared_ptr<MediaFrame>& frame, RTPPacket::shared& packet, size_t stapAStartIdx, size_t stapAEndIdx) |
include/rtp/RTPPacketizer.h
Outdated
|
|
||
| class RTPPacketizer | ||
| { | ||
| public: | ||
| static constexpr uint32_t StapANaluSize = 2; |
There was a problem hiding this comment.
this declaration doesn't belong here
include/rtp/RTPPacketizer.h
Outdated
|
|
||
| class RTPPacketizer | ||
| { | ||
| public: | ||
| static constexpr uint32_t StapANaluSize = 2; | ||
| static constexpr uint8_t StapAHeader = 0b0'11'11000; // STAP-A header nalu type = 24 |
There was a problem hiding this comment.
Not sure I am following, what's the intended usage of this class? I thought that this class is responsible for packetize stapA regardless the codec type? You want it to be moved to h26xPacketizer?
There was a problem hiding this comment.
rtpacketizer is codec agnostic, so it doesn't make sense to have any h264 codec specific info here
include/rtp/RTPPacketizer.h
Outdated
| @@ -25,6 +40,9 @@ class RTPPacketizer | |||
| protected: | |||
| MediaFrame::Type mediaType; | |||
| DWORD codec; | |||
| private: | |||
| static size_t CalculateAggregationEndIndex(const MediaFrame::RtpPacketizationInfo& info, size_t startIdx, uint32_t maxSize); | |||
There was a problem hiding this comment.
didn't we agree to move this to somewhere inside h264 code?
There was a problem hiding this comment.
I replied to your previous comment about moving the functions to h264. I think that these two functions are not codec specific operations, so it makes more sense to not move them to a codec specific file?
src/MediaFrameListenerBridge.cpp
Outdated
| { | ||
| uint64_t time = frame->GetTime(); | ||
| uint64_t ms = scheduled.count(); | ||
| uint32_t frameSize = 0; |
There was a problem hiding this comment.
| uint32_t frameSize = 0; | |
| uint32_t frameSize = frame->GetLength();; |
src/MediaFrameListenerBridge.cpp
Outdated
| @@ -546,6 +375,58 @@ void MediaFrameListenerBridge::UpdateFrameInfoAndStats(const std::shared_ptr<Med | |||
| bitrate = acumulator.GetInstant()*8; | |||
| } | |||
|
|
|||
| void MediaFrameListenerBridge::AddRTPPackets(const std::shared_ptr<MediaFrame>& frame, const std::vector<RTPPacket::shared>& rtpPackets, std::chrono::milliseconds scheduled) | |||
There was a problem hiding this comment.
| void MediaFrameListenerBridge::AddRTPPackets(const std::shared_ptr<MediaFrame>& frame, const std::vector<RTPPacket::shared>& rtpPackets, std::chrono::milliseconds scheduled) | |
| void MediaFrameListenerBridge::EnqueueRTPPackets(const std::shared_ptr<MediaFrame>& frame, const std::vector<RTPPacket::shared>& rtpPackets, std::chrono::milliseconds scheduled) |
src/MediaFrameListenerBridge.cpp
Outdated
| auto audioFrame = std::static_pointer_cast<AudioFrame>(frame); | ||
| //Set correct clock rate for audio codec | ||
| auto rate = AudioCodec::GetClockRate(audioFrame->GetCodec()); | ||
| pendingDuration = frame->GetDuration() * frame->GetClockRate() / rate; |
There was a problem hiding this comment.
imo it is better to calculate the pending duration outside and pass it as parameter here
src/rtp/RTPPacketizer.cpp
Outdated
|
|
||
| for (size_t i=0; i<rtpPackets.size(); i++) | ||
| { | ||
| auto packet = rtpPackets[i]; |
There was a problem hiding this comment.
Do not copy, it will cause a new shared pointer to be created, degrading performance
| auto packet = rtpPackets[i]; | |
| auto& packet = rtpPackets[i]; |
src/rtp/RTPPacketizer.cpp
Outdated
| void RTPPacketizer::SetPacketsProp(const std::shared_ptr<MediaFrame>& frame, const std::vector<RTPPacket::shared>& rtpPackets, uint64_t lastTimestamp, uint64_t extSeqNum, uint32_t targetBitrateHint) | ||
| { | ||
| uint32_t rate = 0; | ||
| RTPPacketizer::VideoFrameInfo videoInfo {0}; |
There was a problem hiding this comment.
I don't see the value of having a struct at packetizer level when we are using only internally in this function
No description provided.