Fix TextEngine buffered range calculations

This reverts commit c38d4dd, which
actually broke text range calculations in v2.3.10, and v2.4.2-v2.4.4.
The original commit was meant to account for the period start, but
resulted in a double-accounting of presentationTimeOffset.

The start and ends times passed into TextEngine's appendBuffer were
period-relative, so timestampOffset had already been applied.  To
avoid further confusion and to fix the original issue the reverted
commit tried to address, these have been changed to
presentation-relative timestamps.  Now the period start and all
offsets have been accounted for before the metadata reaches
MediaSourceEngine and TextEngine.

The tests added in the bad commit have been modified to test for the
opposite: that we do not erroneously account for timestamp offset when
calculating the buffered ranges for text.

Backported to v2.4.x

Closes #1562

Change-Id: I9fa7a3f59906c4f3e623f411e48551f86f5c2ff7

externs/shaka/text.js

@@ -127,15 +127,17 @@ shakaExtern.Cue = function() {};
 
 
 /**
- * The start time of the cue in seconds and fractions of a second.
+ * The start time of the cue in seconds, relative to the start of the
+ * presentation.
  * @type {number}
  * @exportDoc
  */
 shakaExtern.Cue.prototype.startTime;
 
 
 /**
- * The end time of the cue in seconds and fractions of a second.
+ * The end time of the cue in seconds, relative to the start of the
+ * presentation.
  * @type {number}
  * @exportDoc
  */
@@ -417,13 +419,13 @@ shakaExtern.TextDisplayer.prototype.append = function(cues) {};
 /**
  * Remove cues in a given time range.
  *
- * @param {number} start
- * @param {number} end
+ * @param {number} startTime relative to the start of the presentation
+ * @param {number} endTime relative to the start of the presentation
  * @return {boolean}
  *
  * @exportDoc
  */
-shakaExtern.TextDisplayer.prototype.remove = function(start, end) {};
+shakaExtern.TextDisplayer.prototype.remove = function(startTime, endTime) {};
 
 
 /**

lib/media/media_source_engine.js

@@ -487,8 +487,8 @@ shaka.media.MediaSourceEngine.prototype.getBuffered_ = function(contentType) {
  *
  * @param {shaka.util.ManifestParserUtils.ContentType} contentType
  * @param {!ArrayBuffer} data
- * @param {?number} startTime
- * @param {?number} endTime
+ * @param {?number} startTime relative to the start of the presentation
+ * @param {?number} endTime relative to the start of the presentation
  * @return {!Promise}
  */
 shaka.media.MediaSourceEngine.prototype.appendBuffer =
@@ -548,8 +548,8 @@ shaka.media.MediaSourceEngine.prototype.getUseEmbeddedText = function() {
  * Enqueue an operation to remove data from the SourceBuffer.
  *
  * @param {shaka.util.ManifestParserUtils.ContentType} contentType
- * @param {number} startTime
- * @param {number} endTime
+ * @param {number} startTime relative to the start of the presentation
+ * @param {number} endTime relative to the start of the presentation
  * @return {!Promise}
  */
 shaka.media.MediaSourceEngine.prototype.remove =
@@ -722,8 +722,8 @@ shaka.media.MediaSourceEngine.prototype.append_ =
 /**
  * Remove data from the SourceBuffer.
  * @param {shaka.util.ManifestParserUtils.ContentType} contentType
- * @param {number} startTime
- * @param {number} endTime
+ * @param {number} startTime relative to the start of the presentation
+ * @param {number} endTime relative to the start of the presentation
  * @private
  */
 shaka.media.MediaSourceEngine.prototype.remove_ =

lib/media/streaming_engine.js

@@ -1744,8 +1744,13 @@ shaka.media.StreamingEngine.prototype.append_ = function(
     if (this.destroyed_) return;
     shaka.log.v1(logPrefix, 'appending media segment');
 
+    // MediaSourceEngine expects times relative to the start of the
+    // presentation.  Reference times are relative to the start of the period.
+    const startTime = reference.startTime + period.startTime;
+    const endTime = reference.endTime + period.startTime;
+
     return this.playerInterface_.mediaSourceEngine.appendBuffer(
-        mediaState.type, segment, reference.startTime, reference.endTime);
+        mediaState.type, segment, startTime, endTime);
   }.bind(this)).then(function() {
     if (this.destroyed_) return;
     shaka.log.v2(logPrefix, 'appended media segment');

lib/text/text_engine.js

@@ -155,8 +155,8 @@ shaka.text.TextEngine.prototype.getStartTime = function(buffer) {
 
 /**
  * @param {!ArrayBuffer} buffer
- * @param {?number} startTime
- * @param {?number} endTime
+ * @param {?number} startTime relative to the start of the presentation
+ * @param {?number} endTime relative to the start of the presentation
  * @return {!Promise}
  */
 shaka.text.TextEngine.prototype.appendBuffer =
@@ -176,8 +176,8 @@ shaka.text.TextEngine.prototype.appendBuffer =
     /** @type {shakaExtern.TextParser.TimeContext} **/
     let time = {
       periodStart: this.timestampOffset_,
-      segmentStart: this.timestampOffset_ + startTime,
-      segmentEnd: this.timestampOffset_ + endTime
+      segmentStart: startTime,
+      segmentEnd: endTime,
     };
 
     // Parse the buffer and add the new cues.
@@ -194,48 +194,49 @@ shaka.text.TextEngine.prototype.appendBuffer =
     // parsed cues.  This is important because some segments may contain no
     // cues, but we must still consider those ranges buffered.
     if (this.bufferStart_ == null) {
-      this.bufferStart_ =
-          Math.max(startTime + this.timestampOffset_, this.appendWindowStart_);
+      this.bufferStart_ = Math.max(startTime, this.appendWindowStart_);
     } else {
       // We already had something in buffer, and we assume we are extending the
       // range from the end.
       goog.asserts.assert((startTime - this.bufferEnd_) <= 1,
                           'There should not be a gap in text references >1s');
     }
-    this.bufferEnd_ =
-        Math.min(endTime + this.timestampOffset_, this.appendWindowEnd_);
+    this.bufferEnd_ = Math.min(endTime, this.appendWindowEnd_);
   }.bind(this));
 };
 
 
 /**
- * @param {number} start
- * @param {number} end
+ * @param {number} startTime relative to the start of the presentation
+ * @param {number} endTime relative to the start of the presentation
  * @return {!Promise}
  */
-shaka.text.TextEngine.prototype.remove = function(start, end) {
+shaka.text.TextEngine.prototype.remove = function(startTime, endTime) {
   // Start the operation asynchronously to avoid blocking the caller.
   return Promise.resolve().then(function() {
-    if (this.displayer_ && this.displayer_.remove(start, end)) {
+    if (this.displayer_ && this.displayer_.remove(startTime, endTime)) {
       if (this.bufferStart_ == null) {
         goog.asserts.assert(this.bufferEnd_ == null,
-                            'end must be null if start is null');
+                            'end must be null if startTime is null');
       } else {
         goog.asserts.assert(this.bufferEnd_ != null,
-                            'end must be non-null if start is non-null');
+                            'end must be non-null if startTime is non-null');
 
         // Update buffered range.
-        if (end <= this.bufferStart_ || start >= this.bufferEnd_) {
+        if (endTime <= this.bufferStart_ || startTime >= this.bufferEnd_) {
           // No intersection.  Nothing was removed.
-        } else if (start <= this.bufferStart_ && end >= this.bufferEnd_) {
+        } else if (startTime <= this.bufferStart_ &&
+                   endTime >= this.bufferEnd_) {
           // We wiped out everything.
           this.bufferStart_ = this.bufferEnd_ = null;
-        } else if (start <= this.bufferStart_ && end < this.bufferEnd_) {
+        } else if (startTime <= this.bufferStart_ &&
+                   endTime < this.bufferEnd_) {
           // We removed from the beginning of the range.
-          this.bufferStart_ = end;
-        } else if (start > this.bufferStart_ && end >= this.bufferEnd_) {
+          this.bufferStart_ = endTime;
+        } else if (startTime > this.bufferStart_ &&
+                   endTime >= this.bufferEnd_) {
           // We removed from the end of the range.
-          this.bufferEnd_ = start;
+          this.bufferEnd_ = startTime;
         } else {
           // We removed from the middle?  StreamingEngine isn't supposed to.
           goog.asserts.assert(

test/test/util/fake_media_source_engine.js

@@ -304,8 +304,12 @@ shaka.test.FakeMediaSourceEngine.prototype.appendBufferImpl = function(
     throw new Error('unexpected data');
   }
 
-  expect(startTime).toBe(this.segmentData[type].segmentStartTimes[i]);
-  expect(endTime).toBe(startTime + this.segmentData[type].segmentDuration);
+  const segmentData = this.segmentData[type];
+  const expectedStartTime =
+      segmentData.segmentPeriodTimes[i] + segmentData.segmentStartTimes[i];
+  const expectedEndTime = expectedStartTime + segmentData.segmentDuration;
+  expect(startTime).toBe(expectedStartTime);
+  expect(endTime).toBe(expectedEndTime);
 
   // Verify that the segment is aligned.
   let start = this.segmentData[type].segmentStartTimes[i] +

test/text/text_engine_unit.js

@@ -166,7 +166,7 @@ describe('TextEngine', function() {
 
         mockDisplayer.append.calls.reset();
         textEngine.setTimestampOffset(4);
-        return textEngine.appendBuffer(dummyData, 0, 3);
+        return textEngine.appendBuffer(dummyData, 4, 7);
       }).then(function() {
         expect(mockParseMedia).toHaveBeenCalledWith(
             new Uint8Array(dummyData),
@@ -240,15 +240,18 @@ describe('TextEngine', function() {
       }).catch(fail).then(done);
     });
 
-    it('handles timestamp offset', async function() {
+    it('does not use timestamp offset', async function() {
+      // The start and end times passed to appendBuffer are now absolute, so
+      // they already account for timestampOffset and period offset.
+      // See https://github.com/google/shaka-player/issues/1562
       textEngine.setTimestampOffset(60);
       await textEngine.appendBuffer(dummyData, 0, 3);
-      expect(textEngine.bufferStart()).toBe(60);
-      expect(textEngine.bufferEnd()).toBe(63);
+      expect(textEngine.bufferStart()).toBe(0);
+      expect(textEngine.bufferEnd()).toBe(3);
 
       await textEngine.appendBuffer(dummyData, 3, 6);
-      expect(textEngine.bufferStart()).toBe(60);
-      expect(textEngine.bufferEnd()).toBe(66);
+      expect(textEngine.bufferStart()).toBe(0);
+      expect(textEngine.bufferEnd()).toBe(6);
     });
   });
 
@@ -283,10 +286,14 @@ describe('TextEngine', function() {
       }).catch(fail).then(done);
     });
 
-    it('handles timestamp offset', async function() {
+    it('does not use timestamp offset', async function() {
+      // The start and end times passed to appendBuffer are now absolute, so
+      // they already account for timestampOffset and period offset.
+      // See https://github.com/google/shaka-player/issues/1562
       textEngine.setTimestampOffset(60);
       await textEngine.appendBuffer(dummyData, 3, 6);
-      expect(textEngine.bufferedAheadOf(64)).toBe(2);
+      expect(textEngine.bufferedAheadOf(4)).toBe(2);
+      expect(textEngine.bufferedAheadOf(64)).toBe(0);
     });
   });