910897 - New DirectShow backend reports incorrect duration for MP3 file

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Trevor Rowbotham [:rowbot]]

If you open the URL in the bug report with the latest Firefox nightly on Windows, the media player will report that the MP3 file as having a duration of 29:14.  However, the file should have a duration of 5:29 give or take a second depending on which player/browser you try to play it in.  Setting media.directshow.enabled = false causes Firefox to report the correct duration.

Comment 1

[Loic]

Regression range:
good=2013-08-13
bad=2013-08-14
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c146d402a55f&tochange=3c61cc01a3b1

Suspected bug:
Bug 861693 - DirectShow backend for MP3 decoding on WinXP

For the record:

General
Complete name                            : 4159_1332269164.mp3
Format                                   : MPEG Audio
File size                                : 10.0 MiB
Duration                                 : 5mn 28s
Overall bit rate mode                    : Variable
Overall bit rate                         : 256 Kbps
Encoded date                             : UTC 2012-03- 6 10:09:19
Writing library                          : LAME3.98.4
major_brand                              : mp42
minor_version                            : 0
compatible_brands                        : isommp42

Audio
Format                                   : MPEG Audio
Format version                           : Version 1
Format profile                           : Layer 3
Mode                                     : Joint stereo
Duration                                 : 5mn 28s
Bit rate mode                            : Variable
Bit rate                                 : 256 Kbps
Channel(s)                               : 2 channels
Sampling rate                            : 44.1 KHz
Compression mode                         : Lossy
Stream size                              : 10.0 MiB (100%)
Writing library                          : LAME3.98.4

Comment 2

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

Attachment: Patch: Use MP3FrameParser in DirectShow backend for duration calculation

Comment 3

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

Attachment: Patch: Use MP3FrameParser in DirectShow backend for duration calculation

Oops, forgot to qref.

Comment 4

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

Comment on attachment 798644 [details] [diff] [review]
Patch: Use MP3FrameParser in DirectShow backend for duration calculation

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

There are test failures here due to spamming durationchange events.

Comment 5

[Paul Adenot (:padenot)]

Can we just aggregate those events? I guess we would need to also check the reported duration vs. actual duration during decoding so we don't miss the last update. But this is a pain...

Comment 6

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

I'll rename MediaDecoder::UpdateMediaDuration() to MediaDecoder::UpdateEstimatedMediaDuration(), and have it ignore the duration request if the new duration is less than 500ms different from the old duration. This will hopefully prevent most of the instability in the duration estimation calculations from making it through to the media element.

Comment 7

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

Attachment: Patch v2: Use MP3FrameParser in DirectShow backend for duration calculation

* As previous patch, but I ignore duration estimate changes if the duration is within 500ms of the existing duration. This stops us changing duration thousands of times as during playback.
* Green: https://tbpl.mozilla.org/?tree=Try&rev=7bb9e3223f9f

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 801335 [details] [diff] [review]
Patch v2: Use MP3FrameParser in DirectShow backend for duration calculation

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

::: content/media/MediaDecoder.h
@@ +511,4 @@
>    void SetMediaDuration(int64_t aDuration) MOZ_OVERRIDE;
> +  // Updates the media duration. This is called while the media is being
> +  // played, calls before the media has reached loaded metadata are ignored.
> +  // The duration is assumed to be an estimate, and so a dregree of

s/dregree/degree/

::: content/media/MediaDecoderStateMachine.cpp
@@ +121,5 @@
> +// The amount of instability we tollerate in calls to
> +// MediaDecoderStateMachine::UpdateEstimatedDuration(); changes of duration
> +// less than this are ignored, as they're assumed to be the result of
> +// instability in the duration estimation.
> +static const int64_t ESTIMATED_DURATION_FUZZ_FACTOR = USECS_PER_S / 2;

Can you suffix this constant by the unit, like we do above? We have been bitten in the past by incoherent computations, in terms of units (comparing ms to us, and the like).

::: content/media/directshow/DirectShowReader.cpp
@@ +379,5 @@
> +    mDuration = duration;
> +    MOZ_ASSERT(mDecoder);
> +    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +    mDecoder->UpdateEstimatedMediaDuration(mDuration);
> +  }  

nit: trailing space.

I see you don't guarantee that the very last duration updates are reflected on the main thread. In practice, it seems that different media players have various ways of reporting media duration, so It probably does not matter much.

Comment 9

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

Landed, with nits picked:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e15819af724

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/1e15819af724

Comment 11

[Trevor Rowbotham [:rowbot]]

Attachment: Media_Duration_Test.html

This bug is only partially fixed.  If you view the file in the link in the bug report, everything appears to be fine and the duration of the file is reported correctly. However, if you watch closely, the duration is originally reported as 29:14, but at a quick glance, you don't see this because of autoplay being enabled.  So, this means that the duration is still being reported incorrectly when we only have metadata.  It is only when the file begins to play, as a result of autoplay, does the duration change to 5:29, which is the expected duration.

