MP3 parser should honour MAX_SKIPPED_BYTES during findFirstFrame() as well

RESOLVED FIXED in Firefox 56

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

11 months ago
At the moment, the MP3 parser obeys MAX_SKIPPED_BYTES (i.e. the maximum amount of data it should read while trying to find a valid MP3 audio frame) only when searching for the "FirstFrame". According to bug 1194085 comment #16, this was done so we'd be more tolerant against stream corruption after we've found a valid frame header and parsing has started in earnest.

Since then however we've added our FindFirstFrame() logic, which means that on startup we're looking not only for *a* valid frame header, but a number of consecutive frame headers. At the moment, once we've got a candidate frame (and the parser therefore its FirstFrame), we ignore MAX_SKIPPED_BYTES, even though the candidate frame could potentially be a false positive, so we potentially go on a wild goose chase hunting down a follow-up to that false positive.
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

11 months ago
mozreview-review
Comment on attachment 8874007 [details]
Bug 1364866 - Part 0 - Cleanups.

https://reviewboard.mozilla.org/r/145322/#review150068

::: dom/media/MP3Demuxer.cpp:426
(Diff revision 1)
>  MP3TrackDemuxer::FindFirstFrame()
>  {
> -  // Get engough successive frames to avoid invalid frame from cut stream.
> -  // However, some website use very short mp3 file so using the same value as Chrome.
> +  // We attempt to find multiple successive frames to avoid locking onto a false
> +  // positive if we're fed a stream that has been cut mid-frame.
> +  // For compatibility reasons we have to use the same frame count as Chrome, since
> +  // some web sites actually a file that short to test our playback capabilities.

+use
Attachment #8874007 - Flags: review?(esawin) → review+

Comment 5

11 months ago
mozreview-review
Comment on attachment 8874008 [details]
Bug 1364866 - Part 1 - Provide a function for directly retrieving the total size of an ID3 tag.

https://reviewboard.mozilla.org/r/145324/#review150072
Attachment #8874008 - Flags: review?(esawin) → review+

Comment 6

11 months ago
mozreview-review
Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

https://reviewboard.mozilla.org/r/139468/#review150078

::: dom/media/MP3Demuxer.h:354
(Diff revision 1)
>    // as a VBRI-style header is searched at a fixed offset relative to frame
>    // begin. Returns whether a valid VBR header was found.
>    bool ParseVBRHeader(mp4_demuxer::ByteReader* aReader);
>  
> +  // True if we're still on the lookout for an ID3 tag.
> +  bool LookingForID3() const;

Maybe reverse that to "ID3Found" or similar.

::: dom/media/MP3Demuxer.cpp:513
(Diff revision 1)
> +    return 0;
> +  } else {
> +    // We've found a valid MPEG stream, so don't impose any limits
> +    // to allow skipping corrupted data until we hit EOS.
> +    return -1;
> +  }

I think this logic doesn't need to go into a separate function, it's only needed in FindNextFrame.

MAX_SKIPPED_BYTES can remain a local constant and you can simply set maxSkippedBytes based on the same conditions there.

That way BUFFER_SIZE can be reverted back to a local constant, too.

Comment 7

11 months ago
mozreview-review
Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

https://reviewboard.mozilla.org/r/139468/#review150646
Attachment #8874009 - Flags: review?(esawin)
(Assignee)

Comment 8

11 months ago
mozreview-review-reply
Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

https://reviewboard.mozilla.org/r/139468/#review150078

> Maybe reverse that to "ID3Found" or similar.

It does sound a bit awkward, yes - unfortunately the reverse isn't simply "ID3Found", but rather "ID3OrMpegFrameFound". I guess it's still an improvement, but possibly not as much as you'd hoped :-)

> I think this logic doesn't need to go into a separate function, it's only needed in FindNextFrame.
> 
> MAX_SKIPPED_BYTES can remain a local constant and you can simply set maxSkippedBytes based on the same conditions there.
> 
> That way BUFFER_SIZE can be reverted back to a local constant, too.

True about the local constants, although it does make the `(!foundFrame)` loop a bit more unwieldy.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

11 months ago
mozreview-review
Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

https://reviewboard.mozilla.org/r/139468/#review151266

::: dom/media/MP3Demuxer.h:355
(Diff revision 2)
>    // begin. Returns whether a valid VBR header was found.
>    bool ParseVBRHeader(mp4_demuxer::ByteReader* aReader);
>  
> +  // True if we found either an ID3 or an MPEG frame header. When false,
> +  // this means that all parsing sessions will search for an ID3 header first.
> +  bool ID3OrMpegFrameFound() const;

You're right, this does sound awkward, too.
I think I still prefer it because this state is explicit while LookingForID3 is derived from the fact that we haven't found a valid stream start yet.

Either solution is odd/OK, so it's your call.

::: dom/media/MP3Demuxer.cpp:513
(Diff revision 2)
>    int32_t read = 0;
>  
>    bool foundFrame = false;
>    int64_t frameHeaderOffset = 0;
> +  int64_t startOffset = mOffset;
> +  bool searchingForID3 = !mParser.ID3OrMpegFrameFound();

Can be const.

::: dom/media/MP3Demuxer.cpp:520
(Diff revision 2)
>    // 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).
> +    int32_t maxSkippedBytes;

We can reduce this block further:

int32_t maxSkippableBytes = 0;
if (!mParser.FirstFrame().Length()) {
  maxSkippableBytes = MAX_SKIPPABLE_BYTES + mParser.ID3Header().TotalTagSize();
} else if (mFrameLock) {
  maxSkippableBytes = numeric_limits<int32_t>::max();
}

With this you don't need ID3OrMpegFrameFound and the maxSkippedBytes >= 0 and searchingForID3 checks.

