Simplify MoqtTraceRecorder by recording new objects directly from existing subscribtion code.
We already have a mechanism to track subscriptions, no need to have a second one. Also, this gets rid of issues where having one listener remove another causes iterator invalidation.
PiperOrigin-RevId: 824615277
diff --git a/quiche/quic/moqt/moqt_integration_test.cc b/quiche/quic/moqt/moqt_integration_test.cc
index 1935580..fcf84df 100644
--- a/quiche/quic/moqt/moqt_integration_test.cc
+++ b/quiche/quic/moqt/moqt_integration_test.cc
@@ -68,6 +68,9 @@
client_->RecordTrace();
client_->session()->trace_recorder().SetParentRecorder(
client_->trace_visitor());
+ server_->RecordTrace();
+ server_->session()->trace_recorder().SetParentRecorder(
+ server_->trace_visitor());
}
void WireUpEndpoints() { test_harness_.WireUpEndpoints(); }
diff --git a/quiche/quic/moqt/moqt_session.cc b/quiche/quic/moqt/moqt_session.cc
index 9b48d78..01b64be 100644
--- a/quiche/quic/moqt/moqt_session.cc
+++ b/quiche/quic/moqt/moqt_session.cc
@@ -1981,13 +1981,11 @@
QUIC_DLOG(INFO) << ENDPOINT << "Created subscription for "
<< subscribe.full_track_name;
session_->subscribed_track_names_.insert(subscribe.full_track_name);
- session_->trace_recorder_.StartRecordingTrack(track_alias, track_publisher);
}
MoqtSession::PublishedSubscription::~PublishedSubscription() {
session_->subscribed_track_names_.erase(track_publisher_->GetTrackName());
track_publisher_->RemoveObjectListener(this);
- session_->trace_recorder_.StopRecordingTrack(track_alias_);
}
SendStreamMap& MoqtSession::PublishedSubscription::stream_map() {
@@ -2096,6 +2094,8 @@
if (!InWindow(location)) {
return;
}
+ session_->trace_recorder_.RecordNewObjectAvaliable(
+ track_alias_, *track_publisher_, location, subgroup, publisher_priority);
DataStreamIndex index(location.group, subgroup);
if (reset_subgroups_.contains(index)) {
// This subgroup has already been reset, ignore.
diff --git a/quiche/quic/moqt/moqt_trace_recorder.cc b/quiche/quic/moqt/moqt_trace_recorder.cc
index 7ff7ff8..0a503ef 100644
--- a/quiche/quic/moqt/moqt_trace_recorder.cc
+++ b/quiche/quic/moqt/moqt_trace_recorder.cc
@@ -4,13 +4,9 @@
#include "quiche/quic/moqt/moqt_trace_recorder.h"
-#include <cstddef>
#include <cstdint>
-#include <memory>
#include <optional>
-#include <utility>
-#include "absl/hash/hash.h"
#include "quiche/quic/moqt/moqt_messages.h"
#include "quiche/quic/moqt/moqt_object.h"
#include "quiche/quic/moqt/moqt_priority.h"
@@ -76,65 +72,33 @@
return event;
}
-MoqtTraceRecorder::Track::Track(MoqtTraceRecorder* recorder,
- std::shared_ptr<MoqtTrackPublisher> publisher,
- uint64_t track_alias)
- : recorder_(recorder),
- publisher_(std::move(publisher)),
- track_alias_(track_alias) {
- publisher_->AddObjectListener(this);
-}
-
-MoqtTraceRecorder::Track::~Track() { publisher_->RemoveObjectListener(this); }
-
-void MoqtTraceRecorder::Track::OnNewObjectAvailable(
- Location sequence, uint64_t subgroup, MoqtPriority publisher_priority) {
- if (recorder_->parent_ == nullptr) {
+void MoqtTraceRecorder::RecordNewObjectAvaliable(
+ uint64_t track_alias, const MoqtTrackPublisher& publisher,
+ Location location, uint64_t subgroup, MoqtPriority publisher_priority) {
+ if (parent_ == nullptr) {
return;
}
- quic_trace::Event* event = recorder_->AddEvent();
+ quic_trace::Event* event = AddEvent();
event->set_event_type(EventType::MOQT_OBJECT_ENQUEUED);
- recorder_->parent_->PopulateTransportState(event->mutable_transport_state());
+ parent_->PopulateTransportState(event->mutable_transport_state());
quic_trace::MoqtObject* object = event->mutable_moqt_object();
- object->set_track_alias(track_alias_);
- object->set_group_id(sequence.group);
- object->set_object_id(sequence.object);
+ object->set_track_alias(track_alias);
+ object->set_group_id(location.group);
+ object->set_object_id(location.object);
object->set_subgroup_id(subgroup);
object->set_publisher_priority(publisher_priority);
std::optional<PublishedObject> object_copy =
- publisher_->GetCachedObject(sequence.group, subgroup, sequence.object);
- if (object_copy.has_value() && object_copy->metadata.location == sequence) {
+ publisher.GetCachedObject(location.group, subgroup, location.object);
+ if (object_copy.has_value() && object_copy->metadata.location == location) {
object->set_payload_size(object_copy->payload.length());
} else {
- QUICHE_DLOG(WARNING) << "Track " << track_alias_ << " has marked "
- << sequence
+ QUICHE_DLOG(WARNING) << "Track " << track_alias << " has marked "
+ << location
<< " as enqueued, but GetCachedObject was not able to "
"return the said object";
}
}
-size_t MoqtTraceRecorder::TrackAliasHash::operator()(
- uint64_t track_alias) const {
- return absl::HashOf(track_alias);
-}
-
-void MoqtTraceRecorder::StartRecordingTrack(
- uint64_t track_alias, std::shared_ptr<MoqtTrackPublisher> publisher) {
- if (parent_ == nullptr) {
- return;
- }
- auto [it, added] = tracks_.emplace(this, std::move(publisher), track_alias);
- QUICHE_DCHECK(added);
-}
-
-void MoqtTraceRecorder::StopRecordingTrack(uint64_t track_alias) {
- if (parent_ == nullptr) {
- return;
- }
- size_t erased = tracks_.erase(track_alias);
- QUICHE_DCHECK_EQ(erased, 1);
-}
-
} // namespace moqt
diff --git a/quiche/quic/moqt/moqt_trace_recorder.h b/quiche/quic/moqt/moqt_trace_recorder.h
index 57c7dbd..a40693a 100644
--- a/quiche/quic/moqt/moqt_trace_recorder.h
+++ b/quiche/quic/moqt/moqt_trace_recorder.h
@@ -5,12 +5,10 @@
#ifndef QUICHE_QUIC_MOQT_MOQT_TRACE_RECORDER_H_
#define QUICHE_QUIC_MOQT_MOQT_TRACE_RECORDER_H_
-#include <cstddef>
#include <cstdint>
#include <memory>
#include "absl/base/nullability.h"
-#include "absl/container/node_hash_set.h"
#include "quiche/quic/core/quic_trace_visitor.h"
#include "quiche/quic/moqt/moqt_messages.h"
#include "quiche/quic/moqt/moqt_priority.h"
@@ -51,70 +49,17 @@
void RecordProbeStreamCreated(webtransport::StreamId stream_id,
uint64_t probe_id);
- // Records the track-related events by registering the recorder as a listener
- // of `publisher`.
- void StartRecordingTrack(uint64_t track_alias,
- std::shared_ptr<MoqtTrackPublisher> publisher);
- // Removes a previously added track.
- void StopRecordingTrack(uint64_t track_alias);
+ // Records the fact that the application has enqueued a new object.
+ void RecordNewObjectAvaliable(uint64_t track_alias,
+ const MoqtTrackPublisher& publisher,
+ Location location, uint64_t subgroup,
+ MoqtPriority publisher_priority);
private:
- // Visitor that records events for a specific published track.
- class Track : public MoqtObjectListener {
- public:
- Track(MoqtTraceRecorder* recorder,
- std::shared_ptr<MoqtTrackPublisher> publisher, uint64_t track_alias);
- ~Track();
-
- Track(const Track&) = delete;
- Track(Track&&) = delete;
- Track& operator=(const Track&) = delete;
- Track& operator=(Track&&) = delete;
-
- // MoqtObjectListener implementation.
- void OnSubscribeAccepted() override {}
- void OnSubscribeRejected(MoqtSubscribeErrorReason reason) override {}
- void OnNewObjectAvailable(Location sequence, uint64_t subgroup,
- MoqtPriority publisher_priority) override;
- void OnNewFinAvailable(Location final_object_in_subgroup,
- uint64_t subgroup_id) override {}
- void OnSubgroupAbandoned(
- uint64_t group, uint64_t subgroup,
- webtransport::StreamErrorCode error_code) override {}
- void OnGroupAbandoned(uint64_t group_id) override {}
- void OnTrackPublisherGone() override {}
-
- uint64_t track_alias() const { return track_alias_; }
-
- private:
- MoqtTraceRecorder* const recorder_;
- std::shared_ptr<MoqtTrackPublisher> const publisher_;
- const uint64_t track_alias_;
- };
-
- // Index Track objects by track_alias.
- struct TrackAliasEq {
- using is_transparent = void;
- bool operator()(const Track& a, const Track& b) const {
- return a.track_alias() == b.track_alias();
- }
- bool operator()(const Track& a, uint64_t b) const {
- return a.track_alias() == b;
- }
- };
- struct TrackAliasHash {
- using is_transparent = void;
- size_t operator()(uint64_t track_alias) const;
- size_t operator()(const Track& track) const {
- return (*this)(track.track_alias());
- }
- };
-
// Adds a new event to the trace, and populates the timestamp.
quic_trace::Event* AddEvent();
quic::QuicTraceVisitor* absl_nullable parent_;
- absl::node_hash_set<Track, TrackAliasHash, TrackAliasEq> tracks_;
};
} // namespace moqt
