Closed Bug 1197985 Opened 4 years ago Closed 4 years ago

New MP3 decoder misidentifies MP3 header within ID3v2 tag

Categories

(Core :: Audio/Video: Playback, defect, P2)

43 Branch
All
Android
defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 --- wontfix
firefox42 + fixed
firefox43 --- fixed
fennec 42+ ---

People

(Reporter: JanH, Assigned: JanH, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 10 obsolete files)

255.57 KB, audio/mpeg
Details
6.37 KB, patch
JanH
: review+
Details | Diff | Splinter Review
6.23 KB, patch
JanH
: review+
Details | Diff | Splinter Review
6.28 KB, patch
Details | Diff | Splinter Review
6.17 KB, patch
Details | Diff | Splinter Review
Attached audio example broken MP3 file
This is a fallout from bug 1194085:

If you try opening this file on Android < 5.0 (in my case a Samsung Galaxy S3 Mini on Android 4.1.2):
http://hwcdn.libsyn.com/p/f/4/3/f434e89b9a46f426/PB-CoffeeCrusade-01.mp3?c_id=536624&expiration=1439435909&hwt=d0c27a2d14831297bf6fa729755ddf55

… prior to bug 1168374 being landed (https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c4c89dc9fc4b&tochange=3be8a68878e6 via mozregression), it plays without problems.
… after the switch to the new MP3 decoder, playback fails, with the file length variously shown either as 00:00, or 22:41:13.

While by default, this currently concerns only Android, by setting "media.mp3.enabled" to "true", it is reproducible in a Nightly on Desktop (Windows 7) as well.

I've also attached an MP3 containing only the first few seconds of the above file (created in mp3DirectCut), which is exhibiting the same symptoms.
I've found another MP3 file which doesn't work with the new MP3 decoder:
http://static.echonest.com/audio2/1439988754650/Farmboy%20Slim%20-%20Me%20Brand%20New%20Tractor.mp3

