Backed out 4 changesets (bug 1500713) on request of jcristau for causing Bug 1535603 a=Backout
authorarthur.iakab <aiakab@mozilla.com>
Fri, 15 Mar 2019 15:50:20 +0200
changeset 525065 5edbe9b1b82220b07ff446bd4a3b9ff2f7dda508
parent 525014 4d62ab0e31fd6918ca95763914a8bdf41afe757a
child 525070 79995994a23fe694827bbbc3ec3f7b7819c8c6f2
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersBackout
bugs1500713, 1535603
milestone67.0a1
backs out5d0cad2c99a4060d888e9fca5b8bfb1b42cc4e2b
f96a12eedd89d22b69e98f4a6c0762d90249540f
2633cea7d11903a6d29f260ed133fd5ebfb3597a
49c4cfbf2beee350e31e4e43e2314735ab6fbc6b
first release with
nightly linux32
5edbe9b1b822 / 67.0a1 / 20190315135105 / files
nightly linux64
5edbe9b1b822 / 67.0a1 / 20190315135105 / files
nightly mac
5edbe9b1b822 / 67.0a1 / 20190315135105 / files
nightly win32
5edbe9b1b822 / 67.0a1 / 20190315135105 / files
nightly win64
5edbe9b1b822 / 67.0a1 / 20190315135105 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out 4 changesets (bug 1500713) on request of jcristau for causing Bug 1535603 a=Backout Backed out changeset 5d0cad2c99a4 (bug 1500713) Backed out changeset f96a12eedd89 (bug 1500713) Backed out changeset 2633cea7d119 (bug 1500713) Backed out changeset 49c4cfbf2bee (bug 1500713)
dom/media/gtest/TestBufferReader.cpp
dom/media/gtest/TestMP3Demuxer.cpp
dom/media/gtest/moz.build
dom/media/gtest/test_vbri.mp3
dom/media/mp3/MP3Demuxer.cpp
dom/media/mp3/MP3Demuxer.h
dom/media/mp3/MP3FrameParser.cpp
deleted file mode 100644
--- a/dom/media/gtest/TestBufferReader.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-#include "gtest/gtest.h"
-#include "BufferReader.h"
-
-TEST(BufferReader, ReaderCursor) {
-  // Allocate a buffer and create a BufferReader.
-  const size_t BUFFER_SIZE = 10;
-  uint8_t buffer[BUFFER_SIZE] = {0};
-
-  const uint8_t* const HEAD = reinterpret_cast<uint8_t*>(buffer);
-  const uint8_t* const TAIL = HEAD + BUFFER_SIZE;
-
-  BufferReader reader(HEAD, BUFFER_SIZE);
-  ASSERT_EQ(reader.Offset(), static_cast<size_t>(0));
-  ASSERT_EQ(reader.Peek(BUFFER_SIZE), HEAD);
-
-  // Keep reading to the end, and make sure the final read failed.
-  const size_t READ_SIZE = 4;
-  ASSERT_NE(BUFFER_SIZE % READ_SIZE, static_cast<size_t>(0));
-  for (const uint8_t* ptr = reader.Peek(0); ptr != nullptr;
-       ptr = reader.Read(READ_SIZE))
-    ;
-
-  // Check the reading cursor of the BufferReader is correct
-  // after reading and seeking.
-  const uint8_t* tail = reader.Peek(0);
-  const uint8_t* head = reader.Seek(0);
-
-  EXPECT_EQ(head, HEAD);
-  EXPECT_EQ(tail, TAIL);
-}
\ No newline at end of file
--- a/dom/media/gtest/TestMP3Demuxer.cpp
+++ b/dom/media/gtest/TestMP3Demuxer.cpp
@@ -42,21 +42,18 @@ class MockMP3StreamMediaResource
 
   int64_t GetLength() override { return -1; }
 
  protected:
   virtual ~MockMP3StreamMediaResource() {}
 };
 
 struct MP3Resource {
-  enum HeaderType { NONE, XING, VBRI };
-
   const char* mFilePath;
   bool mIsVBR;
-  HeaderType mHeaderType;
   int64_t mFileSize;
   int32_t mMPEGLayer;
   int32_t mMPEGVersion;
   uint8_t mID3MajorVersion;
   uint8_t mID3MinorVersion;
   uint8_t mID3Flags;
   uint32_t mID3Size;
 
@@ -81,17 +78,16 @@ struct MP3Resource {
 
 class MP3DemuxerTest : public ::testing::Test {
  protected:
   void SetUp() override {
     {
       MP3Resource res;
       res.mFilePath = "noise.mp3";
       res.mIsVBR = false;
-      res.mHeaderType = MP3Resource::NONE;
       res.mFileSize = 965257;
       res.mMPEGLayer = 3;
       res.mMPEGVersion = 1;
       res.mID3MajorVersion = 3;
       res.mID3MinorVersion = 0;
       res.mID3Flags = 0;
       res.mID3Size = 2141;
       res.mDuration = 30067000;
@@ -126,17 +122,16 @@ class MP3DemuxerTest : public ::testing:
       MP3Resource res;
       // This file trips up the MP3 demuxer if ID3v2 tags aren't properly
       // skipped. If skipping is not properly implemented, depending on the
       // strictness of the MPEG frame parser a false sync will be detected
       // somewhere within the metadata at or after 112087, or failing that, at
       // the artificially added extraneous header at 114532.
       res.mFilePath = "id3v2header.mp3";
       res.mIsVBR = false;
-      res.mHeaderType = MP3Resource::NONE;
       res.mFileSize = 191302;
       res.mMPEGLayer = 3;
       res.mMPEGVersion = 1;
       res.mID3MajorVersion = 3;
       res.mID3MinorVersion = 0;
       res.mID3Flags = 0;
       res.mID3Size = 115304;
       res.mDuration = 3166167;
@@ -166,17 +161,16 @@ class MP3DemuxerTest : public ::testing:
       streamRes.mDemuxer = new MP3TrackDemuxer(streamRes.mResource);
       mTargets.push_back(streamRes);
     }
 
     {
       MP3Resource res;
       res.mFilePath = "noise_vbr.mp3";
       res.mIsVBR = true;
-      res.mHeaderType = MP3Resource::XING;
       res.mFileSize = 583679;
       res.mMPEGLayer = 3;
       res.mMPEGVersion = 1;
       res.mID3MajorVersion = 3;
       res.mID3MinorVersion = 0;
       res.mID3Flags = 0;
       res.mID3Size = 2221;
       res.mDuration = 30081000;
@@ -205,17 +199,16 @@ class MP3DemuxerTest : public ::testing:
       streamRes.mDemuxer = new MP3TrackDemuxer(streamRes.mResource);
       mTargets.push_back(streamRes);
     }
 
     {
       MP3Resource res;
       res.mFilePath = "small-shot.mp3";
       res.mIsVBR = true;
-      res.mHeaderType = MP3Resource::XING;
       res.mFileSize = 6825;
       res.mMPEGLayer = 3;
       res.mMPEGVersion = 1;
       res.mID3MajorVersion = 4;
       res.mID3MinorVersion = 0;
       res.mID3Flags = 0;
       res.mID3Size = 24;
       res.mDuration = 336686;
@@ -246,17 +239,16 @@ class MP3DemuxerTest : public ::testing:
     }
 
     {
       MP3Resource res;
       // This file contains a false frame sync at 34, just after the ID3 tag,
       // which should be identified as a false positive and skipped.
       res.mFilePath = "small-shot-false-positive.mp3";
       res.mIsVBR = true;
-      res.mHeaderType = MP3Resource::XING;
       res.mFileSize = 6845;
       res.mMPEGLayer = 3;
       res.mMPEGVersion = 1;
       res.mID3MajorVersion = 4;
       res.mID3MinorVersion = 0;
       res.mID3Flags = 0;
       res.mID3Size = 24;
       res.mDuration = 336686;
@@ -285,17 +277,16 @@ class MP3DemuxerTest : public ::testing:
       streamRes.mDemuxer = new MP3TrackDemuxer(streamRes.mResource);
       mTargets.push_back(streamRes);
     }
 
     {
       MP3Resource res;
       res.mFilePath = "small-shot-partial-xing.mp3";
       res.mIsVBR = true;
-      res.mHeaderType = MP3Resource::XING;
       res.mFileSize = 6825;
       res.mMPEGLayer = 3;
       res.mMPEGVersion = 1;
       res.mID3MajorVersion = 4;
       res.mID3MinorVersion = 0;
       res.mID3Flags = 0;
       res.mID3Size = 24;
       res.mDuration = 336686;
@@ -320,55 +311,16 @@ class MP3DemuxerTest : public ::testing:
       res.mDemuxer = new MP3TrackDemuxer(res.mResource);
       mTargets.push_back(res);
 
       streamRes.mResource = new MockMP3StreamMediaResource(streamRes.mFilePath);
       streamRes.mDemuxer = new MP3TrackDemuxer(streamRes.mResource);
       mTargets.push_back(streamRes);
     }
 
-    {
-      MP3Resource res;
-      res.mFilePath = "test_vbri.mp3";
-      res.mIsVBR = true;
-      res.mHeaderType = MP3Resource::VBRI;
-      res.mFileSize = 16519;
-      res.mMPEGLayer = 3;
-      res.mMPEGVersion = 1;
-      res.mID3MajorVersion = 3;
-      res.mID3MinorVersion = 0;
-      res.mID3Flags = 0;
-      res.mID3Size = 4202;
-      res.mDuration = 783660;
-      res.mDurationError = 0.01f;
-      res.mSeekError = 0.02f;
-      res.mSampleRate = 44100;
-      res.mSamplesPerFrame = 1152;
-      res.mNumSamples = 29;
-      res.mNumTrailingFrames = 0;
-      res.mBitrate = 0;
-      res.mSlotSize = 1;
-      res.mPrivate = 0;
-      const int syncs[] = {4212, 4734, 5047, 5464, 5986, 6403};
-      res.mSyncOffsets.insert(res.mSyncOffsets.begin(), syncs, syncs + 6);
-
-      // VBR stream resources contain header info on total frames numbers, which
-      // is used to estimate the total duration.
-      MP3Resource streamRes = res;
-      streamRes.mFileSize = -1;
-
-      res.mResource = new MockMP3MediaResource(res.mFilePath);
-      res.mDemuxer = new MP3TrackDemuxer(res.mResource);
-      mTargets.push_back(res);
-
-      streamRes.mResource = new MockMP3StreamMediaResource(streamRes.mFilePath);
-      streamRes.mDemuxer = new MP3TrackDemuxer(streamRes.mResource);
-      mTargets.push_back(streamRes);
-    }
-
     for (auto& target : mTargets) {
       ASSERT_EQ(NS_OK, target.mResource->Open());
       ASSERT_TRUE(target.mDemuxer->Init());
     }
   }
 
   std::vector<MP3Resource> mTargets;
 };
@@ -390,25 +342,22 @@ TEST_F(MP3DemuxerTest, ID3Tags) {
 
 TEST_F(MP3DemuxerTest, VBRHeader) {
   for (const auto& target : mTargets) {
     RefPtr<MediaRawData> frame(target.mDemuxer->DemuxSample());
     ASSERT_TRUE(frame);
 
     const auto& vbr = target.mDemuxer->VBRInfo();
 
-    if (target.mHeaderType == MP3Resource::XING) {
+    if (target.mIsVBR) {
       EXPECT_EQ(FrameParser::VBRHeader::XING, vbr.Type());
       // TODO: find reference number which accounts for trailing headers.
       // EXPECT_EQ(target.mNumSamples / target.mSamplesPerFrame,
       // vbr.NumAudioFrames().value());
-    } else if (target.mHeaderType == MP3Resource::VBRI) {
-      EXPECT_TRUE(target.mIsVBR);
-      EXPECT_EQ(FrameParser::VBRHeader::VBRI, vbr.Type());
-    } else {  // MP3Resource::NONE
+    } else {
       EXPECT_EQ(FrameParser::VBRHeader::NONE, vbr.Type());
       EXPECT_FALSE(vbr.NumAudioFrames());
     }
   }
 }
 
 TEST_F(MP3DemuxerTest, FrameParsing) {
   for (const auto& target : mTargets) {
--- a/dom/media/gtest/moz.build
+++ b/dom/media/gtest/moz.build
@@ -18,17 +18,16 @@ UNIFIED_SOURCES += [
     'TestAudioBuffers.cpp',
     'TestAudioCompactor.cpp',
     'TestAudioMixer.cpp',
     'TestAudioPacketizer.cpp',
     'TestAudioSegment.cpp',
     'TestAudioTrackEncoder.cpp',
     'TestBitWriter.cpp',
     'TestBlankVideoDataCreator.cpp',
-    'TestBufferReader.cpp',
     'TestCDMStorage.cpp',
     'TestDataMutex.cpp',
     'TestGMPCrossOrigin.cpp',
     'TestGMPRemoveAndDelete.cpp',
     'TestGMPUtils.cpp',
     'TestGroupId.cpp',
     'TestIntervalSet.cpp',
     'TestMediaDataDecoder.cpp',
@@ -69,17 +68,16 @@ TEST_HARNESS_FILES.gtest += [
     'short-zero-inband.mov',
     'small-shot-false-positive.mp3',
     'small-shot-partial-xing.mp3',
     'small-shot.mp3',
     'test.webm',
     'test_case_1224361.vp8.ivf',
     'test_case_1224363.vp8.ivf',
     'test_case_1224369.vp8.ivf',
-    'test_vbri.mp3',
 ]
 
 TEST_DIRS += [
     'mp4_demuxer',
 ]
 
 include('/ipc/chromium/chromium-config.mozbuild')
 
deleted file mode 100644
index efd74503385c9d8c6afaf59a8923cf67ebd71805..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
GIT binary patch
literal 0
Hc$@<O00001
--- a/dom/media/mp3/MP3Demuxer.cpp
+++ b/dom/media/mp3/MP3Demuxer.cpp
@@ -292,59 +292,54 @@ MP3TrackDemuxer::SkipToNextRandomAccessP
   return SkipAccessPointPromise::CreateAndReject(
       SkipFailureHolder(NS_ERROR_DOM_MEDIA_DEMUXER_ERR, 0), __func__);
 }
 
 int64_t MP3TrackDemuxer::GetResourceOffset() const { return mOffset; }
 
 TimeIntervals MP3TrackDemuxer::GetBuffered() {
   AutoPinned<MediaResource> stream(mSource.GetResource());
-  TimeIntervals duration;
-  duration += TimeInterval(TimeUnit(), Duration());
+  TimeIntervals buffered;
 
   if (Duration() > TimeUnit() && stream->IsDataCachedToEndOfResource(0)) {
     // Special case completely cached files. This also handles local files.
+    buffered += TimeInterval(TimeUnit(), Duration());
     MP3LOGV("buffered = [[%" PRId64 ", %" PRId64 "]]",
             TimeUnit().ToMicroseconds(), Duration().ToMicroseconds());
-    return duration;
+    return buffered;
   }
 
-  TimeIntervals buffered;
   MediaByteRangeSet ranges;
   nsresult rv = stream->GetCachedRanges(ranges);
   NS_ENSURE_SUCCESS(rv, buffered);
 
   for (const auto& range : ranges) {
     if (range.IsEmpty()) {
       continue;
     }
     TimeUnit start = Duration(FrameIndexFromOffset(range.mStart));
     TimeUnit end = Duration(FrameIndexFromOffset(range.mEnd));
     MP3LOGV("buffered += [%" PRId64 ", %" PRId64 "]", start.ToMicroseconds(),
             end.ToMicroseconds());
     buffered += TimeInterval(start, end);
   }
 
-  // If the number of frames presented in header is valid, the duration
-  // calculated from it should be the maximal duration.
-  return ValidNumAudioFrames().isSome() && buffered.GetEnd() > duration.GetEnd()
-             ? duration
-             : buffered;
+  return buffered;
 }
 
 int64_t MP3TrackDemuxer::StreamLength() const { return mSource.GetLength(); }
 
 TimeUnit MP3TrackDemuxer::Duration() const {
   if (!mNumParsedFrames) {
     return TimeUnit::FromMicroseconds(-1);
   }
 
   int64_t numFrames = 0;
-  const auto numAudioFrames = ValidNumAudioFrames();
-  if (numAudioFrames.isSome()) {
+  const auto numAudioFrames = mParser.VBRInfo().NumAudioFrames();
+  if (mParser.VBRInfo().IsValid() && numAudioFrames.valueOr(0) + 1 > 1) {
     // VBR headers don't include the VBR header frame.
     numFrames = numAudioFrames.value() + 1;
     return Duration(numFrames);
   }
 
   const int64_t streamLen = StreamLength();
   if (streamLen < 0) {  // Live streams.
     // Unknown length, we can't estimate duration.
@@ -728,19 +723,12 @@ double MP3TrackDemuxer::AverageFrameLeng
   const auto& vbr = mParser.VBRInfo();
   if (vbr.IsComplete() && vbr.NumAudioFrames().value() + 1) {
     return static_cast<double>(vbr.NumBytes().value()) /
            (vbr.NumAudioFrames().value() + 1);
   }
   return 0.0;
 }
 
-Maybe<uint32_t> MP3TrackDemuxer::ValidNumAudioFrames() const {
-  return mParser.VBRInfo().IsValid() &&
-                 mParser.VBRInfo().NumAudioFrames().valueOr(0) + 1 > 1
-             ? mParser.VBRInfo().NumAudioFrames()
-             : Maybe<uint32_t>();
-}
-
 }  // namespace mozilla
 
 #undef MP3LOG
 #undef MP3LOGV
--- a/dom/media/mp3/MP3Demuxer.h
+++ b/dom/media/mp3/MP3Demuxer.h
@@ -117,20 +117,16 @@ class MP3TrackDemuxer : public MediaTrac
 
   // Reads aSize bytes into aBuffer from the source starting at aOffset.
   // Returns the actual size read.
   int32_t Read(uint8_t* aBuffer, int64_t aOffset, int32_t aSize);
 
   // Returns the average frame length derived from the previously parsed frames.
   double AverageFrameLength() const;
 
-  // Returns the number of frames reported by the header if it's valid. Nothing
-  // otherwise.
-  Maybe<uint32_t> ValidNumAudioFrames() const;
-
   // The (hopefully) MPEG resource.
   MediaResourceIndex mSource;
 
   // MPEG frame parser used to detect frames and extract side info.
   FrameParser mParser;
 
   // Whether we've locked onto a valid sequence of frames or not.
   bool mFrameLock;
--- a/dom/media/mp3/MP3FrameParser.cpp
+++ b/dom/media/mp3/MP3FrameParser.cpp
@@ -8,17 +8,16 @@
 
 #include <algorithm>
 #include <inttypes.h>
 
 #include "mozilla/Assertions.h"
 #include "mozilla/EndianUtils.h"
 #include "mozilla/Pair.h"
 #include "mozilla/ResultExtensions.h"
-#include "mozilla/ScopeExit.h"
 #include "VideoUtils.h"
 
 extern mozilla::LazyLogModule gMediaDemuxerLog;
 #define MP3LOG(msg, ...) \
   MOZ_LOG(gMediaDemuxerLog, LogLevel::Debug, ("MP3Demuxer " msg, ##__VA_ARGS__))
 #define MP3LOGV(msg, ...)                      \
   MOZ_LOG(gMediaDemuxerLog, LogLevel::Verbose, \
           ("MP3Demuxer " msg, ##__VA_ARGS__))
@@ -78,17 +77,18 @@ Result<bool, nsresult> FrameParser::Pars
   MOZ_ASSERT(aReader && aBytesToSkip);
   *aBytesToSkip = 0;
 
   if (!mID3Parser.Header().Size() && !mFirstFrame.Length()) {
     // No MP3 frames have been parsed yet, look for ID3v2 headers at file begin.
     // ID3v1 tags may only be at file end.
     // TODO: should we try to read ID3 tags at end of file/mid-stream, too?
     const size_t prevReaderOffset = aReader->Offset();
-    const uint32_t tagSize = mID3Parser.Parse(aReader).unwrapOr(0);
+    uint32_t tagSize;
+    MOZ_TRY_VAR(tagSize, mID3Parser.Parse(aReader));
     if (!!tagSize) {
       // ID3 tag found, skip past it.
       const uint32_t skipSize = tagSize - ID3Parser::ID3Header::SIZE;
 
       if (skipSize > aReader->Remaining()) {
         // Skipping across the ID3v2 tag would take us past the end of the
         // buffer, therefore we return immediately and let the calling function
         // handle skipping the rest of the tag.
@@ -351,20 +351,17 @@ Result<bool, nsresult> FrameParser::VBRH
   enum Flags {
     NUM_FRAMES = 0x01,
     NUM_BYTES = 0x02,
     TOC = 0x04,
     VBR_SCALE = 0x08
   };
 
   MOZ_ASSERT(aReader);
-
-  // Seek backward to the original position before leaving this scope.
   const size_t prevReaderOffset = aReader->Offset();
-  auto scopeExit = MakeScopeExit([&] { aReader->Seek(prevReaderOffset); });
 
   // We have to search for the Xing header as its position can change.
   for (auto res = aReader->PeekU32();
        res.isOk() && res.unwrap() != XING_TAG && res.unwrap() != INFO_TAG;) {
     aReader->Read(1);
     res = aReader->PeekU32();
   }
 
@@ -400,16 +397,17 @@ Result<bool, nsresult> FrameParser::VBRH
     }
   }
   if (flags & VBR_SCALE) {
     uint32_t scale;
     MOZ_TRY_VAR(scale, aReader->ReadU32());
     mScale = Some(scale);
   }
 
+  aReader->Seek(prevReaderOffset);
   return mType == XING;
 }
 
 Result<bool, nsresult> FrameParser::VBRHeader::ParseVBRI(
     BufferReader* aReader) {
   static const uint32_t TAG = BigEndian::readUint32("VBRI");
   static const uint32_t OFFSET = 32 + FrameParser::FrameHeader::SIZE;
   static const uint32_t FRAME_COUNT_OFFSET = OFFSET + 14;
@@ -418,34 +416,33 @@ Result<bool, nsresult> FrameParser::VBRH
   MOZ_ASSERT(aReader);
   // ParseVBRI assumes that the ByteReader offset points to the beginning of a
   // frame, therefore as a simple check, we look for the presence of a frame
   // sync at that position.
   auto sync = aReader->PeekU16();
   if (sync.isOk()) {  // To avoid compiler complains 'set but unused'.
     MOZ_ASSERT((sync.unwrap() & 0xFFE0) == 0xFFE0);
   }
-
-  // Seek backward to the original position before leaving this scope.
   const size_t prevReaderOffset = aReader->Offset();
-  auto scopeExit = MakeScopeExit([&] { aReader->Seek(prevReaderOffset); });
 
   // VBRI have a fixed relative position, so let's check for it there.
   if (aReader->Remaining() > MIN_FRAME_SIZE) {
     aReader->Seek(prevReaderOffset + OFFSET);
     uint32_t tag, frames;
     MOZ_TRY_VAR(tag, aReader->ReadU32());
     if (tag == TAG) {
       aReader->Seek(prevReaderOffset + FRAME_COUNT_OFFSET);
       MOZ_TRY_VAR(frames, aReader->ReadU32());
       mNumAudioFrames = Some(frames);
       mType = VBRI;
+      aReader->Seek(prevReaderOffset);
       return true;
     }
   }
+  aReader->Seek(prevReaderOffset);
   return false;
 }
 
 bool FrameParser::VBRHeader::Parse(BufferReader* aReader) {
   auto res = MakePair(ParseVBRI(aReader), ParseXing(aReader));
   const bool rv = (res.first().isOk() && res.first().unwrap()) ||
                   (res.second().isOk() && res.second().unwrap());
   if (rv) {