User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090728 Minefield/3.6a1pre The above multitrack Ogg Theora/Vorbis file plays in VLC and other Ogg players, but refuses to decode in Firefox 3.5.3 and Firefox 3.6a1. Chris Double found: "liboggplay is returning E_OGGPLAY_BAD_INPUT when firefox tries to decode it for some reason". Reproducible: Always Steps to Reproduce: 1. Load URL 2. 3. Actual Results: grey video box with X in it Expected Results: video loaded and ready to play

This is probably a bug in libfishsound, it's not resetting the packet count when you seek to 0, and then the (re)decode of the header packets fails. I have a patch to fix this included in bug 512328. Now we have a separate testcase, I'll extract it into a stand alone patch and fix it here.

Patch with testcase, also makes 501279.ogg not fail with an error. I think that's ok; it's a valid file, it's just got no data. The problem is that libfishsound has no concept of packets, and keeps a packet count so that it can reconstruct fake packet to pass to libvorbis. When we seek to time=0ms, libogg{z,play} seeks to offset=0, rather than the offset of the first data packet (I believe Wiking was working on a fix for this). So we reread the header packets, but because libfishsound maintains its own packet count, and because its packet count is > 3, it assumes the header packets are data packets, and passes them to vorbis to decode, which cannot read them as data packets, and fails. The fix is to reset the fishsound packet count when we seek to 0.

Applied in upstream libfishsound commit 12e315f096d293f8b8752cbdb607489d122346d8

Comment on attachment 400434 [details] [diff] [review] Patch v1 - reset libfishsound after seek(0) D'oh! This causes test failures on Linux with sound enabled - unfortunately I'd only tested with sound disabled, so didn't pick it up. :(

(In reply to comment #5) > (From update of attachment 400434 [details] [diff] [review]) > D'oh! This causes test failures on Linux with sound enabled - unfortunately I'd > only tested with sound disabled, so didn't pick it up. :( I imagine the fix will be to not call fish_sound_reset() on oggplay_seek(0), and to affect the packet count reset via some other means.

(In reply to comment #5) > (From update of attachment 400434 [details] [diff] [review]) > D'oh! This causes test failures on Linux with sound enabled It seems that it can't handle having bug501279.ogg in gSmallTests in the manifest as I'd previously thought. test_load.html fails, as bug501279.ogg doesn't cause the same events to be sent as the other gSmallTests files. bug501279.ogg has only vorbis and theora header packets, but no data packets. bug506094.ogv has only theora header packets, and it sends the expected events in that test, so I guess bug501279.ogg should too.

Pretty much the same as v1, the problem I encountered with Linux failing intermittently has gone away, I suspect due to bug 512328 landing. This patch is the same as the previous, just unbitrotten.

Comment on attachment 403163 [details] [diff] [review] Patch v2 >+ if (track->content_type != OGGZ_CONTENT_VORBIS) { >+ continue; Will this same problem occur with tracks containing Speex, Flac and Celt tracks?

It looks like it. libfishsound keeps packet counts for those media types too, and uses that to determine whether the packet is a header packet.

(In reply to comment #9) > (From update of attachment 403163 [details] [diff] [review]) > >+ if (track->content_type != OGGZ_CONTENT_VORBIS) { > >+ continue; > > Will this same problem occur with tracks containing Speex, Flac and Celt > tracks? Not handling the other content types here won't break seeking for us, since we don't support those types yet, and oggz understands their metrics. In order to reset the packet counts for those other types, I'd have to patch against libfishsound without being able to test them, which would not be wise, so I suggest we ignore them for now.

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/503880cd5ddb

Comment on attachment 403163 [details] [diff] [review] Patch v2 Requesting approval for landing on 1.9.2. This patch allows a number of ogg files to play, without this they don't. Includes test.