Closed Bug 918135 Opened 11 years ago Closed 11 years ago

Improve MP3 stream duration estimate

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: eflores, Assigned: eflores)

References

Details

Attachments

(5 files, 7 obsolete files)

23.54 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
8.87 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.19 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
7.74 KB, patch
eflores
: review+
Details | Diff | Splinter Review
179.44 KB, patch
eflores
: review+
Details | Diff | Splinter Review
      No description provided.
Attached patch 918135.patch (obsolete) — Splinter Review
Attachment #806972 - Flags: review?(cpearce)
Comment on attachment 806972 [details] [diff] [review]
918135.patch

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

r+ with nits addressed.

When you file a bug you should say why you're doing what your doing, rather than having a blank comment 0. It's important to document decisions for future reference. Otherwise some jerk might come along in a few years time and not be able to figure out why we did this, and revert the change. That person might even be you.

::: content/media/VideoUtils.cpp
@@ +91,5 @@
>  }
>  
> +static const uint32_t READ_SIZE = 4096;
> +
> +uint64_t

This should be a int64_t, like all our other durations. In particular, since you're returning -1 here.

@@ +95,5 @@
> +uint64_t
> +GetEstimatedMP3Duration(mozilla::MediaResource *aResource,
> +                        mozilla::MP3FrameParser *aParser)
> +{
> +

if (!aResource || !aParser)
  return -1;

@@ +96,5 @@
> +GetEstimatedMP3Duration(mozilla::MediaResource *aResource,
> +                        mozilla::MP3FrameParser *aParser)
> +{
> +
> +  while (true) {

while (aParser->IsMP3())

::: content/media/apple/AppleMP3Reader.cpp
@@ +107,5 @@
>        return NS_ERROR_FAILURE;
>      }
>    } while(totalBytes < *aNumBytes && numBytes);
>  
>    mDecoder->NotifyBytesConsumed(totalBytes);

You won't need the NotiifyBytesConsumed() call (and don't add it back in!) now that Bug 915957 has landed.

