Last Comment Bug 520500 - Vorbis files rejected when fishsound fails to parse comments
: Vorbis files rejected when fishsound fails to parse comments
: regression, testcase, verified1.9.2
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Chris Pearce (:cpearce)
: Maire Reavy [:mreavy]
: 496326 529632 (view as bug list)
Depends on:
Blocks: 529632
  Show dependency treegraph
Reported: 2009-10-04 20:12 PDT by Matthew Gregan [:kinetik]
Modified: 2010-02-01 18:54 PST (History)
5 users (show)
roc: blocking1.9.2+
cpearce: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (26.73 KB, patch)
2009-10-13 17:58 PDT, Chris Pearce (:cpearce)
cajbir.bugzilla: review+
dveditz: approval1.9.1.8+
Details | Diff | Splinter Review

Description Matthew Gregan [:kinetik] 2009-10-04 20:12:06 PDT
The following files from the bug URL fail to play:

The reason is the same for every file: they contain what looks like an invalid Vorbis comment, so fish_sound_comments_decode returns FISH_SOUND_ERR_OUT_OF_MEMORY, which causes fs_vorbis_decode to bail early with the same error.

I think it makes more sense to just ignore the invalid comments and try to treat the rest of the file as valid than to reject the entire file due to some bad optional metadata.
Comment 1 Matthew Gregan [:kinetik] 2009-10-04 20:44:44 PDT
The invalid comment is a plain string, not a KV pair as required by the spec.  It looks like fishsound tries to handle this case, because fish_sound_comments_decode has logic for handling a key without a value, but it calls fs_comment_new passing an explicit NULL, and that is rejected by fs_comment_validate_byname.

#0  fs_comment_validate_byname (name=0x1d0b1680 "Track encoded by encoder_example.c", value=0x0) at /Users/kinetik/work/mozilla-central/media/libfishsound/src/libfishsound/fishsound_comments.c:141
#1  0x127af5cf in fs_comment_new (name=0x1d0b1680 "Track encoded by encoder_example.c", value=0x0) at /Users/kinetik/work/mozilla-central/media/libfishsound/src/libfishsound/fishsound_comments.c:160
#2  0x127afe5c in fish_sound_comments_decode (fsound=0x1c9f75d0, comments=0x1298c07 " ", length=79) at /Users/kinetik/work/mozilla-central/media/libfishsound/src/libfishsound/fishsound_comments.c:515
(More stack frames follow...)

(gdb) fr
#0  fs_comment_validate_byname (name=0x1d0b1680 "Track encoded by encoder_example.c", value=0x0) at /Users/kinetik/work/mozilla-central/media/libfishsound/src/libfishsound/fishsound_comments.c:141
141	  if (!name || !value) return 0;

It seems like this case could be fixed easily by passing an empty string, rather than NULL, to fs_comment_new.  But that doesn't fix the general problem of treating comment decoding as a hard error, which I suspect we should treat as non-fatal.
Comment 2 Chris Pearce (:cpearce) 2009-10-12 19:21:28 PDT
*** Bug 496326 has been marked as a duplicate of this bug. ***
Comment 3 Chris Pearce (:cpearce) 2009-10-12 19:22:40 PDT
There's a work-in-progress patch in bug 521863 which fixes libfishsound's comment parsing. We should move it here. Additionally bug 496326 is a dupe of this. It's test cases: (Crashes with WIP patch from bug
Comment 4 Chris Pearce (:cpearce) 2009-10-13 17:58:50 PDT
Created attachment 406148 [details] [diff] [review]
Patch v1

Patch to libfishsound to not reject vorbis tracks with comments with name or value length 0. No whitespace changes for all you Emacs lovers out there.
Comment 5 Chris Pearce (:cpearce) 2009-10-13 20:55:23 PDT
Comment 6 Chris Pearce (:cpearce) 2009-10-13 20:58:01 PDT
We should take this on 1.9.2, it ensures that some files with valid vorbis comments are not incorrectly rejected as invalid.
Comment 7 Henrik Skupin (:whimboo) 2009-10-15 06:29:06 PDT
It's working fine on 1.9.1. So do we know which fix it has been regressed?

