Closed Bug 1291543 Opened 3 years ago Closed 3 years ago

Wrong calculation of duration for some MP3 files

Categories

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

46 Branch
x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 - wontfix
firefox49 + fixed
firefox50 + fixed
firefox51 + fixed

People

(Reporter: epinal99-bugzilla2, Assigned: esawin)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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
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?
This is not a recent regression, we won't do a dot release for this. 
And maybe it is just a badly formatted file...
Assignee: nobody → esawin
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.
Flags: needinfo?(jh+bugzilla)
Attachment #8777330 - Flags: review?(jyavenard)
Attachment #8777330 - Flags: feedback?(jh+bugzilla)
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.
Attachment #8777330 - Flags: review?(jyavenard) → review+
(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 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.
Attachment #8777330 - Flags: feedback?(jh+bugzilla) → feedback+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be293e09860e
[1.1] Accept partial information from VBR headers. r=jya
https://hg.mozilla.org/mozilla-central/rev/be293e09860e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Tracking 51+ for this regression.
Want to request uplift for this?
Flags: needinfo?(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.
Flags: needinfo?(esawin)
Attachment #8777330 - Flags: approval-mozilla-beta?
Attachment #8777330 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(esawin)
Attachment #8777330 - Flags: approval-mozilla-beta?
Attachment #8777330 - Flags: approval-mozilla-beta+
Attachment #8777330 - Flags: approval-mozilla-aurora?
Attachment #8777330 - Flags: approval-mozilla-aurora+
Blocks: 1324789
Adding regression test in bug 1324789.
Flags: needinfo?(esawin)
You need to log in before you can comment on or make changes to this bug.