1364866 - MP3 parser should honour MAX_SKIPPED_BYTES during findFirstFrame() as well

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[Jan Henning [:JanH]]

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.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1364866 - Part 0 - Cleanups.

Review commit: https://reviewboard.mozilla.org/r/145322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145322/

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1364866 - Part 1 - Provide a function for directly retrieving the total size of an ID3 tag.

Review commit: https://reviewboard.mozilla.org/r/145324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145324/

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

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.

Review commit: https://reviewboard.mozilla.org/r/139468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139468/

Comment 4

[Eugen Sawin [:esawin]]

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

Comment 5

[Eugen Sawin [:esawin]]

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

Comment 6

[Eugen Sawin [:esawin]]

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

[Eugen Sawin [:esawin]]

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

Comment 8

[Jan Henning [:JanH]]

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 9

[Jan Henning [:JanH]]

Comment on attachment 8874007 [details]
Bug 1364866 - Part 0 - Cleanups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145322/diff/1-2/

Comment 10

[Jan Henning [:JanH]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145324/diff/1-2/

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139468/diff/1-2/

Comment 12

[Eugen Sawin [:esawin]]

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.

Comment 13

[Jan Henning [:JanH]]

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 14

[Jan Henning [:JanH]]

Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139468/diff/2-3/

Comment 15

[Eugen Sawin [:esawin]]

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.

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 8874009 [details]
Bug 1364866 - Part 2 - Set MAX_SKIPPED_BYTES dynamically depending on parsing state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139468/diff/3-4/

Comment 17

[Jan Henning [:JanH]]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cab7f322f3ef
https://hg.mozilla.org/mozilla-central/rev/2335a5bb9c16
https://hg.mozilla.org/mozilla-central/rev/daa88d8a2901