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

@@ -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);

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<CharReader> source)
+Status FileReader::Open(const std::string& filename,
+                        std::unique_ptr<FileReader>* out) {
+  const char* kReadOnly = "r";
+
+  std::unique_ptr<File, FileCloser> file(
+      File::Open(filename.c_str(), kReadOnly));
+
+  if (!file) {
+    return Status(error::INVALID_ARGUMENT,
+                  "Could not open input file " + filename);
+  }
+
+  *out = std::unique_ptr<FileReader>(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, FileCloser> file)
+    : file_(std::move(file)) {
+  DCHECK(file_);
+}
+
+PeekingReader::PeekingReader(std::unique_ptr<FileReader> 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<CharReader> source)
+LineReader::LineReader(std::unique_ptr<FileReader> 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<CharReader> source)
+BlockReader::BlockReader(std::unique_ptr<FileReader> source)
     : source_(std::move(source)) {}
 
 bool BlockReader::Next(std::vector<std::string>* out) {
@@ -89,15 +118,5 @@ bool BlockReader::Next(std::vector<std::string>* 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

packager/media/formats/webvtt/text_readers.h

@@ -11,33 +11,57 @@
 #include <string>
 #include <vector>
 
+#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<FileReader>* out);
 
-class PeekingCharReader : public CharReader {
+  /// Read the next character from the file. If there is a next character,
- public:
+  /// |out| will be set and true will be returned. If there is no next
-  explicit PeekingCharReader(std::unique_ptr<CharReader> source);
+  /// character false will be returned.
-
+  bool Next(char* out);
-  bool Next(char* out) override;
-  bool Peek(char* out);
 
  private:
-  PeekingCharReader(const PeekingCharReader&) = delete;
+  explicit FileReader(std::unique_ptr<File, FileCloser> file);
-  PeekingCharReader operator=(const PeekingCharReader&) = delete;
 
-  std::unique_ptr<CharReader> source_;
+  FileReader(const FileReader& reader) = delete;
+  FileReader operator=(const FileReader& reader) = delete;
+
+  std::unique_ptr<File, FileCloser> file_;
+};
+
+class PeekingReader {
+ public:
+  explicit PeekingReader(std::unique_ptr<FileReader> source);
+
+  bool Peek(char* out);
+  bool Next(char* out);
+
+ private:
+  PeekingReader(const PeekingReader&) = delete;
+  PeekingReader operator=(const PeekingReader&) = delete;
+
+  std::unique_ptr<FileReader> source_;
   char cached_next_ = 0;
   bool has_cached_next_ = false;
 };
 
 class LineReader {
  public:
-  explicit LineReader(std::unique_ptr<CharReader> source);
+  explicit LineReader(std::unique_ptr<FileReader> 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<CharReader> source);
+  explicit BlockReader(std::unique_ptr<FileReader> source);
 
   bool Next(std::vector<std::string>* 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
 

packager/media/formats/webvtt/text_readers_unittest.cc

@@ -6,31 +6,45 @@
 
 #include <gtest/gtest.h>
 
+#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";
+  const char* text = "abcd";
-  StringCharReader source(input);
+
+  ASSERT_TRUE(File::WriteStringToFile(kFilename, text));
+
+  std::unique_ptr<FileReader> 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";
+  const char* text = "abc";
-  std::unique_ptr<CharReader> source(new StringCharReader(input));
+
-  PeekingCharReader reader(std::move(source));
+  ASSERT_TRUE(File::WriteStringToFile(kFilename, text));
+
+  std::unique_ptr<FileReader> 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";
+  const char* text = "a\nb\nc";
-  std::unique_ptr<CharReader> source(new StringCharReader(input));
+
+  ASSERT_TRUE(File::WriteStringToFile(kFilename, text));
+
+  std::unique_ptr<FileReader> 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";
+  const char* text = "a\r\nb\r\nc";
-  std::unique_ptr<CharReader> source(new StringCharReader(input));
+
+  ASSERT_TRUE(File::WriteStringToFile(kFilename, text));
+
+  std::unique_ptr<FileReader> 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";
+  const char* text = "a\n\rb\n\rc";
-  std::unique_ptr<CharReader> source(new StringCharReader(input));
+
+  ASSERT_TRUE(File::WriteStringToFile(kFilename, text));
+
+  std::unique_ptr<FileReader> 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";
+  const char* text = "a\r\nb\r\nc\r";
-  std::unique_ptr<CharReader> source(new StringCharReader(input));
+
+  ASSERT_TRUE(File::WriteStringToFile(kFilename, text));
+
+  std::unique_ptr<FileReader> 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<CharReader> source(new StringCharReader(input));
+  ASSERT_TRUE(File::WriteStringToFile(kFilename, text));
+
+  std::unique_ptr<FileReader> source;
+  ASSERT_OK(FileReader::Open(kFilename, &source));
+
   BlockReader reader(std::move(source));
 
   std::vector<std::string> 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<CharReader> source(new StringCharReader(input));
+  ASSERT_TRUE(File::WriteStringToFile(kFilename, text));
+
+  std::unique_ptr<FileReader> source;
+  ASSERT_OK(FileReader::Open(kFilename, &source));
+
   BlockReader reader(std::move(source));
 
   std::vector<std::string> 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<FileReader> source;
+  ASSERT_OK(FileReader::Open(kFilename, &source));
 
-  std::unique_ptr<CharReader> source(new StringCharReader(input));
   BlockReader reader(std::move(source));
 
   std::vector<std::string> block;

packager/media/formats/webvtt/webvtt_parser.cc

@@ -46,7 +46,7 @@ bool MaybeCueId(const std::string& line) {
 }
 }  // namespace
 
-WebVttParser::WebVttParser(std::unique_ptr<CharReader> source)
+WebVttParser::WebVttParser(std::unique_ptr<FileReader> source)
     : reader_(std::move(source)) {}
 
 Status WebVttParser::InitializeInternal() {

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<CharReader> source);
+  explicit WebVttParser(std::unique_ptr<FileReader> source);
 
   Status Run() override;
   void Cancel() override;

packager/media/formats/webvtt/webvtt_parser_unittest.cc

@@ -7,6 +7,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#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) {
+  void SetUpAndInitializeGraph(const char* text) {
-    parser_ = std::make_shared<WebVttParser>(
+    const char* kFilename = "memory://test-file";
-        std::unique_ptr<StringCharReader>(new StringCharReader(text_input)));
+
+    // 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<FileReader> reader;
+    ASSERT_OK(FileReader::Open(kFilename, &reader));
+
+    parser_ = std::make_shared<WebVttParser>(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;