From f5e7f0f6356ae6d1703ff6b9eb00ac559adeadbb Mon Sep 17 00:00:00 2001 From: Jan Henning Date: Mon, 15 May 2017 22:49:40 +0200 Subject: [PATCH] Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state. r=esawin As long as we're undecided whether the data we're being fed is actually a valid MPEG stream, we only search a limited amount of data for a valid frame header before giving up. Once we have found a valid frame header however, we change our behavior and search until EOS for the next frame in order to cope with possibly corrupted streams. In practice it has turned out that the first MPEG frame we detect might in fact be a false positive. Therefore, we now turn any found frame into a candidate frame at first and only accept it as the real first frame if it is followed by at least two other matching frames in succession. As currently implemented, our MAX_SKIPPED_BYTES logic however doesn't know about this and turns itself off as soon as we've found anything that *looks* like a valid MP3 frame header. This means that if that frame was in fact a false positive, we can now go off on a wild goose chase through what might potentially be the *whole* file while looking for putative followup frames. To fix this, the parser now records not just whether it has found *any* frame, but also whether it has detected a *succession of valid frames* and then sets the amount of bytes to be searched for the next frame based on that state. MozReview-Commit-ID: 3WQTfLg88kV --HG-- extra : rebase_source : 2bd84568f1e138d1d9bafdab96ddae705dd0a286 --- dom/media/MP3Demuxer.cpp | 39 +++++++++++++++++++++++++++++++++++---- dom/media/MP3Demuxer.h | 3 +++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/dom/media/MP3Demuxer.cpp b/dom/media/MP3Demuxer.cpp index a2678edd1e2e..1067d4a3a672 100644 --- a/dom/media/MP3Demuxer.cpp +++ b/dom/media/MP3Demuxer.cpp @@ -8,6 +8,7 @@ #include #include +#include #include "mozilla/Assertions.h" #include "mozilla/EndianUtils.h" @@ -106,6 +107,7 @@ MP3Demuxer::NotifyDataRemoved() MP3TrackDemuxer::MP3TrackDemuxer(MediaResource* aSource) : mSource(aSource) + , mFrameLock(false) , mOffset(0) , mFirstFrameOffset(0) , mNumParsedFrames(0) @@ -425,6 +427,7 @@ MP3TrackDemuxer::FindFirstFrame() // For compatibility reasons we have to use the same frame count as Chrome, since // some web sites actually use a file that short to test our playback capabilities. static const int MIN_SUCCESSIVE_FRAMES = 3; + mFrameLock = false; MediaByteRange candidateFrame = FindNextFrame(); int numSuccFrames = candidateFrame.Length() > 0; @@ -465,6 +468,7 @@ MP3TrackDemuxer::FindFirstFrame() if (numSuccFrames >= MIN_SUCCESSIVE_FRAMES) { MP3LOG("FindFirst() accepting candidate frame: " "successiveFrames=%d", numSuccFrames); + mFrameLock = true; } else { MP3LOG("FindFirst() no suitable first frame found"); } @@ -493,7 +497,7 @@ MediaByteRange MP3TrackDemuxer::FindNextFrame() { static const int BUFFER_SIZE = 64; - static const int MAX_SKIPPED_BYTES = 1024 * BUFFER_SIZE; + static const uint32_t MAX_SKIPPABLE_BYTES = 1024 * BUFFER_SIZE; MP3LOGV("FindNext() Begin mOffset=%" PRIu64 " mNumParsedFrames=%" PRIu64 " mFrameIndex=%" PRId64 " mTotalFrameLen=%" PRIu64 @@ -506,13 +510,40 @@ MP3TrackDemuxer::FindNextFrame() bool foundFrame = false; int64_t frameHeaderOffset = 0; + int64_t startOffset = mOffset; + const bool searchingForID3 = !mParser.ID3Header().Size(); // Check whether we've found a valid MPEG frame. while (!foundFrame) { - if ((!mParser.FirstFrame().Length() - && mOffset - mParser.ID3Header().TotalTagSize() > MAX_SKIPPED_BYTES) + // How many bytes we can go without finding a valid MPEG frame + // (effectively rounded up to the next full buffer size multiple, as we + // only check this before reading the next set of data into the buffer). + + // This default value of 0 will be used during testing whether we're being + // fed a valid stream, which shouldn't have any gaps between frames. + uint32_t maxSkippableBytes = 0; + + if (!mParser.FirstFrame().Length()) { + // We're looking for the first valid frame. A well-formed file should + // have its first frame header right at the start (skipping an ID3 tag + // if necessary), but in order to support files that might have been + // improperly cut, we search the first few kB for a frame header. + maxSkippableBytes = MAX_SKIPPABLE_BYTES; + // Since we're counting the skipped bytes from the offset we started + // this parsing session with, we need to discount the ID3 tag size only + // if we were looking for one during the current frame parsing session. + if (searchingForID3) { + maxSkippableBytes += mParser.ID3Header().TotalTagSize(); + } + } else if (mFrameLock) { + // We've found a valid MPEG stream, so don't impose any limits + // to allow skipping corrupted data until we hit EOS. + maxSkippableBytes = std::numeric_limits::max(); + } + + if ((mOffset - startOffset > maxSkippableBytes) || (read = Read(buffer, mOffset, BUFFER_SIZE)) == 0) { - MP3LOG("FindNext() EOS or exceeded MAX_SKIPPED_BYTES without a frame"); + MP3LOG("FindNext() EOS or exceeded maxSkippeableBytes without a frame"); // This is not a valid MPEG audio stream or we've reached EOS, give up. break; } diff --git a/dom/media/MP3Demuxer.h b/dom/media/MP3Demuxer.h index e7b3ab1a046d..3e039e6c13f0 100644 --- a/dom/media/MP3Demuxer.h +++ b/dom/media/MP3Demuxer.h @@ -454,6 +454,9 @@ private: // 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; + // Current byte offset in the source stream. int64_t mOffset;