From f9e1b922ef3e0cbe70953dfb7a1d4cb2c44a49e3 Mon Sep 17 00:00:00 2001 From: Magnus Jedvert Date: Thu, 1 Sep 2016 19:58:21 +0200 Subject: [PATCH] Ignore Camera and Flip bits in CVO when parsing video rotation Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is set, then rotation is incorrectly parsed as 0. This CL fixes that issue. The Camera and Flip bit is still unimplemented and will just be ignored though. BUG=webrtc:6120 R=danilchap@webrtc.org, pthatcher@webrtc.org, tommi@webrtc.org Review URL: https://codereview.webrtc.org/2280703002 . Cr-Commit-Position: refs/heads/master@{#14027} --- webrtc/DEPS | 1 + webrtc/common_types.cc | 2 +- webrtc/common_types.h | 3 +- webrtc/modules/rtp_rtcp/include/rtp_cvo.h | 6 ++-- .../rtp_rtcp/source/rtp_header_extensions.cc | 2 +- .../rtp_rtcp/source/rtp_receiver_video.cc | 4 +-- .../rtp_rtcp/source/rtp_sender_unittest.cc | 28 ++++++++++++++++--- webrtc/modules/rtp_rtcp/source/rtp_utility.cc | 6 ++-- webrtc/sdk/DEPS | 1 - webrtc/video/rtp_stream_receiver.cc | 3 +- 10 files changed, 40 insertions(+), 16 deletions(-) diff --git a/webrtc/DEPS b/webrtc/DEPS index 41e0b712ce..ffb67d17f5 100644 --- a/webrtc/DEPS +++ b/webrtc/DEPS @@ -13,6 +13,7 @@ include_rules = [ "+webrtc/call.h", "+webrtc/common.h", "+webrtc/common_types.h", + "+webrtc/common_video/rotation.h", "+webrtc/config.h", "+webrtc/engine_configurations.h", "+webrtc/transport.h", diff --git a/webrtc/common_types.cc b/webrtc/common_types.cc index 86e7813e58..8f117e1153 100644 --- a/webrtc/common_types.cc +++ b/webrtc/common_types.cc @@ -27,7 +27,7 @@ RTPHeaderExtension::RTPHeaderExtension() voiceActivity(false), audioLevel(0), hasVideoRotation(false), - videoRotation(0) {} + videoRotation(kVideoRotation_0) {} RTPHeader::RTPHeader() : markerBit(false), diff --git a/webrtc/common_types.h b/webrtc/common_types.h index 1ef5f91674..cad748aaae 100644 --- a/webrtc/common_types.h +++ b/webrtc/common_types.h @@ -18,6 +18,7 @@ #include #include +#include "webrtc/common_video/rotation.h" #include "webrtc/typedefs.h" #if defined(_MSC_VER) @@ -699,7 +700,7 @@ struct RTPHeaderExtension { // http://www.etsi.org/deliver/etsi_ts/126100_126199/126114/12.07.00_60/ // ts_126114v120700p.pdf bool hasVideoRotation; - uint8_t videoRotation; + VideoRotation videoRotation; PlayoutDelay playout_delay = {-1, -1}; }; diff --git a/webrtc/modules/rtp_rtcp/include/rtp_cvo.h b/webrtc/modules/rtp_rtcp/include/rtp_cvo.h index 5d5c995cd5..b60b06e5df 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_cvo.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_cvo.h @@ -34,8 +34,10 @@ inline uint8_t ConvertVideoRotationToCVOByte(VideoRotation rotation) { return 0; } -inline VideoRotation ConvertCVOByteToVideoRotation(uint8_t rotation) { - switch (rotation) { +inline VideoRotation ConvertCVOByteToVideoRotation(uint8_t cvo_byte) { + // CVO byte: |0 0 0 0 C F R R|. + const uint8_t rotation_bits = cvo_byte & 0x3; + switch (rotation_bits) { case 0: return kVideoRotation_0; case 1: diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc index 2d5dbf91b4..6e65ed3d8d 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc @@ -177,7 +177,7 @@ bool VideoOrientation::IsSupportedFor(MediaType type) { } bool VideoOrientation::Parse(const uint8_t* data, VideoRotation* rotation) { - *rotation = ConvertCVOByteToVideoRotation(data[0] & 0x03); + *rotation = ConvertCVOByteToVideoRotation(data[0]); return true; } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc index 7667c46f06..9f6d80ae0a 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc @@ -94,8 +94,8 @@ int32_t RTPReceiverVideo::ParseRtpPacket(WebRtcRTPHeader* rtp_header, // Retrieve the video rotation information. if (rtp_header->header.extension.hasVideoRotation) { - rtp_header->type.Video.rotation = ConvertCVOByteToVideoRotation( - rtp_header->header.extension.videoRotation); + rtp_header->type.Video.rotation = + rtp_header->header.extension.videoRotation; } rtp_header->type.Video.playout_delay = diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc index e11bf1ee5a..b9f061a9d3 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -262,8 +262,7 @@ class RtpSenderVideoTest : public RtpSenderTest { EXPECT_EQ(rtp_sender_->SSRC(), rtp_header.ssrc); EXPECT_EQ(0, rtp_header.numCSRCs); EXPECT_EQ(0U, rtp_header.paddingLength); - EXPECT_EQ(ConvertVideoRotationToCVOByte(rotation), - rtp_header.extension.videoRotation); + EXPECT_EQ(rotation, rtp_header.extension.videoRotation); } }; @@ -577,8 +576,7 @@ TEST_F(RtpSenderTestWithoutPacer, BuildRTPPacketWithVideoRotation_MarkerBit) { VerifyRTPHeaderCommon(rtp_header); EXPECT_EQ(length, rtp_header.headerLength); EXPECT_TRUE(rtp_header.extension.hasVideoRotation); - EXPECT_EQ(ConvertVideoRotationToCVOByte(kRotation), - rtp_header.extension.videoRotation); + EXPECT_EQ(kRotation, rtp_header.extension.videoRotation); } // Test CVO header extension is not set when marker bit is false. @@ -1691,4 +1689,26 @@ TEST_F(RtpSenderVideoTest, SendVideoWithCVO) { transport_.sent_packets_[1]->size(), true, &map, kSeqNum + 1, hdr.rotation); } + +// Make sure rotation is parsed correctly when the Camera (C) and Flip (F) bits +// are set in the CVO byte. +TEST_F(RtpSenderVideoTest, SendVideoWithCameraAndFlipCVO) { + // Test extracting rotation when Camera (C) and Flip (F) bits are zero. + EXPECT_EQ(kVideoRotation_0, ConvertCVOByteToVideoRotation(0)); + EXPECT_EQ(kVideoRotation_90, ConvertCVOByteToVideoRotation(1)); + EXPECT_EQ(kVideoRotation_180, ConvertCVOByteToVideoRotation(2)); + EXPECT_EQ(kVideoRotation_270, ConvertCVOByteToVideoRotation(3)); + // Test extracting rotation when Camera (C) and Flip (F) bits are set. + const int flip_bit = 1 << 2; + const int camera_bit = 1 << 3; + EXPECT_EQ(kVideoRotation_0, + ConvertCVOByteToVideoRotation(flip_bit | camera_bit | 0)); + EXPECT_EQ(kVideoRotation_90, + ConvertCVOByteToVideoRotation(flip_bit | camera_bit | 1)); + EXPECT_EQ(kVideoRotation_180, + ConvertCVOByteToVideoRotation(flip_bit | camera_bit | 2)); + EXPECT_EQ(kVideoRotation_270, + ConvertCVOByteToVideoRotation(flip_bit | camera_bit | 3)); +} + } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtp_utility.cc b/webrtc/modules/rtp_rtcp/source/rtp_utility.cc index e38b3a3b43..7dd59736fd 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_utility.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_utility.cc @@ -13,6 +13,7 @@ #include #include "webrtc/base/logging.h" +#include "webrtc/modules/rtp_rtcp/include/rtp_cvo.h" #include "webrtc/modules/rtp_rtcp/source/byte_io.h" namespace webrtc { @@ -246,7 +247,7 @@ bool RtpHeaderParser::Parse(RTPHeader* header, // May not be present in packet. header->extension.hasVideoRotation = false; - header->extension.videoRotation = 0; + header->extension.videoRotation = kVideoRotation_0; // May not be present in packet. header->extension.playout_delay.min_ms = -1; @@ -397,7 +398,8 @@ void RtpHeaderParser::ParseOneByteExtensionHeader( // | ID | len=0 |0 0 0 0 C F R R| // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ header->extension.hasVideoRotation = true; - header->extension.videoRotation = ptr[0]; + header->extension.videoRotation = + ConvertCVOByteToVideoRotation(ptr[0]); break; } case kRtpExtensionTransportSequenceNumber: { diff --git a/webrtc/sdk/DEPS b/webrtc/sdk/DEPS index 16ef0f6a06..d6e53d504d 100644 --- a/webrtc/sdk/DEPS +++ b/webrtc/sdk/DEPS @@ -2,7 +2,6 @@ include_rules = [ "+WebRTC", "+webrtc/api", "+webrtc/common_video/include", - "+webrtc/common_video/rotation.h", "+webrtc/media", "+webrtc/system_wrappers", ] diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index d5b547f629..20acc06216 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -443,8 +443,7 @@ void RtpStreamReceiver::NotifyReceiverOfFecPacket(const RTPHeader& header) { rtp_header.type.Video.codec = payload_specific.Video.videoCodecType; rtp_header.type.Video.rotation = kVideoRotation_0; if (header.extension.hasVideoRotation) { - rtp_header.type.Video.rotation = - ConvertCVOByteToVideoRotation(header.extension.videoRotation); + rtp_header.type.Video.rotation = header.extension.videoRotation; } rtp_header.type.Video.playout_delay = header.extension.playout_delay;