Now, this part might be a completely different bug but, I see a different behavior when you try to embed the same file into a webpage using the audio element.  I have attached a sample test case that shows the duration is still reported incorrectly, when only metadata is available and during playback, when the file is placed in an actual web page.  In addition to the incorrect duration, I have experienced the following anomalies since this bug landed, which only apply to when the audio file is embedded in a web page:

1) During playback a durationchange event was dispatched causing the duration to change.  I have seen the duration change from 29:14 to 5:33, 12:37, 43:17, and 49:18:59.

2) After seeking, a durationchange event was dispatched causing the duration to change from 29:14 to 7:01.

3) During playback the durationchange event was dispatched 3 times in rapid succession with the value changing wildly each time.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Can QA confirm comment 11 and help determine if we should be reopening this bug or file a (tracking-26 nominated) follow up?

Comment 13

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

Trevor: Thanks for testing this, and for providing feedback. It's with help like yours that we can ensure that we get a robust product.

I'm looking at the issue you pointed out, and have a fix. I will keep you informed. Thanks!

Comment 14

[Trevor Rowbotham [:rowbot]]

Glad to help out.

I have noticed another thing while testing this further. In comment 11, I noted a few anomalies related to the duration of the media file changing during playback.  I have found that if the duration changes during playback, once the current time reaches the reported duration both the current time and duration keep incrementing.  While playing around with the file in this bug report, its duration changed from 29:14 to 5:28 (coincidentally 5:28 is the expected duration of this file) after a few seconds of playback, but kept playing until the current time and duration reached 9:43.  So you get something like the following:

Current Time / Duration
0:00 / 29:14 (Metadata)
0:01 / 29:14 (Playback begins)
...
0:04 / 29:14
0:05 / 5:28  (Duration changes a few seconds in)
...
5:25 / 5:28
5:26 / 5:28
5:27 / 5:28
5:28 / 5:28  (You expect playback to end here)
5:29 / 5:29
5:30 / 5:30
5:31 / 5:31
...
9:41 / 9:41
9:42 / 9:42
9:43 / 9:43  (Playback finally ends)

I wasn't sure if your upcoming patch would also fix this so I wanted to report it as well.

Comment 15

[u279076]

I tested this with Firefox 26.0b1 on Windows 7 SP1 32-bit. Intial playback time is the time it shows on load. Actual playback time is the time it plays to naturally, without interruption. Seek-ahead playback time is the time it plays to after seeking ahead.

> http://www.nxt-level.com/music/4159_1332269164.mp3
* Initial playback time = 5:29
* Actual playback time = 5:29
* Seek-ahead playback time = 5:32

> https://bugzilla.mozilla.org/attachment.cgi?id=803447
* Initial playback time = 5:29
* Actual playback time = 5:29
* Seek-ahead playback time = 5:33

It would appear, at least for me, that this is fixed within a reasonable discrepency. I certainly cannot reproduce the exteremes reported in comment 11 nor comment 14. In my opinion this bug should be marked verified fixed and a new bug filed to improve accuracy further. Of course, if Trevor can still reproduce the extremes he reported then I suggest that warrants further investigation in *this* bug report.

Comment 16

[Trevor Rowbotham [:rowbot]]

Anthony, I am getting the same results as you are on the current Nightly.  Firefox 26 should no longer exhibit this now that bug 930372 landed (except for Win XP).  Out of curiosity, I ran a mozregression as this should still be a problem in Firefox 27+.  The last time that I was able to reproduce the extremes in comment 14 was in the 2013-10-24 Nightly build.  This appears to have been fixed since the 2013-10-25 Nightly build.

Mozregression:
Good: 2013-10-25
Bad: 2013-10-24

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=19fd3388c372&tochange=9f8233fcce1d

The only bug that jumps out at me that might have affected this would be bug 930372.  And if that is the case, then the problem isn't actually fixed, but merely hidden. Are you able to confirm that this was still an issue on the 24th, but works on the 25th?

While running mozregression, I was unable to reproduce the extremes that I had observed in comment 11. However, I only ran mozregression with a start date of 2013-10-01. Would you like me to test earlier builds to see if I can find the last build that has the extremes in comment 11?

Comment 17

[u279076]

(In reply to Trevor Rowbotham from comment #16)
> Would you like me to test earlier builds to see if I can
> find the last build that has the extremes in comment 11?

That's up to Lukas and/or Chris. I can't say whether it's valuable information to have or not. Personally, I am fine with this bug as it is given the extremes are no longer reproducible.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

I agree with Anthony's read here, if we're not seeing the extremes from comment 11 then we can consider this fixed in FF26 - updating status to reflect that.