From b3e85ff810024f6bc5957a3609e54b9df01c9032 Mon Sep 17 00:00:00 2001 From: KongQun Yang Date: Wed, 6 Jan 2016 15:52:18 -0800 Subject: [PATCH] Enable '-Wimplicit-fallthrough' and fix issues Change-Id: I9e1e09f3924dcd8f59af2fbc952456b81e2d7c4e --- packager/app/stream_descriptor.h | 2 +- packager/app/widevine_encryption_flags.cc | 3 +- packager/common.gypi | 3 +- packager/media/base/http_key_fetcher.cc | 2 + packager/media/base/macros.h | 54 +++++++++++++++++++ packager/media/base/media_base.gyp | 1 + .../media/formats/mp4/mp4_media_parser.cc | 6 +++ .../formats/webvtt/webvtt_media_parser.cc | 5 ++ .../webvtt/webvtt_media_parser_unittest.cc | 6 +-- packager/packager.gyp | 15 +----- 10 files changed, 76 insertions(+), 21 deletions(-) create mode 100644 packager/media/base/macros.h diff --git a/packager/app/stream_descriptor.h b/packager/app/stream_descriptor.h index 65b2e9c1e5..9a29772921 100644 --- a/packager/app/stream_descriptor.h +++ b/packager/app/stream_descriptor.h @@ -12,7 +12,7 @@ #include #include -#include "media/base/container_names.h" +#include "packager/media/base/container_names.h" namespace edash_packager { namespace media { diff --git a/packager/app/widevine_encryption_flags.cc b/packager/app/widevine_encryption_flags.cc index ba5caa2c96..1ddf8f8f2c 100644 --- a/packager/app/widevine_encryption_flags.cc +++ b/packager/app/widevine_encryption_flags.cc @@ -10,6 +10,7 @@ #include "packager/app/validate_flag.h" #include "packager/base/logging.h" +#include "packager/base/strings/string_piece.h" #include "packager/base/strings/string_util.h" DEFINE_bool(enable_widevine_encryption, @@ -76,7 +77,7 @@ bool ValidateWidevineCryptoFlags() { success = false; } if (widevine_crypto && FLAGS_signer.empty() && - base::StartsWith(FLAGS_key_server_url, "http", + base::StartsWith(base::StringPiece(FLAGS_key_server_url), "http", base::CompareCase::INSENSITIVE_ASCII)) { LOG(WARNING) << "--signer is likely required with " "--enable_widevine_encryption/decryption."; diff --git a/packager/common.gypi b/packager/common.gypi index feec248ebb..eebcee0948 100644 --- a/packager/common.gypi +++ b/packager/common.gypi @@ -19,8 +19,7 @@ 'conditions': [ ['clang==1', { 'cflags': [ - # Temporary workaround a gtest bug on ImplicitCast_. - '-Wno-pessimizing-move', + '-Wimplicit-fallthrough', ], # Revert the relevant settings in Chromium's common.gypi. 'cflags!': [ diff --git a/packager/media/base/http_key_fetcher.cc b/packager/media/base/http_key_fetcher.cc index 3441d49361..d7701f04de 100644 --- a/packager/media/base/http_key_fetcher.cc +++ b/packager/media/base/http_key_fetcher.cc @@ -7,6 +7,8 @@ #include "packager/media/base/http_key_fetcher.h" #include + +#include "packager/base/logging.h" #include "packager/base/strings/stringprintf.h" #include "packager/base/synchronization/lock.h" diff --git a/packager/media/base/macros.h b/packager/media/base/macros.h new file mode 100644 index 0000000000..980af75eb8 --- /dev/null +++ b/packager/media/base/macros.h @@ -0,0 +1,54 @@ +// Copyright 2016 Google Inc. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file or at +// https://developers.google.com/open-source/licenses/bsd + +#ifndef MEDIA_BASE_MACROS_H_ +#define MEDIA_BASE_MACROS_H_ + +// The FALLTHROUGH_INTENDED macro can be used to annotate implicit fall-through +// between switch labels: +// switch (x) { +// case 40: +// case 41: +// if (truth_is_out_there) { +// ++x; +// FALLTHROUGH_INTENDED; // Use instead of/along with annotations in +// // comments. +// } else { +// return x; +// } +// case 42: +// ... +// +// As shown in the example above, the FALLTHROUGH_INTENDED macro should be +// followed by a semicolon. It is designed to mimic control-flow statements +// like 'break;', so it can be placed in most places where 'break;' can, but +// only if there are no statements on the execution path between it and the +// next switch label. +// +// When compiled with clang in C++11 mode, the FALLTHROUGH_INTENDED macro is +// expanded to [[clang::fallthrough]] attribute, which is analysed when +// performing switch labels fall-through diagnostic ('-Wimplicit-fallthrough'). +// See clang documentation on language extensions for details: +// http://clang.llvm.org/docs/AttributeReference.html#fallthrough-clang-fallthrough +// +// When used with unsupported compilers, the FALLTHROUGH_INTENDED macro has no +// effect on diagnostics. +// +// In either case this macro has no effect on runtime behavior and performance +// of code. +#if defined(__clang__) && __cplusplus >= 201103L && defined(__has_warning) +#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough") +#define FALLTHROUGH_INTENDED [[clang::fallthrough]] // NOLINT +#endif +#endif + +#ifndef FALLTHROUGH_INTENDED +#define FALLTHROUGH_INTENDED \ + do { \ + } while (0) +#endif + +#endif // MEDIA_BASE_MACROS_H_ diff --git a/packager/media/base/media_base.gyp b/packager/media/base/media_base.gyp index caf5e093e3..08ceffb064 100644 --- a/packager/media/base/media_base.gyp +++ b/packager/media/base/media_base.gyp @@ -43,6 +43,7 @@ 'key_source.cc', 'key_source.h', 'limits.h', + 'macros.h', 'media_parser.h', 'media_sample.cc', 'media_sample.h', diff --git a/packager/media/formats/mp4/mp4_media_parser.cc b/packager/media/formats/mp4/mp4_media_parser.cc index 5257eb2586..f0e8e8281e 100644 --- a/packager/media/formats/mp4/mp4_media_parser.cc +++ b/packager/media/formats/mp4/mp4_media_parser.cc @@ -16,6 +16,7 @@ #include "packager/media/base/buffer_reader.h" #include "packager/media/base/decrypt_config.h" #include "packager/media/base/key_source.h" +#include "packager/media/base/macros.h" #include "packager/media/base/media_sample.h" #include "packager/media/base/video_stream_info.h" #include "packager/media/file/file.h" @@ -393,10 +394,15 @@ bool MP4MediaParser::ParseMoov(BoxReader* reader) { << actual_format << " in stsd box."; return false; } + break; case FOURCC_DTSC: + FALLTHROUGH_INTENDED; case FOURCC_DTSH: + FALLTHROUGH_INTENDED; case FOURCC_DTSL: + FALLTHROUGH_INTENDED; case FOURCC_DTSE: + FALLTHROUGH_INTENDED; case FOURCC_DTSM: extra_data = entry.ddts.extra_data; max_bitrate = entry.ddts.max_bitrate; diff --git a/packager/media/formats/webvtt/webvtt_media_parser.cc b/packager/media/formats/webvtt/webvtt_media_parser.cc index 45f731f0c4..39da52b609 100644 --- a/packager/media/formats/webvtt/webvtt_media_parser.cc +++ b/packager/media/formats/webvtt/webvtt_media_parser.cc @@ -13,6 +13,7 @@ #include "packager/base/strings/string_number_conversions.h" #include "packager/base/strings/string_split.h" #include "packager/base/strings/string_util.h" +#include "packager/media/base/macros.h" #include "packager/media/base/media_sample.h" #include "packager/media/base/text_stream_info.h" @@ -67,6 +68,7 @@ bool ReadLine(std::string* data, std::string* line) { bool TimestampToMilliseconds(const std::string& original_str, uint64_t* time_ms) { + const size_t kMinimalHoursLength = 2; const size_t kMinutesLength = 2; const size_t kSecondsLength = 2; const size_t kMillisecondsLength = 3; @@ -90,6 +92,8 @@ bool TimestampToMilliseconds(const std::string& original_str, // Check if hours is in the right format, if so get the number. // -1 for excluding colon for splitting hours and minutes. const size_t hours_length = str.size() - kMinimalLength - 1; + if (hours_length < kMinimalHoursLength) + return false; if (!base::StringToInt(str.substr(0, hours_length), &hours)) return false; str_index += hours_length; @@ -333,6 +337,7 @@ bool WebVttMediaParser::Parse(const uint8_t* buf, int size) { // timing, so fall thru. Setting state_ to kCueTiming so that the state // always matches the case. state_ = kCueTiming; + FALLTHROUGH_INTENDED; } case kCueTiming: { DCHECK(has_arrow); diff --git a/packager/media/formats/webvtt/webvtt_media_parser_unittest.cc b/packager/media/formats/webvtt/webvtt_media_parser_unittest.cc index 16b73bd9b5..81a889814f 100644 --- a/packager/media/formats/webvtt/webvtt_media_parser_unittest.cc +++ b/packager/media/formats/webvtt/webvtt_media_parser_unittest.cc @@ -146,7 +146,7 @@ TEST_F(WebVttMediaParserTest, MalformedHourTimestamp) { TEST_F(WebVttMediaParserTest, SpacesInTimestamp) { EXPECT_CALL(new_sample_callback_, Call(_, _)).Times(0); - const char kHourStringTooShort[] = + const char kSpacesInTimestamp[] = "WEBVTT\n" "\n" "0:01: 1.004 --> 0 :04:25.092\n" @@ -154,8 +154,8 @@ TEST_F(WebVttMediaParserTest, SpacesInTimestamp) { InitializeParser(); EXPECT_FALSE( - parser_.Parse(reinterpret_cast(kHourStringTooShort), - arraysize(kHourStringTooShort) - 1)); + parser_.Parse(reinterpret_cast(kSpacesInTimestamp), + arraysize(kSpacesInTimestamp) - 1)); } MATCHER_P(MatchesPayload, data, "") { diff --git a/packager/packager.gyp b/packager/packager.gyp index 8589001eb0..ce1ffc01b8 100644 --- a/packager/packager.gyp +++ b/packager/packager.gyp @@ -89,20 +89,6 @@ 'testing/gtest.gyp:gtest', ], }, - { - 'target_name': 'All', - 'type': 'none', - 'dependencies': [ - 'media/base/media_base.gyp:*', - 'media/event/media_event.gyp:*', - 'media/file/file.gyp:*', - 'media/formats/mp2t/mp2t.gyp:*', - 'media/formats/mp4/mp4.gyp:*', - 'media/formats/webm/webm.gyp:*', - 'media/formats/wvm/wvm.gyp:*', - 'mpd/mpd.gyp:*', - ], - }, { 'target_name': 'packager_builder_tests', 'type': 'none', @@ -114,6 +100,7 @@ 'media/formats/mp2t/mp2t.gyp:mp2t_unittest', 'media/formats/mp4/mp4.gyp:mp4_unittest', 'media/formats/webm/webm.gyp:webm_unittest', + 'media/formats/webvtt/webvtt.gyp:webvtt_unittest', 'media/formats/wvm/wvm.gyp:wvm_unittest', 'mpd/mpd.gyp:mpd_unittest', 'packager_test',