Verified fixed on trunk with builds on OS X and Windows like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091014 Minefield/3.7a1pre ID:20091014030825
Comment 8 Chris Pearce (:cpearce) 2009-10-15 14:57:05 PDT
Marking in-testsuite+, as the patch included content/media/test/bug520500.ogg which test this condition.

(In reply to comment #7)
> It's working fine on 1.9.1. So do we know which fix it has been regressed?

I was unable to play bug520500.ogg on Windows Firefox 3.5.3 and current 191 [Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv: Gecko/20091015 Shiretoko/3.5.5pre (.NET CLR 3.5.30729)]. FF3.5.3 fails to load the file, whereas 3.5.5pre can load it, but fails to play it.

However is another example of a valid ogg file which exhibits this problem, and it does work in 3.5.3 but not 3.5.5pre. So we've regressed between 3.5.3 and 3.5.5pre.

I can only assume one of the Annodex ogg library updates that we recently pushed to 191 caused this behaviour change between 3.5.3 and 3.5.5pre. Regardless, the 191 branch still can only play some of these valid files and has regressed since 3.5.3.

dveditz: Do you want to take this patch on 3.5.3 as well to fix this regression? It merely means we don't sometimes reject some valid ogg files with 0 length vorbis "comment" meta-data. I don't know how common this type of file is, but they're valid files.
Comment 9 Chris Pearce (:cpearce) 2009-10-18 15:09:24 PDT
Comment 10 Conrad Parker 2009-10-18 16:39:46 PDT
Applied to upstream libfishsound commit ac2015dbd72d59208a482de5b0e69e74774e7987
Comment 11 Conrad Parker 2009-10-18 16:52:12 PDT
Updated in upstream libfishsound commit 46faa6b42a47efed66be99569291f0153f50aae8

(earlier applied patch from 520500, this commit includes additional change in comment #4 patch)
Comment 12 Chris Pearce (:cpearce) 2009-10-18 17:39:43 PDT
(In reply to comment #9)

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.
Comment 13 Henrik Skupin (:whimboo) 2009-10-20 14:02:36 PDT
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b2pre) Gecko/20091019 Namoroka/3.6b2pre ID:20091019052445

Lets ask for 1.9.1 status so we don't loose track on 1.9.1.
Comment 14 Chris Pearce (:cpearce) 2009-11-18 14:20:57 PST
*** Bug 529632 has been marked as a duplicate of this bug. ***
Comment 15 Chris Pearce (:cpearce) 2009-11-18 14:24:01 PST
Comment on attachment 406148 [details] [diff] [review]
Patch v1

dveditz: can we take this patch bug on 1.9.1? It is a regression introduced by the annodex ogg library updates we took in FF3.5.4, and causes a reasonably common set of valid vorbis files to not play in 3.5.4.
Comment 16 Daniel Veditz [:dveditz] 2009-11-19 15:55:06 PST
Comment on attachment 406148 [details] [diff] [review]
Patch v1

We can take it on the 1.9.1 branch, but it missed the release.
Comment 17 Daniel Veditz [:dveditz] 2009-12-18 12:08:22 PST
Comment on attachment 406148 [details] [diff] [review]
Patch v1

Approved for, a=dveditz for release-drivers
Comment 18 Chris Pearce (:cpearce) 2009-12-20 18:16:59 PST
Comment 19 Conrad Parker 2010-02-01 18:54:21 PST
(In reply to comment #10)
> Applied to upstream libfishsound commit
> ac2015dbd72d59208a482de5b0e69e74774e7987

It turned out that this commit broke the test suite: it made the vorbiscomment handling fail for multiple comments of the same key, and it also chopped off the first character of every comment value.

I reverted this change and the related 46faa6b4, added a test to allow comments which are not of form KEY=VALUE, and implemented a cleaner fix that can add and retrieve such comments, in these upstream commits:

e56f4d8a5f2a008f4f51f30d60d53c0584ebb930 Mozilla #520500: Allow NULL-value comments
d0b3830b86302bf903a254d9fe49ac0a44104dfb tests: allow NULL-valued comments
0e01ba99fb5f642d76b012f8a6097cf0c8bfae69 Revert ac2015db, 46faa6b4

This passes the existing testsuite in libfishsound, and also correctly decodes all the comments of files listed in this bug URL.

Note You need to log in before you can comment on or make changes to this bug.