1419736 - mp3 time length bug

Status: RESOLVED FIXED

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

Description

[Carlos]

Attachment: browsers.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

I request an url containing a mp3 file audio.  


Actual results:

The total duration is not well calculated. 
Microsoft Edge 38.14393.1066.0 is well.
Google Chrome Versión 62.0.3202.94 (Build oficial) (64 bits) is well.
MediaInfo 17.10 Copyright (c) MediaArea.net SARL is well.

Firefox Quatum v.57.0 (64-bit) is wrong. 

May be a bug by using older version of mediainfo to calculate length because in older version than 17.10 the duration is not well calculcated in one of the fields description. 




Expected results:

It should be the same time legth that the file audio.

Comment 1

[Blake Wu [:bwu][:blakewu]]

Thanks for reporting this bug. 
Can you provide us your test file?

Comment 2

[Carlos]

Hi,

You can check it out in

http://sdmedia.playser.cadenaser.com/2017/11/22/cadenaser_hablarporhablar_20171122_030000_040000.mp3

Regards.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

There are a number of cases where MP3 durations can't be reported accurately such as VBR. I'm therefore calling this a P3.

Comment 4

[Carlos]

In this case, the audio file has CBR.

Comment 5

[C.M.Chang[:chunmin]]

(In reply to Carlos from comment #2)
> http://sdmedia.playser.cadenaser.com/2017/11/22/cadenaser_hablarporhablar_20171122_030000_040000.mp3
The length varies from frame to frame. It could be 417 or 418 in this case[0]. The total size is 55458429(bytes). The duration shown on control bar is calculated upon we got our first frame[1]. At that time, the total frames contained in this file is roughly estimated by 55458429 / 417 = 132993[2, 3]. Then we will use 132993, which is the total number of frames, to calculate the duration by `duration (sec.) = frames * samples-in-frame / sample-rate (samples / sec.)`. We will get `57 : 54` in this case[4].

However, most frame length in the mp3 is 418 instead of 417 and the average of the frame length is 417.939024. Thus, the total frames should be 55458429 / 417.94 = 132694, instead of 132993, and the duration will be revised to `57 : 46`, which is same as the duration given by other browsers. 

The problem is that we need to use *average frame length* to calculate the duration, and the length of the first frame may be different with the average. With this approach, tiny difference in frame length can lead to a huge gap in duration. The larger the size is, the bigger gap of the duration is. 

The calculation should not rely on *average frame length*, at least in early stage. In fact, the duration can be calculated by

duration (sec.) = size(btyes) * 8 (to bits) / bit-rate (bits / sec.).

I applied it in this case and it works(duration = 57 : 46).

Alfredo, is it ok to modify the calculation?


[0] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/dom/media/mp3/MP3Demuxer.cpp#720
[1] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/dom/media/MediaDecoderStateMachine.cpp#2165,2175
[2] Actually, we will subtract first-frame-offset before dividing. See[3].
[3] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/dom/media/mp3/MP3Demuxer.cpp#395
[4] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/dom/media/mp3/MP3Demuxer.cpp#395,398,401-410

Comment 6

[Alfredo Yang (:alfredo)]

(In reply to Chun-Min Chang[:chunmin] from comment #5)
> (In reply to Carlos from comment #2)
> > http://sdmedia.playser.cadenaser.com/2017/11/22/cadenaser_hablarporhablar_20171122_030000_040000.mp3
> The length varies from frame to frame. It could be 417 or 418 in this
> case[0]. The total size is 55458429(bytes). The duration shown on control
> bar is calculated upon we got our first frame[1]. At that time, the total
> frames contained in this file is roughly estimated by 55458429 / 417 =
> 132993[2, 3]. Then we will use 132993, which is the total number of frames,
> to calculate the duration by `duration (sec.) = frames * samples-in-frame /
> sample-rate (samples / sec.)`. We will get `57 : 54` in this case[4].
> 
> However, most frame length in the mp3 is 418 instead of 417 and the average
> of the frame length is 417.939024. Thus, the total frames should be 55458429
> / 417.94 = 132694, instead of 132993, and the duration will be revised to
> `57 : 46`, which is same as the duration given by other browsers. 
> 
> The problem is that we need to use *average frame length* to calculate the
> duration, and the length of the first frame may be different with the
> average. With this approach, tiny difference in frame length can lead to a
> huge gap in duration. The larger the size is, the bigger gap of the duration
> is. 
> 
> The calculation should not rely on *average frame length*, at least in early
> stage. In fact, the duration can be calculated by
> 
> duration (sec.) = size(btyes) * 8 (to bits) / bit-rate (bits / sec.).
> 
> I applied it in this case and it works(duration = 57 : 46).
> 
> Alfredo, is it ok to modify the calculation?

Hmm... it wastes time to scan the whole file for average frame size. Maybe we could check how chromium calculates it?

Comment 7

[C.M.Chang[:chunmin]]

The `duration`[0] works with `time_base`[1] in FFmpeg(chrome)[2]. If the `time_base` is `1/sample-rate` then the `duration` is `total-samples`. If the `time_base` is `1/bit-rate` then the `duration` is `total-bits-size`. The *duration in seconds* will be calculated by `duration * time_base`.

Not sure how ffmpeg use sample-rate or bit-rate in this case. Still tracing their code.

[0] https://github.com/FFmpeg/FFmpeg/blob/5a366f9770dd7b02b0721b2857d6baa96acdb0af/libavformat/avformat.h#L904-L912
[1] https://github.com/FFmpeg/FFmpeg/blob/5a366f9770dd7b02b0721b2857d6baa96acdb0af/libavformat/avformat.h#L880-L892
[2] https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavformat/avformat.h?rcl=18c815f81428e3b41e6f4efc6cb0cbe5b846d1c2&l=880-892,904-912

Comment 8

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #7)
> Not sure how ffmpeg use sample-rate or bit-rate in this case. Still tracing
> their code.
FFmpeg will use bit-rate to calculate the duration of the file[0]. 

We should also use bit-rate to calculate the duration when there is no enough samples to estimate. It would be better if we have a way to update the duration in case the file is VBR.

[0] https://github.com/ChunMinChang/FFmpeg/blob/master/libavformat/utils.c#L2705-L2707

Comment 9

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #8)
> [0] https://github.com/ChunMinChang/FFmpeg/blob/master/libavformat/utils.c#L2705-L2707
Permalink:
[0] https://github.com/ChunMinChang/FFmpeg/blob/c8bd2c7d09bfc71795e91f0a9aeb4b8a6833eff2/libavformat/utils.c#L2705-L2707

Comment 10

[C.M.Chang[:chunmin]]

Attachment: Bug 1419736 - Calculate the mp3 duration by bitrate if it's CBR;

Review commit: https://reviewboard.mozilla.org/r/206502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206502/

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8935620 [details]
Bug 1419736 - Calculate the mp3 duration by bitrate if it's CBR;

https://reviewboard.mozilla.org/r/206502/#review212226

Comment 12

[Eugen Sawin [:esawin]]

Comment on attachment 8935620 [details]
Bug 1419736 - Calculate the mp3 duration by bitrate if it's CBR;

https://reviewboard.mozilla.org/r/206502/#review212996

We can not classify files based on (missing) VBR headers.
VBR headers are optional and some encoders add VBR headers to CBR files.

Practically speaking, this would probably work better for the majority of files, but it's based on a non-standard assumption.
Cancelling review since I don't have strong opinion on it.

Comment 13

[C.M.Chang[:chunmin]]

Comment on attachment 8935620 [details]
Bug 1419736 - Calculate the mp3 duration by bitrate if it's CBR;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206502/diff/1-2/

Comment 14

[Eugen Sawin [:esawin]]

Comment on attachment 8935620 [details]
Bug 1419736 - Calculate the mp3 duration by bitrate if it's CBR;

https://reviewboard.mozilla.org/r/206502/#review213210

::: dom/media/mp3/MP3Demuxer.cpp:387
(Diff revisions 1 - 2)
>  
>    // Calculate the duration by bitrate directly if it's CBR.
>    if (mParser.VBRInfo().Type() == FrameParser::VBRHeader::NONE) {
>      const int64_t size = StreamLength() - mFirstFrameOffset;
>      const int32_t bitrate = mParser.CurrentFrame().Header().Bitrate();
> -    return media::TimeUnit::FromSeconds(size * 8 / bitrate);
> +    return media::TimeUnit::FromSeconds((double)size * 8 / bitrate);

Use static_cast instead of C-style casts. In this case an implicit cast would be enough, by declaring size as double.

Comment 15

[C.M.Chang[:chunmin]]

Comment on attachment 8935620 [details]
Bug 1419736 - Calculate the mp3 duration by bitrate if it's CBR;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206502/diff/2-3/

Comment 16

[C.M.Chang[:chunmin]]

(In reply to Eugen Sawin [:esawin] from comment #14)
> Comment on attachment 8935620 [details]
> Bug 1419736 - Calculate the mp3 duration by bitrate if it's CBR;
> 
> https://reviewboard.mozilla.org/r/206502/#review213210
> 
> ::: dom/media/mp3/MP3Demuxer.cpp:387
> (Diff revisions 1 - 2)
> >  
> >    // Calculate the duration by bitrate directly if it's CBR.
> >    if (mParser.VBRInfo().Type() == FrameParser::VBRHeader::NONE) {
> >      const int64_t size = StreamLength() - mFirstFrameOffset;
> >      const int32_t bitrate = mParser.CurrentFrame().Header().Bitrate();
> > -    return media::TimeUnit::FromSeconds(size * 8 / bitrate);
> > +    return media::TimeUnit::FromSeconds((double)size * 8 / bitrate);
> 
> Use static_cast instead of C-style casts. In this case an implicit cast
> would be enough, by declaring size as double.
Thanks for the hints! 

Hi :JanH,
I was wondering if you could tell me how you get the duration of id3v2header.mp3.
Our estimation is 3166167μs. It's same as FFmpeg's calculation but our answer in the test is 3160816μs[0].

[0] https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/dom/media/gtest/TestMP3Demuxer.cpp#140

Comment 17

[Jan Henning [:JanH]]

I can't really remember, but the goal of that file was to test the ID3 header handling, not the length estimation. So it's possible that I simply took whatever number our parser (apparently somewhat inexactly) estimated and wrote that into the test, in which case feel free to replace this with a more precise number.

Comment 18

[C.M.Chang[:chunmin]]

The current calculation[0] allow us to estimate the duration for a VBR mp3 with predefined numAudioFrames, even the StreamLength() is -1. The returned -1 from StreamLength() means the mp3 is a live stream. To verify this case, I randomly checked some live radios to see if there are live streams containing predefined numAudioFrames. Finally I found an example:

http://honey.macchiatomedia.org:8080/stream/1/?lang=en-US%2cen%3bq%3d0.5

1) Go to https://www.shoutcast.com/
2) Select `Jazz in `Genre` 
3) Play `KHNY Honey 103`

Before playing music, the radio will give a 9-seconds opening remark: "You are now listening to KHNY Honey 103, .....". The 9 seconds can be calculated by the predefined numAudioFrames[1], so we really need to check numAudioFrames, regardless of returned value of StreamLength(). The estimation is useful for the live radios with opening mark. After opening introduction, the duration will keep increasing during playing music as usual.

[0] https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/media/mp3/MP3Demuxer.cpp#390-404
[1] https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/media/mp3/MP3Demuxer.cpp#393,404

Comment 19

[C.M.Chang[:chunmin]]

Comment on attachment 8935620 [details]
Bug 1419736 - Calculate the mp3 duration by bitrate if it's CBR;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206502/diff/3-4/

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7a72db9069b
Calculate the mp3 duration by bitrate if it's CBR; r=jya

Comment 22

[Cosmin Sabou [:CosminS]]

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