From e9e03a9160502b23749e8f64da517f83457e2ad3 Mon Sep 17 00:00:00 2001 From: Joachim Reiersen Date: Wed, 19 Jul 2023 12:47:29 -0700 Subject: [PATCH] Fix inaccurate contentType in RTCInbound/OutboundRtpStreamStats The existing equality check did not always work since content_type is sometimes overloaded with extra internal information such as simulcast layer index. Fix by using the videocontenttypehelpers::IsScreenshare helper method. Bug: webrtc:15381 Change-Id: I2fe84e7f036ea2c223e4fa6dd58af1c4c0bcfbdb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/312261 Commit-Queue: Harald Alvestrand Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#40448} --- api/video/video_content_type.h | 3 +++ pc/rtc_stats_collector.cc | 4 ++-- pc/rtc_stats_collector_unittest.cc | 24 ++++++++++++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/api/video/video_content_type.h b/api/video/video_content_type.h index 2d38a62366..2251b4a0f5 100644 --- a/api/video/video_content_type.h +++ b/api/video/video_content_type.h @@ -15,6 +15,9 @@ namespace webrtc { +// VideoContentType can take on more values than the two below, therefore care +// should be taken to avoid using the equality operator to check for screenshare +// usage. See https://bugs.chromium.org/p/webrtc/issues/detail?id=15381. enum class VideoContentType : uint8_t { UNSPECIFIED = 0, SCREENSHARE = 1, diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 334c76f6d4..07e8771f2f 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -667,7 +667,7 @@ CreateInboundRTPStreamStatsFromVideoReceiverInfo( } // TODO(bugs.webrtc.org/10529): When info's `content_info` is optional // support the "unspecified" value. - if (video_receiver_info.content_type == VideoContentType::SCREENSHARE) + if (videocontenttypehelpers::IsScreenshare(video_receiver_info.content_type)) inbound_video->content_type = "screenshare"; if (video_receiver_info.decoder_implementation_name.has_value()) { inbound_video->decoder_implementation = @@ -809,7 +809,7 @@ CreateOutboundRTPStreamStatsFromVideoSenderInfo( video_sender_info.quality_limitation_resolution_changes; // TODO(https://crbug.com/webrtc/10529): When info's `content_info` is // optional, support the "unspecified" value. - if (video_sender_info.content_type == VideoContentType::SCREENSHARE) + if (videocontenttypehelpers::IsScreenshare(video_sender_info.content_type)) outbound_video->content_type = "screenshare"; if (video_sender_info.encoder_implementation_name.has_value()) { outbound_video->encoder_implementation = diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 8781b323d6..b0b99cc717 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -2464,6 +2464,18 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRtpStreamStats_Video) { expected_video); EXPECT_TRUE(report->Get(*expected_video.transport_id)); EXPECT_TRUE(report->Get(*expected_video.codec_id)); + + // Make sure content type is still reported as "screenshare" even when the + // VideoContentType enum is overloaded with additional information. + videocontenttypehelpers::SetSimulcastId( + &video_media_info.receivers[0].content_type, 2); + video_media_channels.first->SetStats(video_media_info); + video_media_channels.second->SetStats(video_media_info); + + report = stats_->GetFreshStatsReport(); + EXPECT_EQ( + report->Get(expected_video.id())->cast_to(), + expected_video); } TEST_F(RTCStatsCollectorTest, CollectRTCAudioPlayoutStats) { @@ -2714,6 +2726,18 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRtpStreamStats_Video) { expected_video); EXPECT_TRUE(report->Get(*expected_video.transport_id)); EXPECT_TRUE(report->Get(*expected_video.codec_id)); + + // Make sure content type is still reported as "screenshare" even when the + // VideoContentType enum is overloaded with additional information. + videocontenttypehelpers::SetSimulcastId( + &video_media_info.senders[0].content_type, 2); + video_media_channels.first->SetStats(video_media_info); + video_media_channels.second->SetStats(video_media_info); + + report = stats_->GetFreshStatsReport(); + EXPECT_EQ( + report->Get(expected_video.id())->cast_to(), + expected_video); } TEST_F(RTCStatsCollectorTest, CollectRTCTransportStats) {