520500 - Vorbis files rejected when fishsound fails to parse comments

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

The following files from the bug URL fail to play:
beta4-test.ogg
chain-test3.ogg
lsp-test3.ogg
lsp-test4.ogg
test-short.ogg

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]]

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 3

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

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:

http://whatthebert.com/ihameed/boomtss/zurie-piratesxaimusremix.ogg
http://upload.wikimedia.org/wikipedia/commons/c/cd/Ell-Lernaia_Ydra.ogg (Crashes with WIP patch from bug

Comment 4

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

Attachment: 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 (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/6ff04bb6429b

Comment 6

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

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][⌚️UTC+1]]

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 (Not reading bugmail)]]

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:1.9.1.5pre) 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 http://whatthebert.com/ihameed/boomtss/zurie-piratesxaimusremix.ogg 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 (Not reading bugmail)]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/41857366d02e

Comment 10

[Conrad Parker]

Applied to upstream libfishsound commit ac2015dbd72d59208a482de5b0e69e74774e7987

Comment 11

[Conrad Parker]

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 (Not reading bugmail)]]

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

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][⌚️UTC+1]]

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 15

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

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]]

Comment on attachment 406148 [details] [diff] [review]
Patch v1

We can take it on the 1.9.1 branch, but it missed the 1.9.1.6 release.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 406148 [details] [diff] [review]
Patch v1

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 18

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4e86fc64131c

Comment 19

[Conrad Parker]

(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.