933091 - MP3FrameParser is not used.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Bruce Sun [:brsun]]

This is a regression bug. MP3FrameParser is not used after applying attachment 816973 [details] [diff] [review] from bug 831645.

Need to add it back in order to estimate the duration of VBR MP3 files which do not have Xing or VBRI headers.

Comment 1

[Jason Smith [:jsmith]]

What's the impact of this bug?

Comment 2

[Bruce Sun [:brsun]]

In short, the playback duration of VBR MP3 might be wrong.

By using MP3FrameParser to parse the whole file, we can get the correct number of total frames of any mp3 file. And the correct duration can be calculated by (Number of Frames) * (Samples Per Frame) / (Sampling Rate). (p.s. although MP3FrameParser can count correct number of total frames, it uses some estimation to calculate the duration btw.)

MP3Extractor calculate the duration by the following information:
 - If there is Xing header in the file, calculate it from (Number of Frames) information in Xing header.
 - If there is VBRI header in the file, calculate if from (Number of Frames) information in VBRI header.
 - Otherwise, calculate it simply by dividing file size with the bitrate information parsed so far.

If we simply use MP3Extractor to calculate duration, the estimated duration of a VBR MP3 file, which doesn't have a Xing or VBRI header, might be wrong if the bitrate changes dramatically. I think MP3FrameParser is designed to overcome this kind of duration problem.

ref. bug 831224

Comment 3

[Bruce Sun [:brsun]]

But many VBR MP3 files have Xing or VBRI headers btw.

Comment 4

[Jason Smith [:jsmith]]

Okay. This is a regression from a 1.3 RTSP feature that affects getting the correct playback duration on certain mp3 files - probably worth blocking on.

Comment 5

[Bruce Sun [:brsun]]

Attachment: bug933091_regression_mp3frameparser_is_not_used.patch

Re-enable MP3FrameParser to parse MP3 duration.

Comment 6

[Sotaro Ikeda [:sotaro]]

Comment on attachment 825115 [details] [diff] [review]
bug933091_regression_mp3frameparser_is_not_used.patch

Looks good. Code change around Media playback needs to be reviewed by doublec.

Comment 7

[Bruce Sun [:brsun]]

Attachment: bug933091_regression_mp3frameparser_is_not_used.check_in.patch

Ready to check-in. Based on attachment 825115 [details] [diff] [review] with additional committing message to note reviewers. The results of attachment 825115 [details] [diff] [review] on the try server are all green: https://tbpl.mozilla.org/?tree=Try&rev=dd22bce26287.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/aaed0418059d

Comment 9

[Ed Morley [:emorley]]

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

Comment 10

[Jason Smith [:jsmith]]

Setting to 1.3+ for being a regression from the RTSP work for mp3 playback