[Streaming] Add some validity checking/cleanupupstream-master

This patch does some updates to how we do validity checking for offer
messages, based on a TODO.

Change-Id: I09f7c530fb4b1b9cba8759e081577a92420a5364
Reviewed-on: https://chromium-review.googlesource.com/c/openscreen/+/2896253
Commit-Queue: Jordan Bayles <jophba@chromium.org>
Reviewed-by: Ryan Keane <rwkeane@google.com>

4 files changed, 61 insertions, 65 deletions

diff --git a/cast/streaming/offer_messages.cc b/cast/streaming/offer_messages.cc
index 8ade19d8..f21decb1 100644
--- a/cast/streaming/offer_messages.cc
+++ b/cast/streaming/offer_messages.cc
@@ -268,12 +268,8 @@ EnumNameTable<CastMode, 2> kCastModeNames{
 
 }  // namespace
 
-ErrorOr<Json::Value> Stream::ToJson() const {
-  if (channels < 1 || index < 0 || target_delay.count() <= 0 ||
-      target_delay.count() > std::numeric_limits<int>::max() ||
-      rtp_timebase < 1) {
-    return json::CreateParameterError("Stream");
-  }
+Json::Value Stream::ToJson() const {
+  OSP_DCHECK(IsValid());
 
   Json::Value root;
   root["index"] = index;
@@ -295,47 +291,47 @@ ErrorOr<Json::Value> Stream::ToJson() const {
   return root;
 }
 
-ErrorOr<Json::Value> AudioStream::ToJson() const {
-  // A bit rate of 0 is valid for some codec types, so we don't enforce here.
-  if (bit_rate < 0) {
-    return json::CreateParameterError("AudioStream");
-  }
+bool Stream::IsValid() const {
+  return channels >= 1 && index >= 0 && target_delay.count() > 0 &&
+         target_delay.count() <= std::numeric_limits<int>::max() &&
+         rtp_timebase >= 1;
+}
 
-  auto error_or_stream = stream.ToJson();
-  if (error_or_stream.is_error()) {
-    return error_or_stream;
-  }
+Json::Value AudioStream::ToJson() const {
+  OSP_DCHECK(IsValid());
 
-  error_or_stream.value()[kCodecName] = CodecToString(codec);
-  error_or_stream.value()["bitRate"] = bit_rate;
-  return error_or_stream;
+  Json::Value out = stream.ToJson();
+  out[kCodecName] = CodecToString(codec);
+  out["bitRate"] = bit_rate;
+  return out;
 }
 
-ErrorOr<Json::Value> VideoStream::ToJson() const {
-  if (max_bit_rate <= 0 || !max_frame_rate.is_positive()) {
-    return json::CreateParameterError("VideoStream");
-  }
+bool AudioStream::IsValid() const {
+  return bit_rate >= 0 && stream.IsValid();
+}
 
-  auto error_or_stream = stream.ToJson();
-  if (error_or_stream.is_error()) {
-    return error_or_stream;
-  }
+Json::Value VideoStream::ToJson() const {
+  OSP_DCHECK(IsValid());
 
-  auto& stream = error_or_stream.value();
-  stream["codecName"] = CodecToString(codec);
-  stream["maxFrameRate"] = max_frame_rate.ToString();
-  stream["maxBitRate"] = max_bit_rate;
-  stream["protection"] = protection;
-  stream["profile"] = profile;
-  stream["level"] = level;
-  stream["errorRecoveryMode"] = error_recovery_mode;
+  Json::Value out = stream.ToJson();
+  out["codecName"] = CodecToString(codec);
+  out["maxFrameRate"] = max_frame_rate.ToString();
+  out["maxBitRate"] = max_bit_rate;
+  out["protection"] = protection;
+  out["profile"] = profile;
+  out["level"] = level;
+  out["errorRecoveryMode"] = error_recovery_mode;
 
   Json::Value rs;
   for (auto resolution : resolutions) {
     rs.append(resolution.ToJson());
   }
-  stream["resolutions"] = std::move(rs);
-  return error_or_stream;
+  out["resolutions"] = std::move(rs);
+  return out;
+}
+
+bool VideoStream::IsValid() const {
+  return max_bit_rate > 0 && max_frame_rate.is_positive();
 }
 
 // static
@@ -390,30 +386,28 @@ ErrorOr<Offer> Offer::Parse(const Json::Value& root) {
                std::move(video_streams)};
 }
 
-ErrorOr<Json::Value> Offer::ToJson() const {
+Json::Value Offer::ToJson() const {
+  OSP_DCHECK(IsValid());
   Json::Value root;
-
   root["castMode"] = GetEnumName(kCastModeNames, cast_mode).value();
   Json::Value streams;
-  for (auto& as : audio_streams) {
-    auto eoj = as.ToJson();
-    if (eoj.is_error()) {
-      return eoj;
-    }
-    streams.append(eoj.value());
+  for (auto& stream : audio_streams) {
+    streams.append(stream.ToJson());
   }
 
-  for (auto& vs : video_streams) {
-    auto eoj = vs.ToJson();
-    if (eoj.is_error()) {
-      return eoj;
-    }
-    streams.append(eoj.value());
+  for (auto& stream : video_streams) {
+    streams.append(stream.ToJson());
   }
 
   root[kSupportedStreams] = std::move(streams);
   return root;
 }
 
+bool Offer::IsValid() const {
+  return std::all_of(audio_streams.begin(), audio_streams.end(),
+                     [](const AudioStream& a) { return a.IsValid(); }) &&
+         std::all_of(video_streams.begin(), video_streams.end(),
+                     [](const VideoStream& v) { return v.IsValid(); });
+}
 }  // namespace cast
 }  // namespace openscreen
diff --git a/cast/streaming/offer_messages.h b/cast/streaming/offer_messages.h
index a64280ff..efb3a1f6 100644
--- a/cast/streaming/offer_messages.h
+++ b/cast/streaming/offer_messages.h
@@ -46,7 +46,8 @@ constexpr int kDefaultNumAudioChannels = 2;
 struct Stream {
   enum class Type : uint8_t { kAudioSource, kVideoSource };
 
-  ErrorOr<Json::Value> ToJson() const;
+  Json::Value ToJson() const;
+  bool IsValid() const;
 
   int index = 0;
   Type type = {};
@@ -67,7 +68,8 @@ struct Stream {
 };
 
 struct AudioStream {
-  ErrorOr<Json::Value> ToJson() const;
+  Json::Value ToJson() const;
+  bool IsValid() const;
 
   Stream stream = {};
   AudioCodec codec;
@@ -76,7 +78,8 @@ struct AudioStream {
 
 
 struct VideoStream {
-  ErrorOr<Json::Value> ToJson() const;
+  Json::Value ToJson() const;
+  bool IsValid() const;
 
   Stream stream = {};
   VideoCodec codec;
@@ -93,7 +96,8 @@ enum class CastMode : uint8_t { kMirroring, kRemoting };
 
 struct Offer {
   static ErrorOr<Offer> Parse(const Json::Value& root);
-  ErrorOr<Json::Value> ToJson() const;
+  Json::Value ToJson() const;
+  bool IsValid() const;
 
   CastMode cast_mode = CastMode::kMirroring;
   std::vector<AudioStream> audio_streams = {};
diff --git a/cast/streaming/offer_messages_unittest.cc b/cast/streaming/offer_messages_unittest.cc
index 0614d522..e1e70550 100644
--- a/cast/streaming/offer_messages_unittest.cc
+++ b/cast/streaming/offer_messages_unittest.cc
@@ -454,18 +454,16 @@ TEST(OfferTest, ParseAndToJsonResultsInSameOffer) {
   ErrorOr<Json::Value> root = json::Parse(kValidOffer);
   ASSERT_TRUE(root.is_value());
   ErrorOr<Offer> offer = Offer::Parse(std::move(root.value()));
-
   ExpectEqualsValidOffer(offer.value());
 
-  auto eoj = offer.value().ToJson();
-  EXPECT_TRUE(eoj.is_value()) << eoj.error();
-  ErrorOr<Offer> reparsed_offer = Offer::Parse(std::move(eoj.value()));
+  ErrorOr<Offer> reparsed_offer =
+      Offer::Parse(std::move(offer.value().ToJson()));
   ExpectEqualsValidOffer(reparsed_offer.value());
 }
 
 // We don't want to enforce that a given offer must have both audio and
 // video, so we don't assert on either.
-TEST(OfferTest, ToJsonSucceedsWithMissingStreams) {
+TEST(OfferTest, IsValidWithMissingStreams) {
   ErrorOr<Json::Value> root = json::Parse(kValidOffer);
   ASSERT_TRUE(root.is_value());
   ErrorOr<Offer> offer = Offer::Parse(std::move(root.value()));
@@ -474,14 +472,14 @@ TEST(OfferTest, ToJsonSucceedsWithMissingStreams) {
 
   Offer missing_audio_streams = valid_offer;
   missing_audio_streams.audio_streams.clear();
-  EXPECT_TRUE(missing_audio_streams.ToJson().is_value());
+  EXPECT_TRUE(missing_audio_streams.IsValid());
 
   Offer missing_video_streams = valid_offer;
   missing_video_streams.audio_streams.clear();
-  EXPECT_TRUE(missing_video_streams.ToJson().is_value());
+  EXPECT_TRUE(missing_video_streams.IsValid());
 }
 
-TEST(OfferTest, ToJsonFailsWithInvalidStreams) {
+TEST(OfferTest, InvalidIfInvalidStreams) {
   ErrorOr<Json::Value> root = json::Parse(kValidOffer);
   ASSERT_TRUE(root.is_value());
   ErrorOr<Offer> offer = Offer::Parse(std::move(root.value()));
@@ -490,11 +488,11 @@ TEST(OfferTest, ToJsonFailsWithInvalidStreams) {
 
   Offer video_stream_invalid = valid_offer;
   video_stream_invalid.video_streams[0].max_frame_rate = SimpleFraction{1, 0};
-  EXPECT_TRUE(video_stream_invalid.ToJson().is_error());
+  EXPECT_FALSE(video_stream_invalid.IsValid());
 
   Offer audio_stream_invalid = valid_offer;
   video_stream_invalid.audio_streams[0].bit_rate = 0;
-  EXPECT_TRUE(video_stream_invalid.ToJson().is_error());
+  EXPECT_FALSE(video_stream_invalid.IsValid());
 }
 
 TEST(OfferTest, FailsIfUnencrypted) {
diff --git a/cast/streaming/sender_message.cc b/cast/streaming/sender_message.cc
index e03cba18..d776c67e 100644
--- a/cast/streaming/sender_message.cc
+++ b/cast/streaming/sender_message.cc
@@ -93,7 +93,7 @@ ErrorOr<Json::Value> SenderMessage::ToJson() const {
 
   switch (type) {
     case SenderMessage::Type::kOffer:
-      root[kOfferMessageBody] = absl::get<Offer>(body).ToJson().value();
+      root[kOfferMessageBody] = absl::get<Offer>(body).ToJson();
       break;
 
     case SenderMessage::Type::kRpc: