Bug 1594067 - Fix RemoveTrailing to handle correctly keep frames equals to zero. r=padenot
authorAlex Chronopoulos <achronop@gmail.com>
Thu, 07 Nov 2019 09:58:27 +0000
changeset 501123 f331f382d86bc23f85029c7380b938cc67010a3a
parent 501122 cb5434e79ec6a188207818423e92485ff84d9c23
child 501124 01eb66307875ee964208ed951ee995b74f01ab9b
push id114168
push userdluca@mozilla.com
push dateSun, 10 Nov 2019 03:08:55 +0000
treeherdermozilla-inbound@33f64c1ef3e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspadenot
bugs1594067
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1594067 - Fix RemoveTrailing to handle correctly keep frames equals to zero. r=padenot Fix `MediaSegmentBase::RemoveTrailing` to be able to accept the first argument, keep frames, equal to zero. The patch avoids calling the `AudioChunk::SliceTo()` method with zero slice duration which hits an assert. The crash was being triggered when in the AudioSegment was including one or more chunks, with the first chunk containing silence (null). Then the `AudioSegment::FlushAfter` had to be called with a duration smaller or equal to the duration of the first chunk. A unit test has been created, verifying the duration of the final segment. Differential Revision: https://phabricator.services.mozilla.com/D52092
dom/media/MediaSegment.h
dom/media/gtest/TestAudioSegment.cpp
--- a/dom/media/MediaSegment.h
+++ b/dom/media/MediaSegment.h
@@ -446,29 +446,27 @@ class MediaSegmentBase : public MediaSeg
     MOZ_ASSERT(mChunks.Capacity() >= DEFAULT_SEGMENT_CAPACITY,
                "Capacity must be retained after removing chunks");
   }
 
   void RemoveTrailing(TrackTime aKeep, uint32_t aStartIndex) {
     NS_ASSERTION(aKeep >= 0, "Can't keep negative duration");
     TrackTime t = aKeep;
     uint32_t i;
-    for (i = aStartIndex; i < mChunks.Length(); ++i) {
+    for (i = aStartIndex; i < mChunks.Length() && t; ++i) {
       Chunk* c = &mChunks[i];
       if (c->GetDuration() > t) {
         c->SliceTo(0, t);
         break;
       }
       t -= c->GetDuration();
-      if (t == 0) {
-        break;
-      }
     }
-    if (i + 1 < mChunks.Length()) {
-      mChunks.RemoveElementsAt(i + 1, mChunks.Length() - (i + 1));
+    // At this point `i` is already advanced due to last check in the loop.
+    if (i < mChunks.Length()) {
+      mChunks.RemoveElementsAt(i, mChunks.Length() - i);
     }
     MOZ_ASSERT(mChunks.Capacity() >= DEFAULT_SEGMENT_CAPACITY,
                "Capacity must be retained after removing chunks");
     // Caller must adjust mDuration
   }
 
   AutoTArray<Chunk, DEFAULT_SEGMENT_CAPACITY> mChunks;
 };
--- a/dom/media/gtest/TestAudioSegment.cpp
+++ b/dom/media/gtest/TestAudioSegment.cpp
@@ -258,20 +258,46 @@ void fillChunkWithStereo(AudioChunk* c, 
 
   c->mChannelData.SetLength(2);
   c->mChannelData[0] = ch1;
   c->mChannelData[1] = ch2;
 
   c->mBufferFormat = AUDIO_FORMAT_FLOAT32;
 }
 
-TEST(AudioSegment, FlushAfterZero)
+TEST(AudioSegment, FlushAfter_ZeroDuration)
 {
   AudioChunk c;
   fillChunkWithStereo<float>(&c, 10);
 
   AudioSegment s;
   s.AppendAndConsumeChunk(&c);
   s.FlushAfter(0);
   EXPECT_EQ(s.GetDuration(), 0);
 }
 
+TEST(AudioSegment, FlushAfter_SmallerDuration)
+{
+  // It was crashing when the first chunk was silence (null) and FlushAfter
+  // was called for a duration, smaller or equal to the duration of the
+  // first chunk.
+  TrackTime duration = 10;
+  TrackTime smaller_duration = 8;
+  AudioChunk c1;
+  c1.SetNull(duration);
+  AudioChunk c2;
+  fillChunkWithStereo<float>(&c2, duration);
+
+  AudioSegment s;
+  s.AppendAndConsumeChunk(&c1);
+  s.AppendAndConsumeChunk(&c2);
+  s.FlushAfter(smaller_duration);
+  EXPECT_EQ(s.GetDuration(), smaller_duration) << "Check new duration";
+
+  TrackTime chunkByChunkDuration = 0;
+  for (AudioSegment::ChunkIterator iter(s); !iter.IsEnded(); iter.Next()) {
+    chunkByChunkDuration += iter->GetDuration();
+  }
+  EXPECT_EQ(s.GetDuration(), chunkByChunkDuration)
+      << "Confirm duration chunk by chunk";
+}
+
 }  // namespace audio_segment