From 71eb61cf37c57e76082768029962bd7b7a41e6a6 Mon Sep 17 00:00:00 2001 From: magjed Date: Thu, 8 Sep 2016 03:24:58 -0700 Subject: [PATCH] Reland of Ignore Camera and Flip bits in CVO when parsing video rotation (patchset #1 id:1 of https://codereview.webrtc.org/2300323002/ ) Reason for revert: Downstream build is fixed. Original issue's description: > Revert of Ignore Camera and Flip bits in CVO when parsing video rotation (patchset #3 id:80001 of https://codereview.webrtc.org/2280703002/ ) > > Reason for revert: > Breaks downstream build. > > Original issue's description: > > 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 > > > > Committed: https://chromium.googlesource.com/external/webrtc/+/f9e1b922ef3e0cbe70953dfb7a1d4cb2c44a49e3 > > TBR=pthatcher@webrtc.org,danilchap@webrtc.org,tommi@webrtc.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=webrtc:6120 > > Committed: https://crrev.com/97667c7746282704acccd896e26175decee349c0 > Cr-Commit-Position: refs/heads/master@{#14035} TBR=pthatcher@webrtc.org,danilchap@webrtc.org,tommi@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:6120 Review-Url: https://codereview.webrtc.org/2320913003 Cr-Commit-Position: refs/heads/master@{#14124} --- 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 ea040fdb5c..6d30719ddc 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) @@ -697,7 +698,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 a851bc3bae..0d4ce95c6b 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 d4cba75d23..811ac77398 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -266,8 +266,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); } }; @@ -676,8 +675,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. @@ -1790,4 +1788,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;