520493 - audio file won't play - regression from 3.5

Status: RESOLVED FIXED

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

Description

[Christopher Blizzard (:blizzard)]

The .ogg audio file from the URL above won't play in recent 1.9.2 builds but does play fine in Firefox 3.5.3.  Reproduced on Windows 7 and Fedora 11.

Comment 1

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

Regression window: [2009-09-28-04, 2009-09-29-04]
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-09-28&enddate=2009-09-29

Looks like a regression from bug 516323. Sigh.

Comment 2

[Matthew Gregan [:kinetik]]

Curiously, removing the oggplay_seek(mPlayer, 0) from LoadOggHeaders here: http://hg.mozilla.org/mozilla-central/file/1fa6860af3d6/content/media/ogg/nsOggDecoder.cpp#l1872

...allows this file to play.  That seek is totally redundant as far as I can tell, as oggplay_get_duration should not change the observable stream position when called, so it shouldn't be necessary to seek afterwards.  However, removing that seek causes the test file Chris Pearce added in bug 516323 (bug516323.ogv) to end playback earlier than expected with a currentTime of ~3.9 when we expect ~4.4.

Comment 3

[Matthew Gregan [:kinetik]]

Here's another case where we hit what I think is the same bug: http://www.annodex.net/~silvia/itext/elephant_no_skin.html

Audio is fine when played in the page, but fullscreen it and we get no audio.  We only hit it when fullscreening because the server sends X-Content-Duration, so we don't do the process headers/seek back to 0/reprocess headers dance.  When fullscreened, we create a new decoder that shares the cache but doesn't have the duration from X-Content-Duration, so it seeks to calculate the duration, and runs into this bug.  It's easy to reproduce by downloading the video and loading it playing it from disk.

Comment 4

[Damon Sicore (:damons)]

blocking 1.9.2+  P2.  This doesn't require a beta, does it?

Comment 5

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

(In reply to comment #4)
> blocking 1.9.2+  P2.  This doesn't require a beta, does it?

Beta coverage would be nice... I'm not sure what subset of videos don't play due to this, so having it tested by a wider audience would be good...

Comment 6

[Matthew Gregan [:kinetik]]

More testcases: http://kyon.haruhi.fr/videos/test.php

Comment 7

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

Attachment: Patch v1

Three problems:
1. liboggz's seek bisection terminates when "(unit_at == unit_last_iter)"; when it lands on the same page in two consecutive iterations. We don't want to terminate the seek in this condition when our bisection lands in the same page twice due to the bisection-guess not estimating far enough away to get off the page it's on. We need to keep bisecting, until we get the page we want. This is particularly important when we hit this doing the seek to get duration, we record the current time unit, seek to end, then seek back. If the seek back lands on the wrong page, we can get into trouble and some files won't play. I also discovered that the bisection is using the inaccurate offset values to cut down it's bisection window. It's using offset_at rather than offset_next. offset_at is the guess, but offset_next is the offset of the next page. Using offset_next reduces the number of bisections required.

2. Yet another infinite loop in our old friend oggplay_step_decoding. This one happens on small files. When we read some data, and the callback hits EOF and all tracks are marked inactive, the "overhang" detection code will then wrongly assume that all data has been returned in a callback and exit without returning the data its read. We need to detect this case, and ensure that all data read is returned in a callback.

3. Problem with controls, had to add a !ended to controls to ensure throbber didn't throb in the case when there was only 1 timeupdate event and playback reaches the end. The controls count the number of timeupdate events to determine whether to show the throbber or not. Because the small test file only has 1 frame, it doesn't send more than 1 timeupdate event, and we were getting a throbber when playback reached the end on that small file.

Patch includes test.

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/4769862ade7f

Comment 9

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/264b5a4b9054

Comment 10

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

(In reply to comment #9)
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/264b5a4b9054

Turns out we've already branched of 1.9.2 for beta 1, so this fix won't show up on 1.9.2 until a release after the beta. status-1.9.2 -> final-fixed.