Another MP3 that doesn't play in DirectShow backend

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

Trunk
mozilla26
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

See bug 908862 comment 14, this MP3 doesn't play with the DirectShow backend.

http://nxt-level.com/music/DJ%20Shadow%20feat.%20Mos%20Def%20-%20Six%20Days.mp3

On inspection, it seems there is noise after the end of the first ID3v2.2 tag and start of the second ID3v2.3 tag. I guess we need to do what nsMediaSniffer does, and use the ID3 tag lengths to enable us to skip up to the first MP3 sync pattern.
So the problem with this MP3 file is that it has multiple ID3 tags, and some junk bytes in between the tags, which is confusing my parser. My parser (which copies code from mp3_sniff.c) also is getting false positives when searching for MP3 frames, so I'm going to change to use B2G's MP3FrameParser instead. This will reduce the number of MP3 parsers we have in the tree. I'll have to fix a few problems with MP3FrameParser, but in general its MP3 frame parser is more robust than mp3_sniff.c's.
Make MP3FrameParser usable outside of B2G code.
Attachment #798630 - Flags: review?(paul)
* Alter MP3FrameParser to determine the offset of the first MP3 frame in the stream.
* Refactor MP3FrameParser a bit to (hopefully) make it a little clearer what's going on.
* Change MP3FrameParser to handle multiple ID3 tags in the stream.
* Change MP3FrameParser to handle junk in between ID3 tags
* Remove the buggy "retries" logic from MP3FrameParser, and instead keep count of the number of bytes the parser has skipped trying to find a sync frame, and assume the stream is not MP3 if we've not encountered an MP3 or ID3v2 frame yet, and we've skipped 200KB looking for headers. (I've seen ID3v2 tags of size of 143KB). The "retries" logic was failing when we were appending bytes and there was junk between headers.
* Use MP3FrameParser in DirectShow's SourceFilter, to get the offset of MP3 frames, so that it can strip off ID3v2 tags from the stream it exposes to DirectShow, since DirectShow can't handle some ID3v2 tags.
* Add test that includes two ID3v2 tags with in between them.

Thomas: Thanks for writing the MP3FrameParser! Can you test this patch on your existing MP3 test set on B2G? I don't have a B2G device or build environment at the moment, and I don't have your test set of MP3s. Thanks!
Attachment #798632 - Flags: review?(paul)
Attachment #798632 - Flags: feedback?(tzimmermann)
Attachment #798630 - Flags: review?(paul) → review+
Comment on attachment 798632 [details] [diff] [review]
Patch 2v1: Use MP3FrameParser in DirectShow's SourceFilter

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

Hi!

There is trivial compiler error and a few nits. I just tested the patch set with b2g and my mp3s seem to work fine.

::: content/media/MP3FrameParser.cpp
@@ -112,5 @@
>  
> -  int64_t GetTrailing() const {
> -    return mTrailing;
> -  }
> -

It looks like those trailing bytes are now handled in MP3FrameParser::Parse, right? Otherwise you might miscalculate the duration.

@@ +250,5 @@
>  
>      nsresult rv = DecodeFrameHeader(buffer, &frameSize, &bitRate, &duration);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }

No warning here?

@@ -461,5 @@
>  }
>  
> -void MP3FrameParser::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset)
> -{
> -  Parse(reinterpret_cast<const uint8_t*>(aBuffer), aLength, aOffset);

Removing this cast results in a compile error on b2g.

::: content/media/MP3FrameParser.h
@@ +59,2 @@
>  private:
> +  

Whitespace error.

@@ +70,2 @@
>  
> +  // mBuffer must be at least 19 bytes long, in case the last byte in the 

Whitespace error.
Attachment #798632 - Flags: feedback?(tzimmermann) → feedback+
Comment on attachment 798632 [details] [diff] [review]
Patch 2v1: Use MP3FrameParser in DirectShow's SourceFilter

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

Thanks for looking at this patch Thomas!

I'll fix the nits you pointed out.

::: content/media/MP3FrameParser.cpp
@@ -112,5 @@
>  
> -  int64_t GetTrailing() const {
> -    return mTrailing;
> -  }
> -

Right. We detect trailing bytes as whatever is left over after offset+length in the buffer.

@@ +250,5 @@
>  
>      nsresult rv = DecodeFrameHeader(buffer, &frameSize, &bitRate, &duration);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }

Yes, because we now use this to function to test if we found a sync frame, so we'd expect it to fail possibly hundreds of times before it finds sync. We don't need those warnings spamming our logs. ;)
* Fixed compile error on B2G.
* Removed trailing whitespace.
* Green: https://tbpl.mozilla.org/?tree=Try&rev=72afbdcd1b3d
Attachment #798632 - Attachment is obsolete: true
Attachment #798632 - Flags: review?(paul)
Attachment #799915 - Flags: review?(paul)
Comment on attachment 799915 [details] [diff] [review]
Patch 2v2: Use MP3FrameParser in DirectShow's SourceFilter

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

Sorry for the delay!

::: content/media/MP3FrameParser.cpp
@@ +413,1 @@
>      MOZ_ASSERT(trailing < (NS_ARRAY_LENGTH(mBuffer)*sizeof(*mBuffer)));

sizeof(mBuffer) / sizeof(mBuffer[0]) * sizeof(mBuffer[0]), I had missed this one the first time :-)

::: content/media/MP3FrameParser.h
@@ +60,4 @@
>  
> +  // Parses aBuffer, starting at offset 0. Returns the number of bytes
> +  // parsed, relative to the start of the buffer. Note this may be
> +  // greater than aLength if the headers in the buffer inidicate that

s/inidicate/indicate/

@@ +81,5 @@
>    uint64_t mBitRateSum;
>    uint64_t mNumFrames;
> +
> +  // Offset of the last data parsed. This is the end offset of the last data
> +  // blocked parsed, so it's the start offset we expect to get on the next

s/data blocked/data block/
Attachment #799915 - Flags: review?(paul) → review+
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a20a9a879347
https://hg.mozilla.org/integration/mozilla-inbound/rev/48a66737dc5c

And I also pushed a bustage fix after I forgot to qref before importing and pushing my patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acc75d3ef7c0

I'm making a great start to the week. I also managed to mislabel my changesets as belonging to bug 910966 instead of bug 910996. :(
https://hg.mozilla.org/mozilla-central/rev/a20a9a879347
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
This patch fixed the file not being decoded on Windows Vista/7, but it still does not play on Windows XP.  The console says: Media resource http://nxt-level.com/music/DJ%20Shadow%20feat.%20Mos%20Def%20-%20Six%20Days.mp3 could not be decoded.
Please ignore comment 11 about the file not working in Windows XP.  I forgot to update nightly before testing this on my XP VM. Sorry for any confusion.

Updated

6 years ago
Depends on: 919572
You need to log in before you can comment on or make changes to this bug.