910996 - Another MP3 that doesn't play in DirectShow backend

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1v1: Make MP3 accessible outside of B2G code

Make MP3FrameParser usable outside of B2G code.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2v1: Use MP3FrameParser in DirectShow's SourceFilter

* 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!

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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. ;)

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2v2: Use MP3FrameParser in DirectShow's SourceFilter

* Fixed compile error on B2G.
* Removed trailing whitespace.
* Green: https://tbpl.mozilla.org/?tree=Try&rev=72afbdcd1b3d

Comment 7

[Paul Adenot (:padenot)]

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/

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

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. :(

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a20a9a879347

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/48a66737dc5c
https://hg.mozilla.org/mozilla-central/rev/acc75d3ef7c0

Comment 11

[Trevor Rowbotham [:rowbot]]

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.

Comment 12

[Trevor Rowbotham [:rowbot]]

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.