diff --git a/logging/rtc_event_log/rtc_event_log_parser.cc b/logging/rtc_event_log/rtc_event_log_parser.cc index d10e4f987a..08fb9408c1 100644 --- a/logging/rtc_event_log/rtc_event_log_parser.cc +++ b/logging/rtc_event_log/rtc_event_log_parser.cc @@ -961,23 +961,21 @@ ParsedRtcEventLog::LoggedRtpStreamOutgoing::~LoggedRtpStreamOutgoing() = ParsedRtcEventLog::LoggedRtpStreamView::LoggedRtpStreamView( uint32_t ssrc, - const LoggedRtpPacketIncoming* ptr, - size_t num_elements) - : ssrc(ssrc), - packet_view(PacketView::Create( - ptr, - num_elements, - offsetof(LoggedRtpPacketIncoming, rtp))) {} + const std::vector& packets) + : ssrc(ssrc), packet_view() { + for (const LoggedRtpPacketIncoming& packet : packets) { + packet_view.push_back(&(packet.rtp)); + } +} ParsedRtcEventLog::LoggedRtpStreamView::LoggedRtpStreamView( uint32_t ssrc, - const LoggedRtpPacketOutgoing* ptr, - size_t num_elements) - : ssrc(ssrc), - packet_view(PacketView::Create( - ptr, - num_elements, - offsetof(LoggedRtpPacketOutgoing, rtp))) {} + const std::vector& packets) + : ssrc(ssrc), packet_view() { + for (const LoggedRtpPacketOutgoing& packet : packets) { + packet_view.push_back(&(packet.rtp)); + } +} ParsedRtcEventLog::LoggedRtpStreamView::LoggedRtpStreamView( const LoggedRtpStreamView&) = default; @@ -1161,13 +1159,11 @@ ParsedRtcEventLog::ParseStatus ParsedRtcEventLog::ParseStream( // Build PacketViews for easier iteration over RTP packets. for (const auto& stream : incoming_rtp_packets_by_ssrc_) { incoming_rtp_packet_views_by_ssrc_.emplace_back( - LoggedRtpStreamView(stream.ssrc, stream.incoming_packets.data(), - stream.incoming_packets.size())); + LoggedRtpStreamView(stream.ssrc, stream.incoming_packets)); } for (const auto& stream : outgoing_rtp_packets_by_ssrc_) { outgoing_rtp_packet_views_by_ssrc_.emplace_back( - LoggedRtpStreamView(stream.ssrc, stream.outgoing_packets.data(), - stream.outgoing_packets.size())); + LoggedRtpStreamView(stream.ssrc, stream.outgoing_packets)); } // Set up convenience wrappers around the most commonly used RTCP types. diff --git a/logging/rtc_event_log/rtc_event_log_parser.h b/logging/rtc_event_log/rtc_event_log_parser.h index 67e1a09fff..4898022fae 100644 --- a/logging/rtc_event_log/rtc_event_log_parser.h +++ b/logging/rtc_event_log/rtc_event_log_parser.h @@ -64,144 +64,108 @@ namespace webrtc { enum PacketDirection { kIncomingPacket = 0, kOutgoingPacket }; +// This class is used to process lists of LoggedRtpPacketIncoming +// and LoggedRtpPacketOutgoing without duplicating the code. +// TODO(terelius): Remove this class. Instead use e.g. a vector of pointers +// to LoggedRtpPacket or templatize the surrounding code. template -class PacketView; - -template -class PacketIterator { - friend class PacketView; - +class DereferencingVector { public: - // Standard iterator traits. - using difference_type = std::ptrdiff_t; - using value_type = T; - using pointer = T*; - using reference = T&; - using iterator_category = std::bidirectional_iterator_tag; + template + class DereferencingIterator { + public: + // Standard iterator traits. + using difference_type = std::ptrdiff_t; + using value_type = T; + using pointer = typename std::conditional_t; + using reference = typename std::conditional_t; + using iterator_category = std::bidirectional_iterator_tag; - // The default-contructed iterator is meaningless, but is required by the - // ForwardIterator concept. - PacketIterator() : ptr_(nullptr), element_size_(0) {} - PacketIterator(const PacketIterator& other) - : ptr_(other.ptr_), element_size_(other.element_size_) {} - PacketIterator(const PacketIterator&& other) - : ptr_(other.ptr_), element_size_(other.element_size_) {} - ~PacketIterator() = default; + using representation = + typename std::conditional_t; - PacketIterator& operator=(const PacketIterator& other) { - ptr_ = other.ptr_; - element_size_ = other.element_size_; - return *this; - } - PacketIterator& operator=(const PacketIterator&& other) { - ptr_ = other.ptr_; - element_size_ = other.element_size_; - return *this; - } + explicit DereferencingIterator(representation ptr) : ptr_(ptr) {} - bool operator==(const PacketIterator& other) const { - RTC_DCHECK_EQ(element_size_, other.element_size_); - return ptr_ == other.ptr_; - } - bool operator!=(const PacketIterator& other) const { - RTC_DCHECK_EQ(element_size_, other.element_size_); - return ptr_ != other.ptr_; - } + DereferencingIterator(const DereferencingIterator& other) + : ptr_(other.ptr_) {} + DereferencingIterator(const DereferencingIterator&& other) + : ptr_(other.ptr_) {} + ~DereferencingIterator() = default; - PacketIterator& operator++() { - ptr_ += element_size_; - return *this; - } - PacketIterator& operator--() { - ptr_ -= element_size_; - return *this; - } - PacketIterator operator++(int) { - PacketIterator iter_copy(ptr_, element_size_); - ptr_ += element_size_; - return iter_copy; - } - PacketIterator operator--(int) { - PacketIterator iter_copy(ptr_, element_size_); - ptr_ -= element_size_; - return iter_copy; - } + DereferencingIterator& operator=(const DereferencingIterator& other) { + ptr_ = other.ptr_; + return *this; + } + DereferencingIterator& operator=(const DereferencingIterator&& other) { + ptr_ = other.ptr_; + return *this; + } - T& operator*() { return *reinterpret_cast(ptr_); } - const T& operator*() const { return *reinterpret_cast(ptr_); } + bool operator==(const DereferencingIterator& other) const { + return ptr_ == other.ptr_; + } + bool operator!=(const DereferencingIterator& other) const { + return ptr_ != other.ptr_; + } - T* operator->() { return reinterpret_cast(ptr_); } - const T* operator->() const { return reinterpret_cast(ptr_); } + DereferencingIterator& operator++() { + ++ptr_; + return *this; + } + DereferencingIterator& operator--() { + --ptr_; + return *this; + } + DereferencingIterator operator++(int) { + DereferencingIterator iter_copy(ptr_); + ++ptr_; + return iter_copy; + } + DereferencingIterator operator--(int) { + DereferencingIterator iter_copy(ptr_); + --ptr_; + return iter_copy; + } - private: - PacketIterator(typename std::conditional::value, - const void*, - void*>::type p, - size_t s) - : ptr_(reinterpret_cast(p)), element_size_(s) {} + template + std::enable_if_t operator*() { + return **ptr_; + } - typename std::conditional::value, const char*, char*>::type - ptr_; - size_t element_size_; -}; + template + std::enable_if_t<_IsConst, reference> operator*() const { + return **ptr_; + } -// Suppose that we have a struct S where we are only interested in a specific -// member M. Given an array of S, PacketView can be used to treat the array -// as an array of M, without exposing the type S to surrounding code and without -// accessing the member through a virtual function. In this case, we want to -// have a common view for incoming and outgoing RtpPackets, hence the PacketView -// name. -// Note that constructing a PacketView bypasses the typesystem, so the caller -// has to take extra care when constructing these objects. The implementation -// also requires that the containing struct is standard-layout (e.g. POD). -// -// Usage example: -// struct A {...}; -// struct B { A a; ...}; -// struct C { A a; ...}; -// size_t len = 10; -// B* array1 = new B[len]; -// C* array2 = new C[len]; -// -// PacketView view1 = PacketView::Create(array1, len, offsetof(B, a)); -// PacketView view2 = PacketView::Create(array2, len, offsetof(C, a)); -// -// The following code works with either view1 or view2. -// void f(PacketView view) -// for (A& a : view) { -// DoSomething(a); -// } -template -class PacketView { - public: - template - static PacketView Create(U* ptr, size_t num_elements, size_t offset) { - static_assert(std::is_standard_layout::value, - "PacketView can only be created for standard layout types."); - static_assert(std::is_standard_layout::value, - "PacketView can only be created for standard layout types."); - return PacketView(ptr, num_elements, offset, sizeof(U)); - } + template + std::enable_if_t operator->() { + return *ptr_; + } + + template + std::enable_if_t<_IsConst, pointer> operator->() const { + return *ptr_; + } + + private: + representation ptr_; + }; using value_type = T; using reference = value_type&; using const_reference = const value_type&; - using iterator = PacketIterator; - using const_iterator = PacketIterator; + using iterator = DereferencingIterator; + using const_iterator = DereferencingIterator; using reverse_iterator = std::reverse_iterator; using const_reverse_iterator = std::reverse_iterator; - iterator begin() { return iterator(data_, element_size_); } - iterator end() { - auto end_ptr = data_ + num_elements_ * element_size_; - return iterator(end_ptr, element_size_); - } + iterator begin() { return iterator(elems_.data()); } + iterator end() { return iterator(elems_.data() + elems_.size()); } - const_iterator begin() const { return const_iterator(data_, element_size_); } + const_iterator begin() const { return const_iterator(elems_.data()); } const_iterator end() const { - auto end_ptr = data_ + num_elements_ * element_size_; - return const_iterator(end_ptr, element_size_); + return const_iterator(elems_.data() + elems_.size()); } reverse_iterator rbegin() { return reverse_iterator(end()); } @@ -214,35 +178,27 @@ class PacketView { return const_reverse_iterator(begin()); } - size_t size() const { return num_elements_; } + size_t size() const { return elems_.size(); } - bool empty() const { return num_elements_ == 0; } + bool empty() const { return elems_.empty(); } T& operator[](size_t i) { - auto elem_ptr = data_ + i * element_size_; - return *reinterpret_cast(elem_ptr); + RTC_DCHECK_LT(i, elems_.size()); + return *elems_[i]; } const T& operator[](size_t i) const { - auto elem_ptr = data_ + i * element_size_; - return *reinterpret_cast(elem_ptr); + RTC_DCHECK_LT(i, elems_.size()); + return *elems_[i]; + } + + void push_back(T* elem) { + RTC_DCHECK(elem != nullptr); + elems_.push_back(elem); } private: - PacketView(typename std::conditional::value, - const void*, - void*>::type data, - size_t num_elements, - size_t offset, - size_t element_size) - : data_(reinterpret_cast(data) + offset), - num_elements_(num_elements), - element_size_(element_size) {} - - typename std::conditional::value, const char*, char*>::type - data_; - size_t num_elements_; - size_t element_size_; + std::vector elems_; }; // Conversion functions for version 2 of the wire format. @@ -345,14 +301,12 @@ class ParsedRtcEventLog { struct LoggedRtpStreamView { LoggedRtpStreamView(uint32_t ssrc, - const LoggedRtpPacketIncoming* ptr, - size_t num_elements); + const std::vector& packets); LoggedRtpStreamView(uint32_t ssrc, - const LoggedRtpPacketOutgoing* ptr, - size_t num_elements); + const std::vector& packets); LoggedRtpStreamView(const LoggedRtpStreamView&); uint32_t ssrc; - PacketView packet_view; + DereferencingVector packet_view; }; class LogSegment { diff --git a/logging/rtc_event_log/rtc_event_log_unittest.cc b/logging/rtc_event_log/rtc_event_log_unittest.cc index 5535bec44f..323e4fe009 100644 --- a/logging/rtc_event_log/rtc_event_log_unittest.cc +++ b/logging/rtc_event_log/rtc_event_log_unittest.cc @@ -974,4 +974,64 @@ INSTANTIATE_TEST_SUITE_P( // TODO(terelius): Verify parser behavior if the timestamps are not // monotonically increasing in the log. +TEST(DereferencingVectorTest, NonConstVector) { + std::vector v{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; + DereferencingVector even; + EXPECT_TRUE(even.empty()); + EXPECT_EQ(even.size(), 0u); + EXPECT_EQ(even.begin(), even.end()); + for (size_t i = 0; i < v.size(); i += 2) { + even.push_back(&v[i]); + } + EXPECT_FALSE(even.empty()); + EXPECT_EQ(even.size(), 5u); + EXPECT_NE(even.begin(), even.end()); + + // Test direct access. + for (size_t i = 0; i < even.size(); i++) { + EXPECT_EQ(even[i], 2 * static_cast(i)); + } + + // Test iterator. + for (int val : even) { + EXPECT_EQ(val % 2, 0); + } + + // Test modification through iterator. + for (int& val : even) { + val = val * 2; + EXPECT_EQ(val % 2, 0); + } + + // Backing vector should have been modified. + std::vector expected{0, 1, 4, 3, 8, 5, 12, 7, 16, 9}; + for (size_t i = 0; i < v.size(); i++) { + EXPECT_EQ(v[i], expected[i]); + } +} + +TEST(DereferencingVectorTest, ConstVector) { + std::vector v{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; + DereferencingVector odd; + EXPECT_TRUE(odd.empty()); + EXPECT_EQ(odd.size(), 0u); + EXPECT_EQ(odd.begin(), odd.end()); + for (size_t i = 1; i < v.size(); i += 2) { + odd.push_back(&v[i]); + } + EXPECT_FALSE(odd.empty()); + EXPECT_EQ(odd.size(), 5u); + EXPECT_NE(odd.begin(), odd.end()); + + // Test direct access. + for (size_t i = 0; i < odd.size(); i++) { + EXPECT_EQ(odd[i], 2 * static_cast(i) + 1); + } + + // Test iterator. + for (int val : odd) { + EXPECT_EQ(val % 2, 1); + } +} + } // namespace webrtc