Unlike "The Coffee Crusade", this file not only doesn't play, but after loading it on Nightly, the media player interface doesn't even show up until I mouse over/tap on it. If I open that file via the Infinite Jukebox (http://labs.echonest.com/Uploader/index.html?trid=TRCXWVI14F46066E06&thresh=80), it actually manages to crash Firefox, see e.g here: bp-4968c7f8-7774-4000-90e3-6d7112150829
The original file comes from this podcast (http://www.comedy.co.uk/podcasts/john_dredge_show/series4_6/ around 15:59) and plays on Nightly without problems. The extract above was created by cutting the file in Mp3DirectCut and editing the metadata in Winamp before uploading it to the Infinite Jukebox.
I've done a bit of further investigation, and it seems that for the song from Comment 1, the ID3v2 tag breaks parsing - at 0x1b5d7 the title data contains the byte sequence FF FE 4D…, i.e. (UTF-16 BOM)Me Brand…, which seems to trip up the MP3 parser. After changing the title in Winamp to e.g. "Brand New Tractor", the file starts working in Nightly, although its length is still misdetected as 00:58 instead of approximately 00:40.
Likewise, putting the "M" back at the beginning of the title breaks playback again.
This sounds like we have a false positive in the ID3 header detection ([1]). Should be straight-forward to fix with the reproducing file.

Jan, if you like to work on this fix, just flag me for review. Otherwise, I'll take this bug.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MP3Demuxer.cpp#961
Flags: needinfo?(jh+bugzilla)
I've taken a look at the Coffee Crusade as well, and it seems that there it's detecting the ICC profile of the embedded cover art image (ÿâ.XICC_PROFILE, i.e. FF E2 0C 58) as a false header.

I'm probably a bit busy in the next three or four days, and I should probably warn you that my practical C++ knowledge so far is nearly zero, but if possible, I'd like to take a stab at it anyway.
Flags: needinfo?(jh+bugzilla)
Summary: New MP3 decoder breaks certain MP3 files → New MP3 decoder misidentifies MP3 header within ID3v2 tag
Priority: -- → P2
(In reply to Jan Henning (:JanH) from comment #4)
> I'm probably a bit busy in the next three or four days, and I should
> probably warn you that my practical C++ knowledge so far is nearly zero, but
> if possible, I'd like to take a stab at it anyway.

If you're interested, you're welcome to work on this.

The patch won't require a lot of code changes, but you will need to know/learn about the ID3 header and possibly MPEG frame formats and understand bitwise operations.

We would cover potential issue during the review, so don't worry about getting everything right at first try.

Ideally, you just drop by in our IRC channel #mobile and we'll help you getting started.

I'm assigning this to you, please unassigned in case that you don't plan/have time to work on this any more.
Assignee: nobody → jh+bugzilla
Mentor: esawin
I think I've found what goes wrong:
In FrameParser::Parse, if we have detected a valid ID3v2 header, we try skipping past the ID3 tag, but unless the ID3 tag is small enough to fit within the buffer this function is operating on (and ID3 tags with cover art usually aren't), we end up skipping past the end of the buffer, which finally returns control back to MP3TrackDemuxer::FindNextFrame. That in turn proceeds to simply read the next 4K bytes into the buffer and resume normal parsing, ignoring the fact that we're still right in the middle of the ID3 tag.

I've also found two other small issues:
- The code for skipping the ID3 tag doesn't account for the possible presence of a ID3v2.4 footer. As the difference is only ten bytes and the ID3 header/footer doesn't contain any 0xFF which might confuse the MPEG frame parser, it's probably not really critical, but if I'll be touching that code, I might as well fix that as well.
- While parsing the last byte of an ID3 header, ID3Header::IsValid(aPos) prematurely returns true, so the last size byte is never checked for validity.

Patch hopefully follows within the next few days.
(In reply to Jan Henning (:JanH) from comment #6)
> In FrameParser::Parse, if we have detected a valid ID3v2 header, we try
> skipping past the ID3 tag, but unless the ID3 tag is small enough to fit
> within the buffer this function is operating on (and ID3 tags with cover art
> usually aren't), we end up skipping past the end of the buffer, which
> finally returns control back to MP3TrackDemuxer::FindNextFrame. That in turn
> proceeds to simply read the next 4K bytes into the buffer and resume normal
> parsing, ignoring the fact that we're still right in the middle of the ID3
> tag.

Great find! This should be easy to fix by passing the actual skip range back to MP3TrackDemuxer. I just fear it would make the ::Parse interface slightly inconsistent (by returning values > aEnd).

> The code for skipping the ID3 tag doesn't account for the possible
> presence of a ID3v2.4 footer. As the difference is only ten bytes and the
> ID3 header/footer doesn't contain any 0xFF which might confuse the MPEG
> frame parser, it's probably not really critical, but if I'll be touching
> that code, I might as well fix that as well.

I'm not concerned about it, as you've said, it's just 10 (safe) optional bytes.

> While parsing the last byte of an ID3 header, ID3Header::IsValid(aPos)
> prematurely returns true, so the last size byte is never checked for
> validity.

Another good find, but make sure that ID3Header::IsValid() remains safe to use outside of this context after the fix.
Here we go, my first patch for review.
Attachment #8657196 - Flags: review?(esawin)
On second thoughts, better split the two commits it into two files.
Attachment #8657196 - Attachment is obsolete: true
Attachment #8657196 - Flags: review?(esawin)
Attachment #8657208 - Flags: review?(esawin)
Comment on attachment 8657208 [details] [diff] [review]
Bug 1197985 - Part 1 - Successfully skip ID3 headers stretching beyond the current input buffer.patch

Review of attachment 8657208 [details] [diff] [review]:
-----------------------------------------------------------------

Good work! With the comments addressed, this should be a good patch.

::: dom/media/MP3Demuxer.cpp
@@ +404,5 @@
>      MOZ_ASSERT(mOffset + read > mOffset);
>      mOffset += read;
>      bufferEnd = buffer + read;
>      frameBeg = mParser.Parse(buffer, bufferEnd);
> +    

Whitespace.

@@ +405,5 @@
>      mOffset += read;
>      bufferEnd = buffer + read;
>      frameBeg = mParser.Parse(buffer, bufferEnd);
> +    
> +    if (frameBeg - bufferEnd > 0) {

|frameBeg > bufferEnd| would be clearer and since you're using the difference multiple times, put it into a temp variable.

@@ +406,5 @@
>      bufferEnd = buffer + read;
>      frameBeg = mParser.Parse(buffer, bufferEnd);
> +    
> +    if (frameBeg - bufferEnd > 0) {
> +      // We need to skip an ID3 header which spans multiple buffers.

nit: it's the ID3 tag, the header is the part we've parsed to get the tag size.

@@ +409,5 @@
> +    if (frameBeg - bufferEnd > 0) {
> +      // We need to skip an ID3 header which spans multiple buffers.
> +      MOZ_ASSERT(mOffset + frameBeg - bufferEnd > mOffset);
> +      mOffset += frameBeg - bufferEnd;
> +      frameBeg = bufferEnd;

I assume you do this to comply with the while-loop condition, it might be cleaner to adjust the condition instead. There is also an "Exit" check following the while-loop which needs to be adjusted in this case.

@@ +643,5 @@
>      aBeg -= FrameHeader::SIZE;
>      return aBeg;
>    }
> +  // If no headers (both ID3 and MP3) have been found, this is equivalent to returning aEnd.
> +  // If we have found a large ID3 header and want to skip past it, aBeg will point past the

ID3 tag.

@@ +645,5 @@
>    }
> +  // If no headers (both ID3 and MP3) have been found, this is equivalent to returning aEnd.
> +  // If we have found a large ID3 header and want to skip past it, aBeg will point past the
> +  // end of the buffer, which needs to be handled by the calling function.
> +  return aBeg;

In this case, the earlier |return aBeg| can be removed.

@@ +1048,5 @@
>  }
>  
>  bool
>  ID3Parser::ID3Header::IsValid(int aPos) const {
> +  if (aPos >= SIZE) {

With this, IsValid() should just return IsValid(mPos).
This should also be fixed for FrameHeader::IsValid.
Attachment #8657208 - Flags: review?(esawin) → feedback+
Comment on attachment 8657209 [details] [diff] [review]
Bug 1197985 - Part 2 - Prevent buffer pointer from potentially overflowing.patch

Review of attachment 8657209 [details] [diff] [review]:
-----------------------------------------------------------------

This is a valid solutions but suffers in complexity due to the current interface which doesn't allow for easy overflow handling.
Would you be willing to explore a solution based on ByteReader?

The idea is to construct a ByteReader in FindNextFrame and pass it (by reference) to the Parse function for the byte-wise processing.
This would give us a clean and easy way to check for overflows and would allow us to use the return values for error handling and byte skipping.

::: dom/media/MP3Demuxer.cpp
@@ +403,5 @@
>      }
>      MOZ_ASSERT(mOffset + read > mOffset);
>      mOffset += read;
>      bufferEnd = buffer + read;
> +    const FrameParserResult parseResults = mParser.Parse(buffer, bufferEnd);

With the adjusted while-loop condition (see first patch's review), the returned buffer position would indicate the required range to skip implicitly. I don't think we gain much with have a dedicated parser result structure.

@@ +634,5 @@
> +        // Skipping across the ID3 header would take us past the end of the buffer, therefore we
> +        // return immediately and let the calling function handle skipping the rest of the ID3 header.
> +        return FrameParserResult{ aEnd, id3Offset + totalID3HeaderSize - bufferSize  };
> +      }
> +      aBeg = id3Beg + totalID3HeaderSize;

This seems rather complex because of the given interface. I think we could make the interface cleaner and avoiding some of these issues by passing a ByteReader [1] to all Parse functions instead of the two buffer pointers.
 
[1] https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/include/mp4_demuxer/ByteReader.h

::: dom/media/MP3Demuxer.h
@@ +111,5 @@
>    ID3Header mHeader;
>  };
>  
> +struct FrameParserResult {
> +  const uint8_t* bufferPos;

Use m prefix for member variables.
Attachment #8657209 - Flags: review?(esawin) → feedback+
(In reply to Eugen Sawin [:esawin] from comment #11)
> Good work! With the comments addressed, this should be a good patch.

Thanks!


> Whitespace.

Fixed.

> |frameBeg > bufferEnd| would be clearer and since you're using the
> difference multiple times, put it into a temp variable.

This was a remnant of my first try at handling potential overflows, fixed.

> nit: it's the ID3 tag, the header is the part we've parsed to get the tag
> size.

Duh. I noticed that myself, but apparently didn't catch all instances.

> I assume you do this to comply with the while-loop condition, it might be
> cleaner to adjust the condition instead. There is also an "Exit" check
> following the while-loop which needs to be adjusted in this case.

My reasoning was that I didn't want to leave the pointer pointing to some random piece of memory for longer than necessary, and in Part 2, FrameParser::Parse() reverts to simply returning values between [aBeg, aEnd] anyway. See also my following comments for Part 2.

> ID3 tag.

> In this case, the earlier |return aBeg| can be removed.

Both fixed.

> With this, IsValid() should just return IsValid(mPos).
> This should also be fixed for FrameHeader::IsValid.

Good catch, I changed FrameHeader::IsValid(mPos) to check the passed argument (instead of mPos, which gets incremented behind its back) as well.
Regarding both IsValid() functions, you mean changing |return mPos >= SIZE;| to |return IsValid(mPos);|?
That won't work because IsValid(mPos) needs to validate the current char, while IsValid needs to return true/false based on the current state of the parser, i.e. only return true if the whole header is valid and false otherwise, even if the currently parsed character itself is valid.
Attachment #8657208 - Attachment is obsolete: true
Attachment #8657295 - Flags: review?(esawin)
Duh, the commit message needs to refer to tags as well.
Attachment #8657295 - Attachment is obsolete: true
Attachment #8657295 - Flags: review?(esawin)
Attachment #8657303 - Flags: review?(esawin)
(In reply to Eugen Sawin [:esawin] from comment #12)
> Comment on attachment 8657209 [details] [diff] [review]
> Bug 1197985 - Part 2 - Prevent buffer pointer from potentially
> overflowing.patch
> 
> Review of attachment 8657209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a valid solutions but suffers in complexity due to the current
> interface which doesn't allow for easy overflow handling.
> Would you be willing to explore a solution based on ByteReader?

As per our IRC conversation, I don't know how much time I can invest into this how soon, but in principle yes, I'd be willing to look into it.

However I'd leave that as a not-so-urgent follow-up bug and first concentrate on getting these patches landed, because I'd hate to see this bug make it into release.
 
> ::: dom/media/MP3Demuxer.cpp
> @@ +403,5 @@
> >      }
> >      MOZ_ASSERT(mOffset + read > mOffset);
> >      mOffset += read;
> >      bufferEnd = buffer + read;
> > +    const FrameParserResult parseResults = mParser.Parse(buffer, bufferEnd);
> 
> With the adjusted while-loop condition (see first patch's review), the
> returned buffer position would indicate the required range to skip
> implicitly. I don't think we gain much with have a dedicated parser result
> structure.

My reasoning was the following one: If for whatever reasons the OS decides to allocate a buffer near the address space boundary (within 256 MB) and the MP3 file contains a relatively large ID3 tag, then the result from mParser.Parse (which is basically the buffer start address + ID3 tag size) might overflow.

After the first patch, this return value now has to do double duty not only as a pointer into the given buffer if parsing was successful (or point to bufferEnd if it was completely unsuccessful), but also as an indicator of a range of bytes that needs to be skipped if a large ID3 tag was detected. This complicates handling and even more so if we want to handle the overflow case as well. And we probably should, after all, it's not the MP3 file's fault that the OS decided to allocate a buffer near the address space boundary.

As a temporary solution until the whole thing can be switched to using ByteReader, I find using the struct cleaner than trying to overload the return pointer with two completely different meanings, one of which might lead to it overflowing.

> Use m prefix for member variables.

Done, also fixed further bits of ID3 header vs. tag confusion in variable names and comments.
Attachment #8657209 - Attachment is obsolete: true
Attachment #8657313 - Flags: feedback?(esawin)
Comment on attachment 8657303 [details] [diff] [review]
Bug 1197985 - Part 1v3 - Successfully skip ID3 headers stretching beyond the current input buffer.patch

Review of attachment 8657303 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work!
Attachment #8657303 - Attachment filename: 1197985 - Part 1v3 - Successfully skip ID3 headers stretching beyond the current input buffer.patch → 1197985 - Part 1v3 - Successfully skip ID3 headers stretching beyond the current input buffer.patch
Attachment #8657303 - Flags: review?(esawin) → review+
Comment on attachment 8657303 [details] [diff] [review]
Bug 1197985 - Part 1v3 - Successfully skip ID3 headers stretching beyond the current input buffer.patch

Review of attachment 8657303 [details] [diff] [review]:
-----------------------------------------------------------------

nit

::: dom/media/MP3Demuxer.cpp
@@ +626,5 @@
>      const uint8_t* id3Beg = mID3Parser.Parse(aBeg, aEnd);
>      if (id3Beg != aEnd) {
>        // ID3 headers found, skip past them.
> +      aBeg = id3Beg + ID3Parser::ID3Header::SIZE + mID3Parser.Header().Size()
> +             + mID3Parser.Header().FooterSize();

Leave the |+| at the end of the previous line for consistency (within this module).
Comment on attachment 8657313 [details] [diff] [review]
Bug 1197985 - Part 2v2 - Prevent buffer pointer from potentially overflowing.patch

Review of attachment 8657313 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MP3Demuxer.cpp
@@ +409,2 @@
>  
> +    if (parseResults.mBytesToSkip) {

This check won't be required, if...

@@ +411,2 @@
>        // We need to skip an ID3 tag which stretches beyond the current buffer.
> +      MOZ_ASSERT(mOffset + parseResults.mBytesToSkip > mOffset);

... you change the constraint to >=.

@@ +414,1 @@
>        frameBeg = bufferEnd;

This shouldn't be required any more.

@@ +612,5 @@
>  FrameParser::VBRInfo() const {
>    return mVBRHeader;
>  }
>  
> +const FrameParserResult

Don't return const values, this is only useful when returning references.

@@ +617,3 @@
>  FrameParser::Parse(const uint8_t* aBeg, const uint8_t* aEnd) {
>    if (!aBeg || !aEnd || aBeg >= aEnd) {
> +    return FrameParserResult{ aEnd, 0 };

The class name can be omitted (multiple cases in the patch).

@@ +626,5 @@
>      const uint8_t* id3Beg = mID3Parser.Parse(aBeg, aEnd);
>      if (id3Beg != aEnd) {
> +      // ID3 tag found, skip past it.
> +      const uint32_t totalID3TagSize = ID3Parser::ID3Header::SIZE + mID3Parser.Header().Size()
> +                                          + mID3Parser.Header().FooterSize();

Indentation.

@@ +632,5 @@
> +      const uint32_t bufferSize = aEnd - aBeg;
> +      if (id3Offset + totalID3TagSize > bufferSize) {
> +        // Skipping across the ID3 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.
> +        return FrameParserResult{ aEnd, id3Offset + totalID3TagSize - bufferSize  };

Let's try to simplify the expression, e.g.:

const uint32_t tagSize = ...;
const uint32_t remainingBuffer = aEnd - id3Beg;
if (remainingBuffer < tagSize) {
  return { aEnd, tagSize - remainingBuffer };
}
Attachment #8657313 - Flags: feedback?(esawin) → feedback+
Nit fixed, carrying forward r+ from esawin.
Attachment #8657303 - Attachment is obsolete: true
Attachment #8657477 - Flags: review+
(In reply to Eugen Sawin [:esawin] from comment #18)
> Comment on attachment 8657313 [details] [diff] [review]
> Bug 1197985 - Part 2v2 - Prevent buffer pointer from potentially
> overflowing.patch
> 
> Review of attachment 8657313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This check won't be required, if...
> ... you change the constraint to >=.
> 
> This shouldn't be required any more.

Good point, fixed.
 
> > +const FrameParserResult
> 
> Don't return const values, this is only useful when returning references.

Thanks, this is one bit where my lack of experience comes into play :-) But now that you mention it, it sounds logical.

> The class name can be omitted (multiple cases in the patch).

Removed.

> Indentation.

Oops.

> Let's try to simplify the expression, e.g.:
> 
> const uint32_t tagSize = ...;
> const uint32_t remainingBuffer = aEnd - id3Beg;
> if (remainingBuffer < tagSize) {
>   return { aEnd, tagSize - remainingBuffer };
> }

Good point, your version saves one temp variable when compared to mine.
Attachment #8657313 - Attachment is obsolete: true
Attachment #8657478 - Flags: review?(esawin)
This contains one additional change in FrameHeader::IsValid(aPos):

>    if (aPos == frame_header::BITRATE_SAMPLERATE_PADDING_PRIVATE) {
> -    return RawBitrate() != 0xF;
> +    return RawBitrate() != 0xF && RawBitrate() != 0 &&
> +           RawSampleRate() != 3;
>    }

I've been playing around with some of the sample files from MPlayer (http://samples.mplayerhq.hu/A-codecs/MP3/) and especially this one:
http://samples.mplayerhq.hu/A-codecs/MP3/Boot%20to%20the%20Head.MP3

With the new MP3 parser implementation, playback stops at around 00:40, which can be fixed by extending FrameHeader::IsValid(aPos) to check for RawSampleRate() != 3, which according to the MP3 specification is "reserved" and therefore effectively invalid.
After that, playback stops at 00:55, which can be fixed by additionally checking for RawBitrate() != 0 as we don't support the "free" bitrate anyway.

With both changes in place, we can actually play that file completely, even though we get a few skips while passing some corrupted bits of the file.

On a side note, for this file the new implementation of the MP3 parser still seems to perform slightly worse than the old one - around 0:44 for example, there is a noticeable skip which almost swallows the word "pebble" which isn't present in the old implementation.
But as that file has quite a bit of corruption, it's probably not worth investing too much energy into that issue - the important thing is to get non-corrupted files to play back successfully, which this patch does.
Attachment #8657477 - Attachment is obsolete: true
Attachment #8657620 - Flags: review?(esawin)
Rebased on top of Part 1v5, otherwise no change.
Attachment #8657478 - Attachment is obsolete: true
Attachment #8657478 - Flags: review?(esawin)
Attachment #8657621 - Flags: review?(esawin)
Blocks: 1172419
Comment on attachment 8657620 [details] [diff] [review]
Bug 1197985 - Part 1v5 - Successfully skip ID3 tags stretching beyond the current input buffer.patch

Review of attachment 8657620 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good (see comment for optional change).

::: dom/media/MP3Demuxer.cpp
@@ +408,5 @@
> +
> +    if (frameBeg > bufferEnd) {
> +      // We need to skip an ID3 tag which stretches beyond the current buffer.
> +      const uint32_t bytesToSkip = frameBeg - bufferEnd;
> +      MOZ_ASSERT(mOffset + bytesToSkip > mOffset);

I think this and the previous assertion should be changed to NS_ENSURE_TRUE(...) instead to abort on overflow on release builds, too, even though this is a highly unlikely situation.
Attachment #8657620 - Flags: review?(esawin) → review+
Comment on attachment 8657621 [details] [diff] [review]
Bug 1197985 - Part 2v4 - Prevent buffer pointer from potentially overflowing.patch

Review of attachment 8657621 [details] [diff] [review]:
-----------------------------------------------------------------

Let's land this first and refactor the Parse functions to use ByteReader in a follow-up bug.

::: dom/media/MP3Demuxer.cpp
@@ +409,4 @@
>  
> +    // If mBytesToSkip is > 0, this skips the rest of an ID3 tag which stretches
> +    // beyond the current buffer.
> +    MOZ_ASSERT(mOffset + parseResults.mBytesToSkip >= mOffset);

Maybe also better change to NS_ENSURE_TRUE?
Attachment #8657621 - Flags: review?(esawin) → review+
(In reply to Jan Henning (:JanH) from comment #21)
> On a side note, for this file the new implementation of the MP3 parser still
> seems to perform slightly worse than the old one - around 0:44 for example,
> there is a noticeable skip which almost swallows the word "pebble" which
> isn't present in the old implementation.

I assume with "old implementation" you mean using the OMX-plugin? It's difficult to directly compare the solutions since with the new demuxer implementation, we're using a whole new media pipeline and more importantly the MediaCodec decoder.
(In reply to Eugen Sawin [:esawin] from comment #25)
> I assume with "old implementation" you mean using the OMX-plugin?

Basically yes. I've mostly used the desktop build for comparison, as I've used it for developing (Fennec doesn't build on Windows), but the behaviour there (with media.mp3.enabled=false/not set) and on Android (with OMX) is similar.
That file seems to have a number of frames with missing bytes, so my suspicion is that with the new implementation, after having parsed a frame, we jump forward and because the previous frame was a few bytes short, we jump past the next frame header into the middle of the next frame. Then we skip forward until we detect the next MPEG frame header (and without the changes from Comment 21, twice get a false positive along the way), causing an audible jump. I haven't really looked at the old implementation to see what it does differently there, though.

I'll make the changes regarding MOZ_ASSERT as soon as I get back home.

The follow-up for implementing the ByteReader when I get round to it is bug 1202286 (already CC'd you).
Replace usage of MOZ_ASSERT with NS_ENSURE_TRUE as suggested, carrying forward r+ from esawin.
Attachment #8657620 - Attachment is obsolete: true
Attachment #8657890 - Flags: review+
Replace usage of MOZ_ASSERT with NS_ENSURE_TRUE as suggested, carrying forward r+ from esawin.
Attachment #8657621 - Attachment is obsolete: true
Attachment #8657891 - Flags: review+
Component: Audio/Video → Audio/Video: Playback
Keywords: checkin-needed
Blocks: 1202475
No longer blocks: 1202475
https://hg.mozilla.org/mozilla-central/rev/afc2ca41d78b
https://hg.mozilla.org/mozilla-central/rev/772e70e43088
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8657890 [details] [diff] [review]
Bug 1197985 - Part 1v6 - Successfully skip ID3 tags stretching beyond the current input buffer.patch

Approval Request Comment
[Feature/regressing bug #]: MP3 playback on Android, Bug 1168374 and Bug 1166779
[User impact if declined]: Affected MP3 files will either show wrong duration, not play at all, or, when used in a WebAudio context, possibly even crash Firefox.
[Describe test coverage new/current, TreeHerder]: pre-existing gtest for the MP3 demuxer + manual testing
[Risks and why]: Low, changes affect only a small part of the MP3 demuxer. ID3 header size detection was already implemented, only we erroneously didn't skip more than 4096 bytes.
[String/UUID change made/needed]: none
Attachment #8657890 - Flags: approval-mozilla-beta?
Attachment #8657890 - Flags: approval-mozilla-aurora?
Comment on attachment 8657891 [details] [diff] [review]
Bug 1197985 - Part 2v5 - Prevent buffer pointer from potentially overflowing.patch

Approval Request Comment
[Feature/regressing bug #]: MP3 playback on Android, Bug 1168374 and Bug 1166779
[User impact if declined]: Affected MP3 files will either show wrong duration, not play at all, or, when used in a WebAudio context, possibly even crash Firefox. Part 2 also prevents a potential overflow of a buffer pointer which might lead to another crash.
[Describe test coverage new/current, TreeHerder]: pre-existing gtest for the MP3 demuxer + manual testing
[Risks and why]: Low, changes affect only a small part of the MP3 demuxer. ID3 header size detection was already implemented, only we erroneously didn't skip more than 4096 bytes and didn't check for overflows of the buffer pointer.
[String/UUID change made/needed]: none

Note: This patch depends on the changes made in Part 1.
Attachment #8657891 - Flags: approval-mozilla-beta?
Attachment #8657891 - Flags: approval-mozilla-aurora?
Blocks: 1203217
Snorp, since this is related to MP3 playback in Android and we are in the end game of Beta41 (gtb RC next week), I would like your opinion on whether this is a safe fix to take so late in Beta41 cycle? Or can this wait to be fixed in 42? I am unable to judge if this is a pretty wide spread issue or only affects a small fraction of users. Given that 40 is unaffected, it seems to be a recent regression. Appreciate your help.
Flags: needinfo?(snorp)
For context, 40 is unaffected because it uses the old MP3 decoding code, which as far as I have gathered doesn't work on Android 5.x. This bug was introduced as part of the new MP3 decoder which is supposed to enable MP3 playback on Android 5+ and which was made live in bug 1168374.
Desktop is unaffected as well because for the time being it continues using the old MP3 decoder unless you manually create a new about:config pref.

Which users are affected depends on which MP3 files they want to listen to - after a short search, I've been able to find affected files on podiobooks.com, mixcloud.com, The Infinite Jukebox (http://labs.echonest.com/Uploader/index.html) and some random podcasts hosted by libsyn.

But I defer to snorp for judging the safeness of this patches.
Comment on attachment 8657891 [details] [diff] [review]
Bug 1197985 - Part 2v5 - Prevent buffer pointer from potentially overflowing.patch

We can take this for Aurora42. 

With recent HTML5 youtube playback issue in 41.0b7 (bug 1202296) I feel the need to be extra cautious with Beta uplifts now. Given that this code change is in the MP3 playback in Android, we may be ok to live with the current issue rather than regressing existing release quality. I do not feel comfortable taking this in Beta41 cycle.
Attachment #8657891 - Flags: approval-mozilla-beta?
Attachment #8657891 - Flags: approval-mozilla-beta-
Attachment #8657891 - Flags: approval-mozilla-aurora?
Attachment #8657891 - Flags: approval-mozilla-aurora+
wontfix for 41, tracked for 42 to ensure we have good coverage on this fix.
Flags: qe-verify+
tracking-fennec: --- → ?
Flags: needinfo?(snorp)
Unless we know of a critical case where this bug is a problem, I don't think we need to uplift for Beta. Aurora seems safe enough.
Comment on attachment 8657890 [details] [diff] [review]
Bug 1197985 - Part 1v6 - Successfully skip ID3 tags stretching beyond the current input buffer.patch

Aurora42+, Beta41-
Attachment #8657890 - Flags: approval-mozilla-beta?
Attachment #8657890 - Flags: approval-mozilla-beta-
Attachment #8657890 - Flags: approval-mozilla-aurora?
Attachment #8657890 - Flags: approval-mozilla-aurora+
tracking-fennec: ? → 42+
... for Aurora.
Keywords: checkin-needed
Hi, this cause conflicts when uplifting to aurora:

grafting 297722:afc2ca41d78b "Bug 1197985 - Part 1 - Successfully skip ID3 tags stretching beyond the current input buffer. r=esawin"
merging dom/media/MP3Demuxer.cpp
3 files to edit
merging dom/media/MP3Demuxer.cpp failed!

can you take a look, thanks!
Flags: needinfo?(jh+bugzilla)
Patch rebased for Aurora uplift
Flags: needinfo?(jh+bugzilla)
Target Milestone: mozilla43 → mozilla42
You need to log in before you can comment on or make changes to this bug.