From 9e7b5c1ca9d3b9d1453ed8b370f4ceffdd1f57b3 Mon Sep 17 00:00:00 2001 From: Aaron Vaage Date: Tue, 2 Jan 2018 16:35:38 -0800 Subject: [PATCH] Change CharReader to FileReader Instead of having multiple char readers, one for strings and one for files, just have one for files and use memory files when testing. Change-Id: Id1a2230046ba540ddf69ca10edb3edc74d2419b6 --- packager/file/memory_file.cc | 7 ++ packager/media/formats/webvtt/text_readers.cc | 49 +++++++--- packager/media/formats/webvtt/text_readers.h | 68 ++++++++------ .../formats/webvtt/text_readers_unittest.cc | 94 ++++++++++++++----- .../media/formats/webvtt/webvtt_parser.cc | 2 +- packager/media/formats/webvtt/webvtt_parser.h | 2 +- .../formats/webvtt/webvtt_parser_unittest.cc | 86 ++++++++++++----- 7 files changed, 212 insertions(+), 96 deletions(-) diff --git a/packager/file/memory_file.cc b/packager/file/memory_file.cc index 92471d2d50..60a4a801a4 100644 --- a/packager/file/memory_file.cc +++ b/packager/file/memory_file.cc @@ -77,6 +77,13 @@ int64_t MemoryFile::Read(void* buffer, uint64_t length) { } int64_t MemoryFile::Write(const void* buffer, uint64_t length) { + // If length is zero, we won't resize the buffer and it is possible for + // |position| to equal the length of the buffer. This will cause a segfault + // when indexing into the buffer for the memcpy. + if (length == 0) { + return 0; + } + const uint64_t size = Size(); if (size < position_ + length) { file_->resize(position_ + length); diff --git a/packager/media/formats/webvtt/text_readers.cc b/packager/media/formats/webvtt/text_readers.cc index 9d77f4c5f3..02fa37dccc 100644 --- a/packager/media/formats/webvtt/text_readers.cc +++ b/packager/media/formats/webvtt/text_readers.cc @@ -7,14 +7,43 @@ #include "packager/media/formats/webvtt/text_readers.h" #include "packager/base/logging.h" +#include "packager/file/file.h" namespace shaka { namespace media { -PeekingCharReader::PeekingCharReader(std::unique_ptr source) +Status FileReader::Open(const std::string& filename, + std::unique_ptr* out) { + const char* kReadOnly = "r"; + + std::unique_ptr file( + File::Open(filename.c_str(), kReadOnly)); + + if (!file) { + return Status(error::INVALID_ARGUMENT, + "Could not open input file " + filename); + } + + *out = std::unique_ptr(new FileReader(std::move(file))); + + return Status::OK; +} + +bool FileReader::Next(char* out) { + // TODO(vaage): If file reading performance is poor, change this to buffer + // data and read from the buffer. + return file_->Read(out, 1) == 1; +} + +FileReader::FileReader(std::unique_ptr file) + : file_(std::move(file)) { + DCHECK(file_); +} + +PeekingReader::PeekingReader(std::unique_ptr source) : source_(std::move(source)) {} -bool PeekingCharReader::Next(char* out) { +bool PeekingReader::Next(char* out) { DCHECK(out); if (Peek(out)) { has_cached_next_ = false; @@ -23,7 +52,7 @@ bool PeekingCharReader::Next(char* out) { return false; } -bool PeekingCharReader::Peek(char* out) { +bool PeekingReader::Peek(char* out) { DCHECK(out); if (!has_cached_next_ && source_->Next(&cached_next_)) { has_cached_next_ = true; @@ -35,7 +64,7 @@ bool PeekingCharReader::Peek(char* out) { return false; } -LineReader::LineReader(std::unique_ptr source) +LineReader::LineReader(std::unique_ptr source) : source_(std::move(source)) {} // Split lines based on https://w3c.github.io/webvtt/#webvtt-line-terminator @@ -63,7 +92,7 @@ bool LineReader::Next(std::string* out) { return read_something; } -BlockReader::BlockReader(std::unique_ptr source) +BlockReader::BlockReader(std::unique_ptr source) : source_(std::move(source)) {} bool BlockReader::Next(std::vector* out) { @@ -89,15 +118,5 @@ bool BlockReader::Next(std::vector* out) { return in_block; } - -StringCharReader::StringCharReader(const std::string& str) : source_(str) {} - -bool StringCharReader::Next(char* out) { - if (pos_ < source_.length()) { - *out = source_[pos_++]; - return true; - } - return false; -} } // namespace media } // namespace shaka diff --git a/packager/media/formats/webvtt/text_readers.h b/packager/media/formats/webvtt/text_readers.h index 12c219e89b..4fe0772148 100644 --- a/packager/media/formats/webvtt/text_readers.h +++ b/packager/media/formats/webvtt/text_readers.h @@ -11,33 +11,57 @@ #include #include +#include "packager/file/file_closer.h" +#include "packager/status.h" + namespace shaka { +class File; + namespace media { -class CharReader { +/// Class to read character-by-character from a file. +class FileReader { public: - virtual bool Next(char* out) = 0; -}; + /// Create a new file reader by opening a file. If the file fails to open (in + /// readonly mode) a non-ok status will be returned. If the file successfully + /// opens, |out| will be set to a new FileReader and an ok status will be + /// returned. + static Status Open(const std::string& filename, + std::unique_ptr* out); -class PeekingCharReader : public CharReader { - public: - explicit PeekingCharReader(std::unique_ptr source); - - bool Next(char* out) override; - bool Peek(char* out); + /// Read the next character from the file. If there is a next character, + /// |out| will be set and true will be returned. If there is no next + /// character false will be returned. + bool Next(char* out); private: - PeekingCharReader(const PeekingCharReader&) = delete; - PeekingCharReader operator=(const PeekingCharReader&) = delete; + explicit FileReader(std::unique_ptr file); - std::unique_ptr source_; + FileReader(const FileReader& reader) = delete; + FileReader operator=(const FileReader& reader) = delete; + + std::unique_ptr file_; +}; + +class PeekingReader { + public: + explicit PeekingReader(std::unique_ptr source); + + bool Peek(char* out); + bool Next(char* out); + + private: + PeekingReader(const PeekingReader&) = delete; + PeekingReader operator=(const PeekingReader&) = delete; + + std::unique_ptr source_; char cached_next_ = 0; bool has_cached_next_ = false; }; class LineReader { public: - explicit LineReader(std::unique_ptr source); + explicit LineReader(std::unique_ptr source); bool Next(std::string* out); @@ -45,12 +69,12 @@ class LineReader { LineReader(const LineReader&) = delete; LineReader operator=(const LineReader&) = delete; - PeekingCharReader source_; + PeekingReader source_; }; class BlockReader { public: - explicit BlockReader(std::unique_ptr source); + explicit BlockReader(std::unique_ptr source); bool Next(std::vector* out); @@ -61,20 +85,6 @@ class BlockReader { LineReader source_; }; -class StringCharReader : public CharReader { - public: - explicit StringCharReader(const std::string& str); - - bool Next(char* out) override; - - private: - StringCharReader(const StringCharReader&) = delete; - StringCharReader& operator=(const StringCharReader&) = delete; - - const std::string source_; - size_t pos_ = 0; -}; - } // namespace media } // namespace shaka diff --git a/packager/media/formats/webvtt/text_readers_unittest.cc b/packager/media/formats/webvtt/text_readers_unittest.cc index a399433e60..85dc1c5839 100644 --- a/packager/media/formats/webvtt/text_readers_unittest.cc +++ b/packager/media/formats/webvtt/text_readers_unittest.cc @@ -6,31 +6,45 @@ #include +#include "packager/file/file.h" #include "packager/media/formats/webvtt/text_readers.h" +#include "packager/status_test_util.h" namespace shaka { namespace media { +namespace { +const char* kFilename = "memory://test-file"; +} // namespace TEST(TextReadersTest, ReadWholeStream) { - const std::string input = "abcd"; - StringCharReader source(input); + const char* text = "abcd"; + + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); char c; - ASSERT_TRUE(source.Next(&c)); + ASSERT_TRUE(source->Next(&c)); ASSERT_EQ(c, 'a'); - ASSERT_TRUE(source.Next(&c)); + ASSERT_TRUE(source->Next(&c)); ASSERT_EQ(c, 'b'); - ASSERT_TRUE(source.Next(&c)); + ASSERT_TRUE(source->Next(&c)); ASSERT_EQ(c, 'c'); - ASSERT_TRUE(source.Next(&c)); + ASSERT_TRUE(source->Next(&c)); ASSERT_EQ(c, 'd'); - ASSERT_FALSE(source.Next(&c)); + ASSERT_FALSE(source->Next(&c)); } TEST(TextReadersTest, Peeking) { - const std::string input = "abc"; - std::unique_ptr source(new StringCharReader(input)); - PeekingCharReader reader(std::move(source)); + const char* text = "abc"; + + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); + + PeekingReader reader(std::move(source)); char c; ASSERT_TRUE(reader.Peek(&c)); @@ -50,8 +64,13 @@ TEST(TextReadersTest, Peeking) { } TEST(TextReadersTest, ReadLinesWithNewLine) { - const std::string input = "a\nb\nc"; - std::unique_ptr source(new StringCharReader(input)); + const char* text = "a\nb\nc"; + + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); + LineReader reader(std::move(source)); std::string s; @@ -65,8 +84,13 @@ TEST(TextReadersTest, ReadLinesWithNewLine) { } TEST(TextReadersTest, ReadLinesWithReturnsAndNewLine) { - const std::string input = "a\r\nb\r\nc"; - std::unique_ptr source(new StringCharReader(input)); + const char* text = "a\r\nb\r\nc"; + + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); + LineReader reader(std::move(source)); std::string s; @@ -80,8 +104,13 @@ TEST(TextReadersTest, ReadLinesWithReturnsAndNewLine) { } TEST(TextReadersTest, ReadLinesWithNewLineAndReturns) { - const std::string input = "a\n\rb\n\rc"; - std::unique_ptr source(new StringCharReader(input)); + const char* text = "a\n\rb\n\rc"; + + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); + LineReader reader(std::move(source)); std::string s; @@ -99,8 +128,13 @@ TEST(TextReadersTest, ReadLinesWithNewLineAndReturns) { } TEST(TextReadersTest, ReadLinesWithReturnAtEnd) { - const std::string input = "a\r\nb\r\nc\r"; - std::unique_ptr source(new StringCharReader(input)); + const char* text = "a\r\nb\r\nc\r"; + + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); + LineReader reader(std::move(source)); std::string s; @@ -114,11 +148,15 @@ TEST(TextReadersTest, ReadLinesWithReturnAtEnd) { } TEST(TextReadersTest, ReadBlocksReadMultilineBlock) { - const std::string input = + const char* text = "block 1 - line 1\n" "block 1 - line 2"; - std::unique_ptr source(new StringCharReader(input)); + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); + BlockReader reader(std::move(source)); std::vector block; @@ -132,7 +170,7 @@ TEST(TextReadersTest, ReadBlocksReadMultilineBlock) { } TEST(TextReadersTest, ReadBlocksSkipBlankLinesBeforeBlocks) { - const std::string input = + const char* text = "\n" "\n" "block 1\n" @@ -140,7 +178,11 @@ TEST(TextReadersTest, ReadBlocksSkipBlankLinesBeforeBlocks) { "\n" "block 2\n"; - std::unique_ptr source(new StringCharReader(input)); + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); + BlockReader reader(std::move(source)); std::vector block; @@ -157,9 +199,13 @@ TEST(TextReadersTest, ReadBlocksSkipBlankLinesBeforeBlocks) { } TEST(TextReadersTest, ReadBlocksWithOnlyBlankLines) { - const std::string input = "\n\n\n\n"; + const char* text = "\n\n\n\n"; + + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + std::unique_ptr source; + ASSERT_OK(FileReader::Open(kFilename, &source)); - std::unique_ptr source(new StringCharReader(input)); BlockReader reader(std::move(source)); std::vector block; diff --git a/packager/media/formats/webvtt/webvtt_parser.cc b/packager/media/formats/webvtt/webvtt_parser.cc index 9b285d7b9d..ff5771dbf4 100644 --- a/packager/media/formats/webvtt/webvtt_parser.cc +++ b/packager/media/formats/webvtt/webvtt_parser.cc @@ -46,7 +46,7 @@ bool MaybeCueId(const std::string& line) { } } // namespace -WebVttParser::WebVttParser(std::unique_ptr source) +WebVttParser::WebVttParser(std::unique_ptr source) : reader_(std::move(source)) {} Status WebVttParser::InitializeInternal() { diff --git a/packager/media/formats/webvtt/webvtt_parser.h b/packager/media/formats/webvtt/webvtt_parser.h index 5a532009dc..2359c54e75 100644 --- a/packager/media/formats/webvtt/webvtt_parser.h +++ b/packager/media/formats/webvtt/webvtt_parser.h @@ -20,7 +20,7 @@ namespace media { // Used to parse a WebVTT source into Cues that will be sent downstream. class WebVttParser : public OriginHandler { public: - explicit WebVttParser(std::unique_ptr source); + explicit WebVttParser(std::unique_ptr source); Status Run() override; void Cancel() override; diff --git a/packager/media/formats/webvtt/webvtt_parser_unittest.cc b/packager/media/formats/webvtt/webvtt_parser_unittest.cc index 10c289fad9..786fdc05bc 100644 --- a/packager/media/formats/webvtt/webvtt_parser_unittest.cc +++ b/packager/media/formats/webvtt/webvtt_parser_unittest.cc @@ -7,6 +7,7 @@ #include #include +#include "packager/file/file.h" #include "packager/media/base/media_handler_test_base.h" #include "packager/media/formats/webvtt/text_readers.h" #include "packager/media/formats/webvtt/webvtt_parser.h" @@ -29,9 +30,18 @@ const char* kNoSettings = ""; class WebVttParserTest : public MediaHandlerTestBase { protected: - void SetUpAndInitializeGraph(const std::string& text_input) { - parser_ = std::make_shared( - std::unique_ptr(new StringCharReader(text_input))); + void SetUpAndInitializeGraph(const char* text) { + const char* kFilename = "memory://test-file"; + + // Create the input file from the text passed to the test. + ASSERT_TRUE(File::WriteStringToFile(kFilename, text)); + + // Read from the file we just wrote. + std::unique_ptr reader; + ASSERT_OK(FileReader::Open(kFilename, &reader)); + + parser_ = std::make_shared(std::move(reader)); + ASSERT_OK(MediaHandlerTestBase::SetUpAndInitializeGraph( parser_, kInputCount, kOutputCount)); } @@ -40,7 +50,9 @@ class WebVttParserTest : public MediaHandlerTestBase { }; TEST_F(WebVttParserTest, FailToParseEmptyFile) { - SetUpAndInitializeGraph(""); + const char* text = ""; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); EXPECT_CALL(*Output(kOutputIndex), OnProcess(testing::_)).Times(0); EXPECT_CALL(*Output(kOutputIndex), OnFlush(testing::_)).Times(0); @@ -49,9 +61,11 @@ TEST_F(WebVttParserTest, FailToParseEmptyFile) { } TEST_F(WebVttParserTest, ParseOnlyHeader) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" - "\n"); + "\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); { testing::InSequence s; @@ -64,9 +78,11 @@ TEST_F(WebVttParserTest, ParseOnlyHeader) { } TEST_F(WebVttParserTest, ParseHeaderWithBOM) { - SetUpAndInitializeGraph( + const char* text = "\xFE\xFFWEBVTT\n" - "\n"); + "\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); { testing::InSequence s; @@ -79,9 +95,11 @@ TEST_F(WebVttParserTest, ParseHeaderWithBOM) { } TEST_F(WebVttParserTest, FailToParseHeaderWrongWord) { - SetUpAndInitializeGraph( + const char* text = "NOT WEBVTT\n" - "\n"); + "\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); EXPECT_CALL(*Output(kOutputIndex), OnProcess(testing::_)).Times(0); EXPECT_CALL(*Output(kOutputIndex), OnFlush(testing::_)).Times(0); @@ -90,10 +108,12 @@ TEST_F(WebVttParserTest, FailToParseHeaderWrongWord) { } TEST_F(WebVttParserTest, FailToParseHeaderNotOneLine) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" "WEBVTT\n" - "\n"); + "\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); EXPECT_CALL(*Output(kOutputIndex), OnProcess(testing::_)).Times(0); EXPECT_CALL(*Output(kOutputIndex), OnFlush(testing::_)).Times(0); @@ -104,14 +124,16 @@ TEST_F(WebVttParserTest, FailToParseHeaderNotOneLine) { // TODO: Add style blocks support to WebVttParser. // This test is disabled until WebVTT parses STYLE blocks. TEST_F(WebVttParserTest, DISABLED_ParseStyleBlocks) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" "\n" "STYLE\n" "::cue {\n" " background-image: linear-gradient(to bottom, dimgray, lightgray);\n" " color: papayawhip;\n" - "}"); + "}"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); { testing::InSequence s; @@ -124,11 +146,13 @@ TEST_F(WebVttParserTest, DISABLED_ParseStyleBlocks) { } TEST_F(WebVttParserTest, ParseOneCue) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" "\n" "00:01:00.000 --> 01:00:00.000\n" - "subtitle\n"); + "subtitle\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); { testing::InSequence s; @@ -144,23 +168,27 @@ TEST_F(WebVttParserTest, ParseOneCue) { } TEST_F(WebVttParserTest, FailToParseCueWithArrowInId) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" "\n" "-->\n" "00:01:00.000 --> 01:00:00.000\n" - "subtitle\n"); + "subtitle\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); ASSERT_NE(Status::OK, parser_->Run()); } TEST_F(WebVttParserTest, ParseOneCueWithId) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" "\n" "id\n" "00:01:00.000 --> 01:00:00.000\n" - "subtitle\n"); + "subtitle\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); { testing::InSequence s; @@ -176,11 +204,13 @@ TEST_F(WebVttParserTest, ParseOneCueWithId) { } TEST_F(WebVttParserTest, ParseOneCueWithSettings) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" "\n" "00:01:00.000 --> 01:00:00.000 size:50%\n" - "subtitle\n"); + "subtitle\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); { testing::InSequence s; @@ -197,7 +227,7 @@ TEST_F(WebVttParserTest, ParseOneCueWithSettings) { // Verify that a typical case with mulitple cues work. TEST_F(WebVttParserTest, ParseMultipleCues) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" "\n" "00:00:01.000 --> 00:00:05.200\n" @@ -207,7 +237,9 @@ TEST_F(WebVttParserTest, ParseMultipleCues) { "subtitle B\n" "\n" "00:00:05.800 --> 00:00:08.000\n" - "subtitle C\n"); + "subtitle C\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); { testing::InSequence s; @@ -231,7 +263,7 @@ TEST_F(WebVttParserTest, ParseMultipleCues) { // Verify that a typical case with mulitple cues work even when comments are // present. TEST_F(WebVttParserTest, ParseWithComments) { - SetUpAndInitializeGraph( + const char* text = "WEBVTT\n" "\n" "NOTE This is a one line comment\n" @@ -251,7 +283,9 @@ TEST_F(WebVttParserTest, ParseWithComments) { "NOTE\tThis is a comment that using a tab\n" "\n" "00:00:05.800 --> 00:00:08.000\n" - "subtitle C\n"); + "subtitle C\n"; + + ASSERT_NO_FATAL_FAILURE(SetUpAndInitializeGraph(text)); { testing::InSequence s;