Closed Bug 519155 Opened 13 years ago Closed 13 years ago

Chained Ogg / Theora files refuse to play entirely


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




Tracking Status
status1.9.2 --- beta1-fixed


(Reporter: gmaxwell, Assigned: cpearce)





(1 file, 1 obsolete file)

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:

Prior versions of firefox (just tested Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: 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
In a build from, 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, 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.
Ever confirmed: true
Flags: blocking1.9.2?
I was too hasty to blame the liboggplay update.  It was actually bug 518169.  Verified by backing out the local back to liboggz.
Blocks: 518169
No longer blocks: CVE-2009-3378
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.
Attached patch Patch v1 (obsolete) — Splinter Review
* 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.
Assignee: nobody → chris
Attachment #403372 - Flags: review?(chris.double)
Attached patch Patch v2Splinter Review
D`oh, forgot to update README_MOZILLA and in previous patch. This one does...
Attachment #403372 - Attachment is obsolete: true
Attachment #403373 - Flags: review?(chris.double)
Attachment #403372 - Flags: review?(chris.double)
Flags: blocking1.9.2? → blocking1.9.2+
Attachment #403373 - Flags: review?(chris.double) → review+
Closed: 13 years ago
Resolution: --- → FIXED
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?
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
(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.
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.
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.
You need to log in before you can comment on or make changes to this bug.