TextRenderer only pushes new cues downstream

media/base/text_ranges.cc

Index: media/base/text_ranges.cc
diff --git a/media/base/text_ranges.cc b/media/base/text_ranges.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e7fbe82a1c391291cc03ac86b78e108346aef8ad
--- /dev/null
+++ b/media/base/text_ranges.cc
@@ -0,0 +1,145 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/base/text_ranges.h"
+
+#include "base/logging.h"
+
+namespace media {
+
+TextRanges::TextRanges() {
+  Flush();
+}
+
+void TextRanges::Flush() {
+  curr_range_itr_ = range_map_.end();
+}
+
+bool TextRanges::AddCue(base::TimeDelta start_time) {
+  typedef RangeMap::iterator Itr;

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: Please just use the full type below.

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

ack

+
+  if (curr_range_itr_ == range_map_.end()) {
+    // No is no active time range, so this is the first cue that
+    // follows a ssek.

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit:s/ssek/Reset()/

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+
+    if (range_map_.empty()) {
+      NewRange(start_time);
+      return true;
+    }
+
+    const Itr first_itr = range_map_.begin();

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: Inline this since it is only used on the next line.

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+
+    if (start_time < first_itr->first) {
+      NewRange(start_time);
+      return true;
+    }
+
+    const Itr itr = --Itr(range_map_.upper_bound(start_time));
+    DCHECK(start_time >= itr->first);
+
+    Range& range = itr->second;
+
+    if (start_time > range.last_time) {
+      NewRange(start_time);
+      return true;
+    }
+
+    if (start_time < range.last_time)
+      range.count = 0;
+    else
+      range.count = 1;
+
+    curr_range_itr_ = itr;
+    return false;
+  }
+
+  DCHECK(start_time >= curr_range_itr_->first);
+
+  Range& curr_range = curr_range_itr_->second;
+
+  if (start_time < curr_range.last_time) {

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

Please put the logic on lines 61 - 74 in a method on Range. The multiple
references to curr_range imply that this really should be a method.

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+    DCHECK_EQ(curr_range.count, 0);
+    return false;
+  }
+
+  if (start_time == curr_range.last_time) {
+    ++curr_range.count;
+
+    if (curr_range.count <= curr_range.max_count)
+      return false;
+
+    ++curr_range.max_count;
+    return true;
+  }
+
+  const Itr next_range_itr = ++Itr(curr_range_itr_);
+
+  if (next_range_itr != range_map_.end()) {
+    DCHECK(next_range_itr->first > curr_range.last_time);
+    DCHECK(start_time <= next_range_itr->first);
+
+    Range& next_range = next_range_itr->second;
+
+    if (start_time == next_range_itr->first) {
+      // We have walked off the curr range, and onto the next one.
+      // There is now no ambiguity about where the curr time range
+      // ends, and so we coalesce the curr and next ranges.
+
+      if (start_time < next_range.last_time)
+        next_range.count = 0;

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: This logic is duplicated. It looks like a ResetCount(start_time) should be
added to Range

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+      else
+        next_range.count = 1;
+
+      curr_range = next_range;

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: I think hiding the logic in this block in a void Merge(const Range&) method
on Range would make this cleaner and remove the need for the next_range
temporary.

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+      range_map_.erase(next_range_itr);
+
+      return false;
+    }
+  }
+
+  // Either curr is the last range in the map, or there is a next
+  // range beyond the curr range, but its start time is ahead of this
+  // cue's start time.  In either case, this cue becomes the new
+  // last_time for the curr range.  Eventually we will see a cue whose
+  // time matches the start time of the next range, in which case we
+  // coalesce the curr and next ranges.
+
+  curr_range.last_time = start_time;

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: Place these assignments in an SetLastTime() method on Range.

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+  curr_range.count = 1;
+  curr_range.max_count = 1;
+
+  return true;
+}
+
+void TextRanges::NewRange(base::TimeDelta start_time) {
+  Range range;
+  range.last_time = start_time;

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: Use the SetLastTime() method mentioned above here to avoid duplication.

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+  range.count = 1;
+  range.max_count = 1;
+
+  typedef RangeMap::iterator Itr;

[acolwell GONE FROM CHROMIUM, 2014/02/05 19:09:36]

nit: Please remove the typdefs and inline since this is only used once and the
inline is more compact.

[Matthew Heaney (Chromium), 2014/02/06 02:25:48]

Done.

+  typedef std::pair<Itr, bool> Result;
+
+  Result result = range_map_.insert(std::make_pair(start_time, range));
+  DCHECK(result.second);
+
+  curr_range_itr_ = result.first;
+}
+
+int TextRanges::Present(base::TimeDelta start_time) const {
+  if (curr_range_itr_ == range_map_.end())
+    return -1;
+
+  if (start_time < curr_range_itr_->first)
+    return 0;
+
+  const Range& r = curr_range_itr_->second;
+
+  if (start_time <= r.last_time)
+    return 1;
+
+  return 0;
+}
+
+}  // namespace media