Update Receiver ownership
Currently, the embedder is responsible for owning Receivers, but they
are not guaranteed to remain valid and are deleted upon renegotiation.
To avoid confusing ownership and lifetimes, this patch changes
ConfiguredReceivers to use dumb pointers, and have the lifetime of both
the audio and video receivers managed directly by the ReceiverSession.
A new OnReceiversDestroyed event is created, and the unit tests are
updated to ensure it is called appropriately.
Change-Id: I0c4fef00ecbafcc4e9b9abf5af5b507b38fece4b
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/1961046
Reviewed-by: Ryan Keane <rwkeane@google.com>
Commit-Queue: Jordan Bayles <jophba@chromium.org>
diff --git a/cast/streaming/receiver_session.cc b/cast/streaming/receiver_session.cc
index 0f6339e..19abfa4 100644
--- a/cast/streaming/receiver_session.cc
+++ b/cast/streaming/receiver_session.cc
@@ -78,13 +78,13 @@
} // namespace
ReceiverSession::ConfiguredReceivers::ConfiguredReceivers(
- std::unique_ptr<Receiver> audio_receiver,
+ Receiver* audio_receiver,
absl::optional<SessionConfig> audio_receiver_config,
- std::unique_ptr<Receiver> video_receiver,
+ Receiver* video_receiver,
absl::optional<SessionConfig> video_receiver_config)
- : audio_receiver_(std::move(audio_receiver)),
+ : audio_receiver_(audio_receiver),
audio_receiver_config_(std::move(audio_receiver_config)),
- video_receiver_(std::move(video_receiver)),
+ video_receiver_(video_receiver),
video_receiver_config_(std::move(video_receiver_config)) {}
ConfiguredReceivers::ConfiguredReceivers(ConfiguredReceivers&&) noexcept =
@@ -127,6 +127,7 @@
}
ReceiverSession::~ReceiverSession() {
+ ResetReceivers();
message_port_->SetClient(nullptr);
}
@@ -229,25 +230,33 @@
return openscreen::Error::Code::kParameterInvalid;
}
+ ResetReceivers();
+
absl::optional<SessionConfig> audio_config;
- std::unique_ptr<Receiver> audio_receiver;
if (audio) {
auto audio_pair = ConstructReceiver(audio->stream);
audio_config = std::move(audio_pair.first);
- audio_receiver = std::move(audio_pair.second);
+ current_audio_receiver_ = std::move(audio_pair.second);
}
absl::optional<SessionConfig> video_config;
- std::unique_ptr<Receiver> video_receiver;
if (video) {
auto video_pair = ConstructReceiver(video->stream);
video_config = std::move(video_pair.first);
- video_receiver = std::move(video_pair.second);
+ current_video_receiver_ = std::move(video_pair.second);
}
- return ConfiguredReceivers{std::move(audio_receiver), std::move(audio_config),
- std::move(video_receiver),
- std::move(video_config)};
+ return ConfiguredReceivers{
+ current_audio_receiver_.get(), std::move(audio_config),
+ current_video_receiver_.get(), std::move(video_config)};
+}
+
+void ReceiverSession::ResetReceivers() {
+ if (current_video_receiver_ || current_audio_receiver_) {
+ client_->OnReceiversDestroyed(this);
+ current_audio_receiver_.reset();
+ current_video_receiver_.reset();
+ }
}
Answer ReceiverSession::ConstructAnswer(
diff --git a/cast/streaming/receiver_session.h b/cast/streaming/receiver_session.h
index e148ede..4ded889 100644
--- a/cast/streaming/receiver_session.h
+++ b/cast/streaming/receiver_session.h
@@ -38,10 +38,15 @@
// In practice, we may have 0, 1, or 2 receivers configured, depending
// on if the device supports audio and video, and if we were able to
// successfully negotiate a receiver configuration.
+
+ // NOTES ON LIFETIMES: The audio and video receiver pointers are expected
+ // to be valid until the OnReceiversDestroyed event is fired, at which
+ // point they become invalid and need to replaced by the results of
+ // the ensuing OnNegotiated call.
ConfiguredReceivers(
- std::unique_ptr<Receiver> audio_receiver,
+ Receiver* audio_receiver,
const absl::optional<SessionConfig> audio_receiver_config,
- std::unique_ptr<Receiver> video_receiver,
+ Receiver* video_receiver,
const absl::optional<SessionConfig> video_receiver_config);
ConfiguredReceivers(const ConfiguredReceivers&) = delete;
ConfiguredReceivers(ConfiguredReceivers&&) noexcept;
@@ -51,19 +56,19 @@
// If the receiver is audio- or video-only, either of the receivers
// may be nullptr. However, in the majority of cases they will be populated.
- Receiver* audio_receiver() const { return audio_receiver_.get(); }
+ Receiver* audio_receiver() const { return audio_receiver_; }
const absl::optional<SessionConfig>& audio_session_config() const {
return audio_receiver_config_;
}
- Receiver* video_receiver() const { return video_receiver_.get(); }
+ Receiver* video_receiver() const { return video_receiver_; }
const absl::optional<SessionConfig>& video_session_config() const {
return video_receiver_config_;
}
private:
- std::unique_ptr<Receiver> audio_receiver_;
+ Receiver* audio_receiver_;
absl::optional<SessionConfig> audio_receiver_config_;
- std::unique_ptr<Receiver> video_receiver_;
+ Receiver* video_receiver_;
absl::optional<SessionConfig> video_receiver_config_;
};
@@ -71,8 +76,13 @@
// When a connection is established, the OnNegotiated callback is called.
class Client {
public:
+ // This method is called when a new set of receivers has been negotiated.
virtual void OnNegotiated(ReceiverSession* session,
ConfiguredReceivers receivers) = 0;
+
+ // This method is called immediately preceding the invalidation of
+ // this session's receivers.
+ virtual void OnReceiversDestroyed(ReceiverSession* session) = 0;
virtual void OnError(ReceiverSession* session, openscreen::Error error) = 0;
};
@@ -151,6 +161,9 @@
void SendMessage(Message* message);
+ // Handles resetting receivers and notifying the client.
+ void ResetReceivers();
+
Client* const client_;
const std::unique_ptr<Environment> environment_;
const std::unique_ptr<MessagePort> message_port_;
@@ -159,6 +172,9 @@
CastMode cast_mode_;
bool supports_wifi_status_reporting_ = false;
ReceiverPacketRouter packet_router_;
+
+ std::unique_ptr<Receiver> current_audio_receiver_;
+ std::unique_ptr<Receiver> current_video_receiver_;
};
} // namespace streaming
diff --git a/cast/streaming/receiver_session_unittest.cc b/cast/streaming/receiver_session_unittest.cc
index 6c9434c..4716a6a 100644
--- a/cast/streaming/receiver_session_unittest.cc
+++ b/cast/streaming/receiver_session_unittest.cc
@@ -204,6 +204,7 @@
OnNegotiated,
(ReceiverSession*, ReceiverSession::ConfiguredReceivers),
(override));
+ MOCK_METHOD(void, OnReceiversDestroyed, (ReceiverSession*), (override));
MOCK_METHOD(void,
OnError,
(ReceiverSession*, openscreen::Error error),
@@ -279,6 +280,7 @@
EXPECT_EQ(cr.video_session_config().value().channels, 1);
EXPECT_EQ(cr.video_session_config().value().rtp_timebase, 90000);
});
+ EXPECT_CALL(client, OnReceiversDestroyed(&session)).Times(1);
raw_port->ReceiveMessage(kValidOfferMessage);
@@ -340,6 +342,7 @@
EXPECT_EQ(cr.video_session_config().value().channels, 1);
EXPECT_EQ(cr.video_session_config().value().rtp_timebase, 90000);
});
+ EXPECT_CALL(client, OnReceiversDestroyed(&session)).Times(1);
raw_port->ReceiveMessage(kValidOfferMessage);
}
@@ -366,6 +369,7 @@
std::move(display)});
EXPECT_CALL(client, OnNegotiated(&session, _)).Times(1);
+ EXPECT_CALL(client, OnReceiversDestroyed(&session)).Times(1);
raw_port->ReceiveMessage(kValidOfferMessage);
const auto& messages = raw_port->posted_messages();
@@ -422,6 +426,7 @@
ReceiverSession::Preferences{});
EXPECT_CALL(client, OnNegotiated(&session, _)).Times(1);
+ EXPECT_CALL(client, OnReceiversDestroyed(&session)).Times(1);
raw_port->ReceiveMessage(kNoAudioOfferMessage);
const auto& messages = raw_port->posted_messages();
@@ -447,6 +452,7 @@
ReceiverSession::Preferences{});
EXPECT_CALL(client, OnNegotiated(&session, _)).Times(1);
+ EXPECT_CALL(client, OnReceiversDestroyed(&session)).Times(1);
raw_port->ReceiveMessage(kNoVideoOfferMessage);
const auto& messages = raw_port->posted_messages();
@@ -473,6 +479,7 @@
// We shouldn't call OnNegotiated if we failed to negotiate any streams.
EXPECT_CALL(client, OnNegotiated(&session, _)).Times(0);
+ EXPECT_CALL(client, OnReceiversDestroyed(&session)).Times(0);
raw_port->ReceiveMessage(kNoAudioOrVideoOfferMessage);
const auto& messages = raw_port->posted_messages();
@@ -493,6 +500,7 @@
// Note that unlike when we simply don't select any streams, when the offer
// is actually completely invalid we call OnError.
EXPECT_CALL(client, OnNegotiated(&session, _)).Times(0);
+ EXPECT_CALL(client, OnReceiversDestroyed(&session)).Times(0);
EXPECT_CALL(client,
OnError(&session, openscreen::Error(
openscreen::Error::Code::kJsonParseError)))
@@ -501,5 +509,18 @@
raw_port->ReceiveMessage(kInvalidJsonOfferMessage);
}
+TEST_F(ReceiverSessionTest, NotifiesReceiverDestruction) {
+ auto message_port = std::make_unique<SimpleMessagePort>();
+ SimpleMessagePort* raw_port = message_port.get();
+ StrictMock<FakeClient> client;
+ ReceiverSession session(&client, std::move(env_), std::move(message_port),
+ ReceiverSession::Preferences{});
+
+ EXPECT_CALL(client, OnNegotiated(&session, _)).Times(2);
+ EXPECT_CALL(client, OnReceiversDestroyed(&session)).Times(2);
+
+ raw_port->ReceiveMessage(kNoAudioOfferMessage);
+ raw_port->ReceiveMessage(kValidOfferMessage);
+}
} // namespace streaming
} // namespace cast
