519155 - Chained Ogg / Theora files refuse to play entirely

Status: RESOLVED FIXED

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

Description

[Gregory Maxwell]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.3a1pre) Gecko/20090926 Minefield/3.7a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.3a1pre) Gecko/20090926 Minefield/3.7a1pre

Trunk is completely refusing to play these chained files from j^'s theora testsuite:

http://v2v.cc/~j/theora_testsuite/chained_streams.ogg
http://v2v.cc/~j/theora_testsuite/multi2.ogg

Prior versions of firefox (just tested Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.4pre) Gecko/20090926 Shiretoko/3.5.4pre) played just the first segment of the chain... which isn't correct either but its better than refusing to play entirely.

Reproducible: Always

Comment 1

[Matthew Gregan [:kinetik]]

In a build from http://hg.mozilla.org/mozilla-central/rev/f6df32312266:, chained_streams.ogg plays the first stream and multi2.ogg loads but doesn't really play (just get a white video, play doesn't seem to do anything).

In a build from http://hg.mozilla.org/mozilla-central/rev/8013abd85baa, both are rejected.

Things that touched content/media or media in that range are: bug 512328, bug 518169, and bug 492286.  Seems likely to be the first one.

Comment 2

[Matthew Gregan [:kinetik]]

I was too hasty to blame the liboggplay update.  It was actually bug 518169.  Verified by backing out the local back to liboggz.

Comment 3

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

In the patch for bug 518169, in oggz_bounded_seek_set(), it assumed that we were in a sane state, and it assumed that on failure you could reset the reader by calling oggz_reset (oggz, previous(oggz->offset), previous(reader->current_unit), SEEK_SET). Unfortunately oggz_seek_end() wasn't setting reader->current_unit after moving the oggz read cursor to the end, leaving reader->current_unit. This would confuse oggz, as it would think that the end of the media had time 0.

Also in oggz_seek_end(), for chained content oggz_get_unit() fails after moving the oggz read cursor to the end, as oggz doesn't know how to convert the gp there to a time, since oggz has not encountered the header packets of the last ogg in the chain.

oggz_seek_end() can also seek to an offset from the end, but in it's current implementation, it seeks to the byte-offset end, gets the time there, then t *always* seeks to the time+offset, even if the offset is 0. This wouldn't be a problem if its calculated the unit_end was correct, but since it's not for chained oggs, it will then seek to (-1 + offset), which is incorrect. It would then fail, and reset oggz to the incorrect timestamp as above.

Comment 4

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

Attachment: Patch v1

* Set reader->current_unit to unit_end in oggz_seek_end() so that oggz_bounded_seek_set() can rollback on fail correctly.
* Change oggz_reset_seek() so that it can reset with -1 unit. This is so that oggz_reset_seek() can be called on failure to signal to liboggz that it needs to recalculate reader->current_unit at the current read cursor.
* Change oggz_seek_end() to not do an extra seek if it fails or if its end-offset is 0.
* Add chained ogg testcase. Mochitests pass on Windows.

Comment 5

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

Attachment: Patch v2

D`oh, forgot to update README_MOZILLA and update.sh in previous patch. This one does...

Comment 6

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

http://hg.mozilla.org/mozilla-central/rev/0ff8f0469f37

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Chris, what do we really test with our automated tests and chained video files? Is it just play back or also seeking? I don't think its the latter one because we also don't have a test beside the fix on bug 518169. Do we need any manual testcase for now?

Comment 8

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

(In reply to comment #7)
> Chris, what do we really test with our automated tests and chained video files?

We test that chained oggs load. The chained ogg didn't play nicely in our playback tests, presumably because its duration can't be determined, so we can only automatically test that they load.

We can't really support playback of chained files properly at the moment because the underlying libraries can't handle them. I added the test so that the next time we break the current (limited) support, we'll know about it.

> Do we need any manual testcase for now?

I wouldn't bother for now, chained oggs don't play "properly" anyway at the moment.

Comment 9

[Gregory Maxwell]

There is one chaining use case which is far more important than the others: The ability to play-through chaining boundaries on icecast streams.  Just about all other Ogg/Vorbis playing apps can handle this case, and without it you can't use firefox as a general internet radio client. (especially stations that send a separate station-ID on connect).   This should be specifically noted as a test case, even though it fails currently.

Comment 10

[cajbir (:cajbir)]

The case mentioned in comment 9 is bug 455165. We can't do much about it until liboggplay fixes the issue or until we no longer use it.

Comment 11

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6d6a0f5fe425