1291543 - Wrong calculation of duration for some MP3 files

Status: RESOLVED FIXED

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

Description

[Loic]

Issue reported here: https://support.mozilla.org/en-US/questions/1133234

STR:
1) Open http://www.djjaybird.com/
2) Check the total duration of both audio files.
NB: direct link e.g. http://www.djjaybird.com/data/.onelove.mp3

Result: 53:08 & 46:40 instead of 05:17 & 04:39.

Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=532a99cadead9024d6a2ad7715d81016e8f5949e&tochange=1e038d4d718fa1b758147b7d9f32072fe9bfe1cd

Jan Henning — Bug 1263334 - Check VBR header is valid before using it for duration calculations. r=esawin

Comment 1

[jscher2000]

The mp3 files on that page are encoded with a bitrate of 320 kbps but VLC player shows an initial read of 32 kbps. Is Firefox making the same mistake, or if this is a glitch in the file, can it be compensated for?

Comment 2

[Sylvestre Ledru [:Sylvestre]]

This is not a recent regression, we won't do a dot release for this. 
And maybe it is just a badly formatted file...

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1291543-1.1-Accept-partial-information-from-VBR-.patch

The file contains a XING header which includes information on the total number of frames, but not the total frame size. This is probably uncommon but nonetheless valid.

VBRHeader::IsValid is too strict to support this case so we discard the XING header and use the average frame length to base our duration computation. In this case this goes wrong, since the first frame is only 104 bytes long (the XING header) so our estimation after parsing 1 frame is off.

With this patch, we accept partial VBR header information instead of discarding it.

We can also further improve the frame-length-based duration estimation by probing some frames ahead in the case where Duration() is called right at the beginning, but I would prefer to move that out into a new bug.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8777330 [details] [diff] [review]
0001-Bug-1291543-1.1-Accept-partial-information-from-VBR-.patch

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

::: dom/media/MP3Demuxer.cpp
@@ +369,4 @@
>  
>    int64_t numFrames = 0;
>    const auto numAudioFrames = mParser.VBRInfo().NumAudioFrames();
> +  if (mParser.VBRInfo().IsValid() && numAudioFrames.valueOr(0) + 1 > 1) {

wouldn't >= 1 be simpler?

@@ +694,4 @@
>      return static_cast<double>(mTotalFrameLen) / mNumParsedFrames;
>    }
>    const auto& vbr = mParser.VBRInfo();
> +  if (vbr.IsComplete() && vbr.NumAudioFrames().value() + 1) {

IsComplete() returns true only if mNumAudioFrames.valueOr(0) is > 0

so vbr.NumAudioFrames().value() + 1 will always be true no?

@@ +1039,5 @@
> +}
> +
> +bool
> +FrameParser::VBRHeader::IsComplete() const {
> +  return IsValid() &&

I'm not sure this goes with the comment.
You write that IsValid() is too strict.

but you only made the condition even more strict.

And wouldn't IsComplete returns true only when mNumBytes is > 0 ?

So either there's something not quite right with the logic, or the comment needs logic.

Comment 5

[Eugen Sawin [:esawin]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> > +  if (mParser.VBRInfo().IsValid() && numAudioFrames.valueOr(0) + 1 > 1) {
> 
> wouldn't >= 1 be simpler?

> IsComplete() returns true only if mNumAudioFrames.valueOr(0) is > 0
> 
> so vbr.NumAudioFrames().value() + 1 will always be true no?

These conditions are meant to implicitly check for overflow to, e.g., avoid dividing by zero.

> I'm not sure this goes with the comment.
> You write that IsValid() is too strict.
> 
> but you only made the condition even more strict.
> 
> And wouldn't IsComplete returns true only when mNumBytes is > 0 ?

The original IsValid was too strict since it would return false for valid headers with partial information, so I renamed it to IsComplete and made IsValid just check for validity, i.e., whether it is a well-formed VBR header.

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8777330 [details] [diff] [review]
0001-Bug-1291543-1.1-Accept-partial-information-from-VBR-.patch

Since the duration estimation worked before I tightened down the VBR header parsing, this seems reasonable.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be293e09860e
[1.1] Accept partial information from VBR headers. r=jya

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Marcia Knous [:marcia]]

Tracking 51+ for this regression.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to request uplift for this?

Comment 11

[Eugen Sawin [:esawin]]

Comment on attachment 8777330 [details] [diff] [review]
0001-Bug-1291543-1.1-Accept-partial-information-from-VBR-.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1263334
[User impact if declined]: Some MP3 streams may show incorrect total durations.
[Describe test coverage new/current, TreeHerder]: Local and Nightly.
[Risks and why]: Minimal, it's a basic code change.
[String/UUID change made/needed]: None.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8777330 [details] [diff] [review]
0001-Bug-1291543-1.1-Accept-partial-information-from-VBR-.patch

Taking it to aurora & beta to fix this regression.

Can we add a test to make sure it doesn't regress?

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0c747cd335a7

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/352cf7e1c267

Comment 15

[Eugen Sawin [:esawin]]

Adding regression test in bug 1324789.