Make the timestamp unroll offset consistent accross PES pids.

media/formats/mp2t/timestamp_unroller.cc

+// 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/formats/mp2t/timestamp_unroller.h"
+
+#include "base/logging.h"
+
+namespace media {
+namespace mp2t {
+
+TimestampUnroller::TimestampUnroller()
+    : is_previous_timestamp_valid_(false),
+      previous_unrolled_timestamp_(0) {
+}
+
+TimestampUnroller::~TimestampUnroller() {
+}
+
+int64 TimestampUnroller::GetUnrolledTimestamp(int64 timestamp) {
+  // Mpeg2 TS timestamps have an accuracy of 33 bits.
+  const int nbits = 33;
+
+  // |timestamp| has a precision of |nbits|
+  // so make sure the highest bits are set to 0.
+  DCHECK_EQ((timestamp >> nbits), 0);
+
+  if (!is_previous_timestamp_valid_) {
+    previous_unrolled_timestamp_ = timestamp;
+    is_previous_timestamp_valid_ = true;
+    return timestamp;
+  }
+
+  // Consider 3 possibilities to estimate the missing high bits of |time|.

[wolenetz, 2014/09/15 22:15:39]

nit: there is no local |time|. s/time/timestamp/ ?

[damienv1, 2014/09/15 22:27:36]

Done.

+  int64 previous_unrolled_time_high =
+      (previous_unrolled_timestamp_ >> nbits);
+  int64 time0 = ((previous_unrolled_time_high - 1) << nbits) | timestamp;

[wolenetz, 2014/09/15 22:15:39]

It looks like this could introduce a negative int64 result, if
previous_unrolled_time_high is 0, and time0 wins the diff'ing, below.

[damienv1, 2014/09/15 22:27:36]

Correct, there is nothing that prevents negativity at this level.
This module addresses only the continuity.

[wolenetz, 2014/09/15 23:24:36]

Acknowledged.

+  int64 time1 = ((previous_unrolled_time_high + 0) << nbits) | timestamp;
+  int64 time2 = ((previous_unrolled_time_high + 1) << nbits) | timestamp;

[wolenetz, 2014/09/15 22:15:39]

It seems overflow could occur here. Impact of implementation-specific
sign-extending on bit shifting needs at least comments in this code block to
help prove correctness.

[damienv1, 2014/09/15 22:27:36]

The underlying assumption is that 64 bit is enough to represent the timeline (an
int64 can represent several years of playback when timestamps are coded on the
90kHz scale).
I'll try to add more comments in the code.

[wolenetz, 2014/09/15 23:24:36]

Acknowledged.

+
+  // Select the min absolute difference with the current time
+  // so as to ensure time continuity.
+  int64 diff0 = time0 - previous_unrolled_timestamp_;
+  int64 diff1 = time1 - previous_unrolled_timestamp_;
+  int64 diff2 = time2 - previous_unrolled_timestamp_;
+  if (diff0 < 0)

[wolenetz, 2014/09/15 22:15:39]

nit: std::abs here and below?

[damienv1, 2014/09/15 22:27:36]

std::abs is defined only for int/float/double.
Only c++ 11 is guaranteed to have the templated version of std::abs

So I prefer to stick to the current approach.

[wolenetz, 2014/09/15 23:24:36]

Acknowledged.

+    diff0 = -diff0;
+  if (diff1 < 0)
+    diff1 = -diff1;
+  if (diff2 < 0)
+    diff2 = -diff2;
+
+  int64 unrolled_time;
+  int64 min_diff;
+  if (diff1 < diff0) {
+    unrolled_time = time1;
+    min_diff = diff1;
+  } else {
+    unrolled_time = time0;
+    min_diff = diff0;
+  }
+  if (diff2 < min_diff)
+    unrolled_time = time2;
+
+  // Update the state of the timestamp unroller.
+  previous_unrolled_timestamp_ = unrolled_time;
+
+  return unrolled_time;
+}
+
+void TimestampUnroller::Reset() {
+  is_previous_timestamp_valid_ = false;
+  previous_unrolled_timestamp_ = 0;
+}
+
+}  // namespace mp2t
+}  // namespace media