::: dom/media/MP3Demuxer.cpp:539
(Diff revision 2)
> +      // We've found a valid MPEG stream, so don't impose any limits
> +      // to allow skipping corrupted data until we hit EOS.
> +      maxSkippedBytes = -1;
> +    }
> +
> +    if (maxSkippedBytes >= 0 && searchingForID3) {

Create a new block for the case maxSkippedBytes >= 0 to nest the next two conditionals into.
Attachment #8874009 - Flags: review?(esawin)
(Assignee)

Comment 13

11 months ago
mozreview-review-reply
Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

https://reviewboard.mozilla.org/r/139468/#review151266

> You're right, this does sound awkward, too.
> I think I still prefer it because this state is explicit while LookingForID3 is derived from the fact that we haven't found a valid stream start yet.
> 
> Either solution is odd/OK, so it's your call.

Neither is perfect, but in the end I'd tend to agree with you on this, so I'll keep it this way.

> We can reduce this block further:
> 
> int32_t maxSkippableBytes = 0;
> if (!mParser.FirstFrame().Length()) {
>   maxSkippableBytes = MAX_SKIPPABLE_BYTES + mParser.ID3Header().TotalTagSize();
> } else if (mFrameLock) {
>   maxSkippableBytes = numeric_limits<int32_t>::max();
> }
> 
> With this you don't need ID3OrMpegFrameFound and the maxSkippedBytes >= 0 and searchingForID3 checks.

I don't think I can get rid of searchingForID3 that easily. Consider a file that was improperly cut (i.e. we initially hit false positives while looking for a frame header) and then had an ID3 header added in front of it.

So we're starting with mOffset = 0, find the ID3 header and skip it. In this case adding the ID3 header size to maxSkippableBytes is indeed correct. Eventually we hit something that looks like a frame header and declare it a candidate frame. Soon however this candidate frame turns out to be bogus, so we reset the FirstFrame data and restart our search with mOffset set one byte past the beginning of the previous candidate frame. Since I compute the amount of skipped bytes relative to the starting offset when entering FindNextFrame() (so I can reduce maxSkippableBytes to 0 while we're verifying the frame consistency in FindFirstFrame()), this means that this time around the ID3 header shouldn't increase maxSkippableBytes, since our starting mOffset was already beyond the ID3 header.

Still, I've realised I can move the incrementing operation into this conditional block, since it is in fact only relevant if we don't have a first frame yet. Once we've got a first frame, the parser stops looking for ID3 headers anyway, so there won't be any need to add their size to maxSkippableBytes. But as I explained above, I think I still need to check for searchingForID3.

Also, if it's only a stylistic matter I think that keeping the maxSkippableBytes = 0 assignment as part of that if-else block makes the logic flow clearer - if I assign that value during initialisation of the variable, I found the corresponding comment block a bit akward to place and formulate.

> Create a new block for the case maxSkippedBytes >= 0 to nest the next two conditionals into.

Not sure this was actually easily possible because the second conditional is mixed together with the "|| read(..." part, but your other suggestion eliminates the necessity for this check, anyway.
Comment hidden (mozreview-request)

Comment 15

11 months ago
mozreview-review
Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

https://reviewboard.mozilla.org/r/139468/#review151770

r=me with nits addressed.

I feel like adjusting the skippable range to such detail might be overkill, can't we just increase the default to account for a sensibly sized ID3 tag?

::: dom/media/MP3Demuxer.cpp:500
(Diff revision 3)
>  
>  MediaByteRange
>  MP3TrackDemuxer::FindNextFrame()
>  {
>    static const int BUFFER_SIZE = 64;
> -  static const int MAX_SKIPPED_BYTES = 1024 * BUFFER_SIZE;
> +  static const int MAX_SKIPPABLE_BYTES = 1024 * BUFFER_SIZE;

uint32_t

::: dom/media/MP3Demuxer.cpp:521
(Diff revision 3)
>    // 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).
> +    uint32_t maxSkippableBytes;

Prefer to initialize variables to sensible defaults. In this case, 0 would be a good default and a short comment would not feel out of place despite the comments above.

::: dom/media/MP3Demuxer.cpp:853
(Diff revision 3)
>  }
>  
>  bool
> +FrameParser::ID3OrMpegFrameFound() const
> +{
> +  return mID3Parser.Header().Size() || mFirstFrame.Length();

This function does not add any real value, I think using the condition directly gives us the same degree of expressiveness.
Attachment #8874009 - Flags: review?(esawin) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 17

11 months ago
mozreview-review-reply
Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

https://reviewboard.mozilla.org/r/139468/#review151770

What counts as a "sensibly sized" ID3 tag, though? Looking through my music library to get at least a rough feeling (though I've no idea how representative my library actually is for that purpose), the .90 quantile is 125 kB, the .95 quantile around 370 kB, and a few podcasts actually reach up to 750 kB, which I think is getting already a little high for reading that much data just to find that it wasn't a valid MP3 file after all.

> This function does not add any real value, I think using the condition directly gives us the same degree of expressiveness.

Done - and in FindNextFrame() I've realised I don't need the !FirstFrame().Length() check either, as the if (searchingForID3) block is already nested within an if block with the same condition.

Comment 18

11 months ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/cab7f322f3ef
Part 0 - Cleanups. r=esawin
https://hg.mozilla.org/integration/autoland/rev/2335a5bb9c16
Part 1 - Provide a function for directly retrieving the total size of an ID3 tag. r=esawin
https://hg.mozilla.org/integration/autoland/rev/daa88d8a2901
Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state. r=esawin

Comment 19

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cab7f322f3ef
https://hg.mozilla.org/mozilla-central/rev/2335a5bb9c16
https://hg.mozilla.org/mozilla-central/rev/daa88d8a2901
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.