1070485 - After forward MP3 VBR plays longer than its actual length

Status: RESOLVED FIXED

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

Description

[Ivan]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140917194002

Steps to reproduce:

1. Press the play, to start playback of tracks;
2. Forward the song closer to the end (for example, 04:05);
3. Wait;
4. Magic (only vbr);
5. Result;

Demo: http://b23.ru/780w


Actual results:

After forward the song will play longer than its actual length.


Expected results:

The song must be completed in its actual length. Forwards should work correctly.

Comment 1

[Ivan]

Bug is only reproduced on Windows.

Comment 2

[Loic]

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8648aa476eef&tochange=9688476c1544

Suspected bug:
Chris Pearce — Bug 945947 - Re-enable DirectShow for MP3 playback on Vista and later. r=ajones We disabled MP3 playback on Windows using DirectShow as the duration was being incorrectly reported. While DirectShow was disabled, we supported MP3 playback via Windows Media Foundation, and we still used DirectShow on WinXP. Now that bug 918135 has landed the duration calculation has been fixed, so we can re-enable DirectShow for MP3 playback for all Windows versions. This enables us to avoid suffering from bug 882537 for MP3 on Windows.

Comment 3

[Loic]

Attachment: vbr.mp3

Comment 4

[Ivan]

Do you need some action/info from me? Can I help with anything?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for reporting this bug. We'll get to it.

Comment 6

[Sergey Radionov]

Hi,

I've investigated the problem a little and think problem is in

https://hg.mozilla.org/mozilla-central/file/4b3d816c21fd/content/media/directshow/DirectShowReader.cpp#l388

IMediaSeeking::SetPositions just can't do precise seeking on vbr files.

I think one of possible solutions is to try use seeking by samples ( IMediaSeeking::SetTimeFormat + TIME_FORMAT_SAMPLE ) since anyway MP3FrameParser used for length calculation, and length of single sample is available there.

please correct me if I wrong.

Comment 7

[Bountysource]

There's a $1000 bounty posted on this issue via Bountysource: https://www.bountysource.com/issues/5099229-after-forward-mp3-vbr-plays-longer-than-its-actual-length

Comment 8

[Loic]

I think it's the same issue with this another VBR MP3:
http://nigelcoldwell.co.uk/audio/mp3vbr/abr032.mp3

Seeking to the three-quarters of the duration (approx. t=30s) make the video end prematurely.

Comment 9

[SSE4]

Attachment: 1070485.patch

unfortunately, MPEG-1 Stream Splitter Filter (http://msdn.microsoft.com/en-us/library/windows/desktop/dd390713(v=vs.85).aspx) supports only TIME_FORMAT_MEDIA_TIME time format for seeking (no TIME_FORMAT_SAMPLE/TIME_FORMAT_BYTE), and it's docummented that such navigation can be imprecise: Because MPEG-1 content is not indexed, seeking can be very approximate. It is usually good for a fixed bitrate MPEG-1 system stream (which is usually hardware generated for video CD).
in other words - there is no way to navigate filter to specified byte or sample, it can be navigate only to media time, but in that case it will be approximate position, which causes file to play from incorrect part and end too early / too late.
therefore, the best we can do is to seek into zero (start positon) and skip samples until we got into target time.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8542203 [details] [diff] [review]
1070485.patch

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

I would prefer to not have to re-decode the entire MP3 stream from offset 0; for big MP3 that could be costly, or at least add significant latency.

While data is being downloaded, we get regular calls to DirectShowReader::NotifyDataArrived(). We parse the MP3 frames using our MP3FrameParser in order to get a better estimate of the duration. Can we extend the MP3FrameParser to index the data that streams in over the network in DirectShowReader::NotifyDataArrived(), and use that index to seek the stream? Then we won't need to re-decode the entire stream to seek.

Currently what we do for MP3 support is we use both the platforms' MP3 stream parser and MP3 decoder. Our long term strategy with MP3 is to write a platform independent MP3 stream parser and then only use the platform's decoder to decode the compressed MP3 data we parse out of the stream. We're going to make a first attempt at this for Android specifically in bug 1093815, as our existing MP3 solution doesn't work on new Android Lollipop devices at all.

I don't know when we'll get the time to make the Android solution work on other platforms, but if we add indexing and seeking to MP3FrameParser, we'll be working towards this goal.

::: dom/media/MP3FrameParser.h
@@ +110,5 @@
>      return mIsMP3 != NOT_MP3;
>    }
>  
> +  bool IsVBR() {
> +	  MutexAutoLock mon(mLock);

Please follow the prevailing style when modifying code; so use spaces instead of tabs to indent code.

Comment 11

[Matt Basta [:basta]]

This seems to no longer be an issue.

- I tested on Firefox 53 on Windows 10 and encountered no issues with the VBR file provided.
- I tested on Firefox 46 on macOS with issues seeking towards the end of the file, but this is not unexpected for VBR files.
- I also tested on Firefox 53 on macOS with no problems at all (even seeking).

I would say this is worth closing as resolved.