@@ +523,5 @@
> +                                  int64_t aOffset)
> +{
> +  // As data enters the cache, pass it to the MP3 parser to estimate the
> +  // duration of the stream.
> +  uint64_t duration = GetEstimatedMP3Duration(mDecoder->GetResource(),

int64_t

@@ +526,5 @@
> +  // duration of the stream.
> +  uint64_t duration = GetEstimatedMP3Duration(mDecoder->GetResource(),
> +                                              &mMP3FrameParser);
> +
> +  if (duration && duration != mDuration) {

Did you mean:

if (duration != -1 && ...)

??
Attachment #806972 - Flags: review?(cpearce) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/66b219dd6519 for frequent (like, once or twice a push) OS X failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=28216761&tree=Mozilla-Inbound, "test_played.html | Start of first range should be the sixth of the duration - got 1.3486496666666667, expected 1.6718080000000002," "End of last range should be greater that five times the sixth of the duration."
Attached patch vbr.patch (obsolete) — Splinter Review
Ah. So there's a header that tells us what we need for VBR. Yeah, that's okay too I guess.

/facepalm

This patch tries to find a Xing or VBRI header and read the number of frames (and calculate the duration) from that. If the header isn't found it assumes CBR.
Attachment #806972 - Attachment is obsolete: true
Attachment #827042 - Flags: review?(cpearce)
Comment on attachment 827042 [details] [diff] [review]
vbr.patch

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

The duration is wrong on this file: http://www.nxt-level.com/music/4159_1332269164.mp3 WMF gives the duration as 5:29.

::: content/media/MP3FrameParser.cpp
@@ +239,5 @@
> +// Helper function to find a VBR header in an MP3 frame.
> +// Based on information from
> +// http://www.codeproject.com/Articles/8295/MPEG-Audio-Frame-Header
> +
> +const uint32_t VBRI_TAG = (uint32_t)'VBRI';

[16:29]	cpearce	edwin: does const uint32_t VBRI_TAG = (uint32_t)'VBRI'; actually do what you think it does, even on big endian systems? why doesn't the compiler treat 'VBRI' as a char?
[16:30]	edwin	cpearce: Good timing. GCC was just asking me that itself.
[16:30]	edwin	Except prepended with `error: '
[16:31]	cpearce	you probably want a macro like "FOURCC(a,b,c,d) (((a)<<24)|((b)<<16)|((c)<<8)|(d))"

@@ +267,5 @@
> +
> +  // We have to search for the Xing header as its position can change.
> +  for (; buffer + sizeof(XING_TAG) < bufferEnd; buffer++) {
> +    if (FROM_BIG_ENDIAN(buffer) == XING_TAG) {
> +      goto foundXing;

Apparently some people consider gotos harmful.

Instead create a function ParseXing(const char*), and `return ParseXing(buffer)` here. Or NumFramesFromXing(), or something appropriate.

@@ +285,5 @@
> +
> +  int64_t numFrames = -1;
> +  if (flags & XING_HAS_NUM_FRAMES) {
> +    numFrames = FROM_BIG_ENDIAN(buffer + 8);
> +    printf("num frames = %d\n", (int)numFrames);

Remove printf.
Attachment #827042 - Flags: review?(cpearce) → review-
Attached patch vbr.patch (obsolete) — Splinter Review
Un-dumbed it a bit.
Attachment #827042 - Attachment is obsolete: true
Attached patch vbr.patch v2 (obsolete) — Splinter Review
O, young goto. So young. You had not long on this earth; a victim of ruthless and undeserved discrimination. May you find peace and acceptance wherever you may now be, and may your sacrifice serve to enlighten others so that it will not have been in vain.

I _will_ avenge you.

RIP.
Attachment #827173 - Attachment is obsolete: true
Attachment #827188 - Flags: review?(cpearce)
Comment on attachment 827188 [details] [diff] [review]
vbr.patch v2

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

A moving eulogy, but I still get a duration of 29:14 on http://www.nxt-level.com/music/4159_1332269164.mp3 when I re-enable the directshow backend with this patch applied.
Attachment #827188 - Flags: review?(cpearce) → review-
(In reply to Edwin Flores [:eflores] [:edwin] from comment #4)
> Created attachment 827042 [details] [diff] [review]
> vbr.patch
> 
> Ah. So there's a header that tells us what we need for VBR. Yeah, that's
> okay too I guess.
> 
> /facepalm
> 
> This patch tries to find a Xing or VBRI header and read the number of frames
> (and calculate the duration) from that. If the header isn't found it assumes
> CBR.

You cannot assume CBR. This is what Android does and it doesn't work because there are VBR files without Xing and VBRI headers. That's actually the reason why the MP3 frame parser was created in the first place.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> You cannot assume CBR. This is what Android does and it doesn't work because
> there are VBR files without Xing and VBRI headers. That's actually the
> reason why the MP3 frame parser was created in the first place.

Is this a large enough proportion of MP3 files out there? or is there a more specific use case here that we need to account for?

All the decoders I've looked at so far seem to make the same assumptions I've made here. I can accept being just as wrong as them unless we know something they ostensibly don't.
Attached patch 918135-windows.patch (obsolete) — Splinter Review
+ stuff that makes this go on Windows.
Attachment #830558 - Flags: review?(cpearce)
Note that you actually need to remove the "(WinUtils::GetWindowsVersion() < WinUtils::VISTA_VERSION)" check in DirectShowDecoder::IsEnabled() for this code to be used on Windows.

Sorry dude, with these two patches applied sometimes the duration is still off, sometimes only by 1s, other times by more:

http://www.jmccanneyscience.com/JamesMcCanneyScienceHour_September_12_2013.mp3
With your patches: 58:15
VLC/WMP: 59:03

http://nxt-level.com/music/DJ%20Shadow%20feat.%20Mos%20Def%20-%20Six%20Days.mp3
Your patches: 3:56
VLC/WMP: 3:55

http://79.142.89.165/nagrani/20130910_-_55_Krutushka-2013.mp3
Your patches: 1:00:26
VLC: 1:00:26
WMP: 1:00:27

The off-by-one's could be rounding differences, but the science hour MP3's duration is off by a more significant difference. What's happening with that MP3?
(In reply to Edwin Flores [:eflores] [:edwin] from comment #10)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> > You cannot assume CBR. This is what Android does and it doesn't work because
> > there are VBR files without Xing and VBRI headers. That's actually the
> > reason why the MP3 frame parser was created in the first place.
> 
> Is this a large enough proportion of MP3 files out there?

How should I know? At least I have MP3 files here that where recently encoded and contain VBR without any extra headers. The question should rather be whether the proportion of VBR files is small enough to justify removing this feature.

> or is there a more
> specific use case here that we need to account for?

Yes, there is at least bug 783512. If the duration of an MP3 is incorrect, you'll only here half of the song.

> All the decoders I've looked at so far seem to make the same assumptions
> I've made here.

I've noticed wrong duration in other media players as well.

Regards
Thomas
(In reply to Chris Pearce (:cpearce) from comment #12)
> Note that you actually need to remove the "(WinUtils::GetWindowsVersion() <
> WinUtils::VISTA_VERSION)" check in DirectShowDecoder::IsEnabled() for this
> code to be used on Windows.

Yes. That's in another patch. Are we using DirectShow everywhere after all, or...?

> Sorry dude, with these two patches applied sometimes the duration is still
> off, sometimes only by 1s, other times by more:
> 
> http://www.jmccanneyscience.com/JamesMcCanneyScienceHour_September_12_2013.
> mp3
> With your patches: 58:15
> VLC/WMP: 59:03
> 
> The off-by-one's could be rounding differences, but the science hour MP3's
> duration is off by a more significant difference. What's happening with that
> MP3?

Sample size = 1 is what happened there.
Comment on attachment 830558 [details] [diff] [review]
918135-windows.patch

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

Just so we're clear, r- until the science hour MP3 duration is correct.
Attachment #830558 - Flags: review?(cpearce) → review-
I also ran into some durations not being detected and just returning Infinity.

Test case: http://jsfiddle.net/j79LB/
Duration properly reported for: https://dl.dropboxusercontent.com/u/2743265/a.mp3
Duration reported as Infinity for: https://dl.dropboxusercontent.com/u/2743265/b.mp3

This is on OS X with: Firefo UX Nightly 28.0a1 (2013-11-18). Testing with Chrome seems to show the correct duration for both.
(In reply to Luqman Aden [:Luqman] from comment #16)
> Duration reported as Infinity for:
> https://dl.dropboxusercontent.com/u/2743265/b.mp3

This file has some pretty jumbo ID3 tags (around 480KB). After we have read enough of the file without having found any MP3 frames, we give up and say it isn't an MP3 file.

Since we can parse ID3, we should probably not be counting ID3 headers towards that non-MP3 total.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #14)
> > Sorry dude, with these two patches applied sometimes the duration is still
> > off, sometimes only by 1s, other times by more:
> > 
> > http://www.jmccanneyscience.com/JamesMcCanneyScienceHour_September_12_2013.
> > mp3
> > With your patches: 58:15
> > VLC/WMP: 59:03

I can't seem to find anything wrong here.

MP3FrameParser estimates 97,071 MP3 frames of 73 bytes each with 576 samples per frame, at a sample rate of 16KHz.

mediainfo seems to disagree, reporting 98,419 frames. To fit into the file (7,086,203 bytes long), they would have to be 72 bytes each. I've xxd'ed the file and looked at it, and as far as I could be bothered looking they're all 73 bytes long; further all headers report being 73 bytes long as parsed by MP3FrameParser.
(In reply to Edwin Flores [:eflores] [:edwin] from comment #17)
> (In reply to Luqman Aden [:Luqman] from comment #16)
> > Duration reported as Infinity for:
> > https://dl.dropboxusercontent.com/u/2743265/b.mp3
> 
> This file has some pretty jumbo ID3 tags (around 480KB).

That's probably because it embeds the album artwork.
Attached patch vbr.patch v3Splinter Review
Alright, let's do this again.

This new patch is much the same as v2, but duration estimation is back in case we get a VBR MP3 without one of the proper header formats.

It also doesn't count ID3 headers towards skipped bytes, so we shouldn't end up rejecting MP3s with giant ID3 sections at the beginning.

Windows part yet to come.
Attachment #827188 - Attachment is obsolete: true
Attachment #8334974 - Flags: review?(cpearce)
I will review this when the Windows part comes, so I can test it.
Attached patch vbr-windows.patch (obsolete) — Splinter Review
Attachment #830558 - Attachment is obsolete: true
Attachment #8335720 - Flags: review?(cpearce)
Comment on attachment 8334974 [details] [diff] [review]
vbr.patch v3

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

Great! Now we must have a test to prevent this regressing.

::: content/media/MP3FrameParser.cpp
@@ +484,5 @@
>    // Update next data offset
>    mOffset = offset + bytesRead;
>  
>    // If we've parsed lots of data and we still have nothing, just give up.
> +  // We don't count ID3 headers towards that count, as MP3 files can have

"We don't count ID3 headers towards that count" - which count? mOffset? The English language gives ample opportunity to be ambiguous. Don't be.

@@ +496,5 @@
>  int64_t MP3FrameParser::GetDuration()
>  {
>    MutexAutoLock mon(mLock);
>  
> +  if (mMP3Offset < 0) {

You divide by mSamplesPerSecond below, so you'd better return error here too if  mSamplesPerSecond==0. Ditto for mFrameCount, and mTotalFrameSize (if mTotalFrameSize==0, then frameSize will be 0f).

@@ +505,5 @@
> +  if (mNumFrames < 0) {
> +    // Estimate the number of frames in the stream based on the average frame
> +    // size and the length of the MP3 file.
> +    double frameSize = (double)mTotalFrameSize / mFrameCount;
> +    frames = (double)(mLength - mMP3Offset) / frameSize;

frameSize could be exactly zero here if mTotalFrameSize is zero, right? So we'd better zero-check mTotalFrameSize above.
Attachment #8334974 - Flags: review?(cpearce) → review+
Attachment #8335720 - Flags: review?(cpearce) → review+
Attached patch vbr-tests.patch (obsolete) — Splinter Review
Added tests for VBR header support and ID3 skipping.
Attachment #8339028 - Flags: review?(cpearce)
Attachment #8339028 - Flags: review?(cpearce) → review+
Attached patch vbr-linux.patchSplinter Review
+ gstreamer.
Attachment #8340166 - Flags: review?(cpearce)
Lower the ridiculous non-MP3 data threshold now that we skip ID3 tags.
Attachment #8340167 - Flags: review?(cpearce)
Changed

#define FROM_BIG_ENDIAN(X) (...)

to

#define FROM_BIG_ENDIAN(X) ((uint32_t) (...))

and suddenly, green!

https://tbpl.mozilla.org/?tree=Try&rev=78264e70ba80
De-orange DirectShow. Forgot |offset += bytesRead;| in DirectShowReader::ParseMP3Headers.

Carry over r=cpearce
Attachment #8335720 - Attachment is obsolete: true
Attachment #8340838 - Flags: review+
Attached patch vbr-tests.patchSplinter Review
Replaced absurdly short MP3 test file with a slightly longer one to keep the DirectShow backend happy. Other than that, essentially the same test.

Carry over r=cpearce.
Attachment #8339028 - Attachment is obsolete: true
Attachment #8340839 - Flags: review+
Now that I've mixed up the patch order, it goes:
1. vbr.patch
2. vbr-windows.patch
3. vbr-tests.patch
4. vbr-linux.patch
5. vbr-id3-threshold.patch

(really the last 4 should be independent, but it's what my queue looks like)
Attachment #8340166 - Flags: review?(cpearce) → review+
Comment on attachment 8340167 [details] [diff] [review]
vbr-id3-threshold.patch

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

Ship it!
Attachment #8340167 - Flags: review?(cpearce) → review+
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: