GNOME Bugzilla – Bug 319986
annodex decoding and encoding support
Last modified: 2006-03-21 12:54:20 UTC
Adds skeldec and cmmldec to gst-plugins-good.
Created attachment 53950 [details] [review] adds skeldec and cmmldec in gst-plugins-good/gst/annodex
Created attachment 53951 [details] [review] adds GST_SKELETON_FISHEAD, GST_SKELETON_FISBONE, GST_TAG_CMML_HEAD, GST_TAG_CMML_CLIP to gstreamer/gst/gsttaglist.h
Created attachment 53952 [details] [review] adds annodex skeleton support to gst-plugins-base/ext/ogg/oggdemux.c
the oggdemux patch contains http://bugzilla.gnome.org/attachment.cgi?id=53627&action=view from http://bugzilla.gnome.org/show_bug.cgi?id=319110
Created attachment 54007 [details] [review] updated version of the patch to apply against gst-plugins-base/ext/ogg/oggdemux.c updated to include the new oggdemux chain finding patch (bug: #319110, attach: #53984)
Created attachment 54076 [details] [review] updated patch to apply against gst-plugins-base/ext/ogg/gstoggdemux.c updated to apply on HEAD
Created attachment 54143 [details] [review] gst-plugins-good/gst/annodex skeldec & cmmldec better clip handling
Created attachment 54397 [details] [review] patch against gst-plugins-base/ext/ogg/gstoggdemux.c updated to apply on head (#319110 was merged)
Created attachment 54399 [details] [review] gst-plugins-good/gst/annodex skeldec & cmmldec fixed end_time(s) for clip tags
Created attachment 54445 [details] [review] patch against gst-plugins-base/ext/ogg/gstoggdemux.c fixed a bug when PLAYING, then going to NULL and then PLAYING again
See test content here, http://media.annodex.net/~scott/ .
Created attachment 55350 [details] [review] gstreamer (core) patch (annodex tags) This should probably be moved in gst-plugins-base
Created attachment 55351 [details] [review] gst-plugins-base patch (oggdemux skeleton support/oggmux cmml support) updated to apply on HEAD. Added text/x-cmml caps to oggmux to mux cmml streams
Created attachment 55352 [details] [review] gst-plugins-good (cmmlenc, cmmldec, skeldec elements) added cmmlenc. updated to apply on head.
I took a quick first look at the new elements, comments follow: - generally looks excellent. Good to see tests, etc! - in gst_annodex_parse_headers, the 2nd g_strstr_len() call might pass a negative length on malformed input; should be checked for. - the to/from npt functions disagree on the signedness of what they're dealing with, which might not matter in practice. But the parser doesn't validate that the fields are in-range; should be fixed. - bunch of (VL_UNRESOLVED) in the header comment chunks. - I have no idea from the description what the 'extract-mode' property does. The description should be sufficient to decribe what it does. - gst_cmml_dec_parse_ident_header() (and probably the other parsing functions, I didn't check them all) aren't checking that the buffer is large enough for the data they're trying to read, so they can overrun their buffer. - "CmmlEnc", "Coenc/Encoder", <-- note typo in Codec. - You use libxml, but don't link against it? This will only work on some OSes. - lines should always be under 80 columns: see the one that starts with preamble = g_strdup_printf ("<?xml (and maybe others) - "utc" seems like an overly short and undescriptive property name. Also, the description doesn't mention what format it is (this latter applies to 'timebase' also). - "base" is also undescriptive; no idea what it's meant to do. - I don't understand this in skeldec: + /* this should be application/x-ogg-skeleton but that would break + * autoplugging */ + GST_STATIC_CAPS ("application/octet-stream") Shouldn't you be adding a typefind function for skeleton so you can typefind this stream? - Use of g_strsplit doesn't seem to check that there's at least one element (though it checks if there's a 2nd element before using it). Also doesn't look like it frees the tokens array? - fisbone->granulerate_* seem to be inconsistently signed. Well, there's a first pass. Also, see thomas's gstreamer/docs/random/moving-plugins document; I didn't notice any gtkdoc documentation for the plugins. Mike
Created attachment 55504 [details] [review] gst-plugins-good (cmmlenc, cmmldec, skeldec elements) * fixed gst_annodex_parse_headers. * the time parsing functions check for hours < 0, 0 <= minutes <= 59, 0 <= seconds <= 59. * fixed the (VL_UNRESOLVED) strings. * all the functions that read data from a buffer check the buffer's size before. * renamed the "utc" property to "calendar-base-time". * renamed "base" to "base-uri". * fixed the tokens array leak in skeldec it still lacks documentation
Created attachment 56052 [details] [review] gst-plugins-good (cmmlenc, cmmldec, skeldec elements) Added multiple tracks support to both cmmlenc and cmmldec. Added test cases for bad input/pipeline to cmmlenc and cmmldec.
Created attachment 56054 [details] [review] gst-plugins-base patch (oggdemux skeleton support/oggmux cmml support) Updated to apply on head
The oggdemux changes have some issues, but I've now tried actually building the new annodex plugins. Problems: - they don't build with gcc4. Straightforward fixes to avoid type-punning warnings (don't worry about this for now, I'll add them in before committing). - It still has the "application/octet-stream" caps from skeldec. That's still completely bogus. I assume the problem of decodebin repeatedly autoplugging this if you put the right caps in is caused by skeldec being in category "Codec/Decoder". Since it doesn't decode - it parses, it should be in "Codec/Parser", I think. Does that fix your problem? - the tests fail. Whilst it's great to have tests for plugins, they should also pass... cmmlenc: Running suite(s): cmmlenc 0%: Checks: 2, Failures: 2, Errors: 0 elements/cmmlenc.c:264:F:general:test_enc: 'keyindex * granulerate' (0) is not equal to 'prev' (1234000000) elements/cmmlenc.c:264:F:general:test_bad_start_time: 'keyindex * granulerate' (0) is not equal to 'prev' (1234000000) Running suite(s): cmmlencWARN (0x8053268 - 0:00:00.075255000) cmmlenc(18828) gstcmmlenc.c(502):gst_cmml_enc_parse_tag_clip:<cmmlenc> error: invalid start time for clip (clip-bad) 0%: Checks: 2, Failures: 2, Errors: 0 elements/cmmlenc.c:264:F:general:test_enc: 'keyindex * granulerate' (0) is not equal to 'prev' (1234000000) elements/cmmlenc.c:264:F:general:test_bad_start_time: 'keyindex * granulerate' (0) is not equal to 'prev' (1234000000) make: *** [elements/cmmlenc.check] Error 2 And cmmldec: msmith@freeze:~/src/gstreamer-HEAD/gst-plugins-good/tests/check$ make elements/cmmldec.check Running suite(s): cmmldecPP: internal error, state == PI Check1 PP: internal error, state == PI 50%: Checks: 2, Failures: 1, Errors: 0 elements/cmmldec.c:169:F:general:test_dec: 'clip-1' (<clip id="clip-3" track="default" start="100:59:59.678"><a href="http://www.csiro.au/">http://www.csiro.au</a><img src="images/index1.jpg"/><desc>Welcome to CSIRO</desc><meta name="test" content="test content"/></clip>) is not equal to (<clip id="clip-1" track="default" start="0:00:01.234"><a href="http://www.csiro.au/">http://www.csiro.au</a><img src="images/index1.jpg"/><desc>Welcome to CSIRO</desc><meta name="test" content="test content"/></clip>) Running suite(s): cmmldecPP: internal error, state == PI Check1 PP: internal error, state == PI 50%: Checks: 2, Failures: 1, Errors: 0 elements/cmmldec.c:169:F:general:test_dec: 'clip-1' (<clip id="clip-3" track="default" start="100:59:59.678"><a href="http://www.csiro.au/">http://www.csiro.au</a><img src="images/index1.jpg"/><desc>Welcome to CSIRO</desc><meta name="test" content="test content"/></clip>) is not equal to (<clip id="clip-1" track="default" start="0:00:01.234"><a href="http://www.csiro.au/">http://www.csiro.au</a><img src="images/index1.jpg"/><desc>Welcome to CSIRO</desc><meta name="test" content="test content"/></clip>) make: *** [elements/cmmldec.check] Error 1
Notes on the ogg demuxer changes: - please use GST_WARN_OBJECT() rather than g_warning(). - fisbone 'content-type' is used to override caps on the pad associated with that fisbone. Not acceptable; the fisbone mimetype might not be the same as the gstreamer caps name. Also, a content-type simply doesn't have the same basic syntax as a gstreamer caps string. As a bug: the return value from gst_caps_from_string() isn't checked, it's just used anyway. But anyway, this chunk of code just needs deleting, which probably has some follow-on effects elsewhere in the code. - typo in /* function called to convert a granuleops to a timestamp */ So, there's only one substantial thing there, but it's major.
Needless to say, the tests pass here on both linux-ppc32 and linux-x86. What arch/system have you tested it on? Could you rerun the tests with the environment variable GST_DEBUG=cmml*:5 set please? Oh and compiling with gcc (GCC) 4.0.3 20051204 (prerelease) (Ubuntu 4.0.2-5ubuntu2) gives no warning/errors here. As for the skeldec element, i've removed it and moved its functionality directly into oggdemux, as you suggested on irc. I will remove the code you mentioned in your last comment, thanks.
Created attachment 56080 [details] debug logs from test runs Test runs with cmml debug enabled. Run on an ubuntu breezy, x86 system. Compiled with gcc 4.0.2 20050808 (prerelease) (Ubuntu 4.0.1-4ubuntu9)
(for those following along, the test failures were resolved; turned out to be due to a local fix for compilation) Apparently changing the CMML parser to 'Parser' rather than 'Decoder' won't work - decodebin still tries to autoplug parsers. I would argue that it's more correct anyway. One suggestion is to get oggdemux to produce "text/x-cmml, parsed=false", and have the cmml decoder accept ONLY that, and to output "text/x-cmml, parsed=true".
I think that's the best option, myself (adding a parsed parameter to cmmldec's output) Also, regarding the new TAG declarations in core, I think we should definitely add these as headers in gst-plugins-base. We've always tried to keep the core as free of actual media information as possible. The other option of course is to merge GStreamer core and gst-plugins-base in 0.12
Created attachment 56527 [details] [review] gst-plugins-good (cmmlenc, cmmldec) Added the "encoded" parameter to cmmlenc/cmmldec caps: cmmldec has sink caps "text/x-cmml, encoded = true" and src caps "text/x-cmml, encoded = false" cmmlenc has sink caps "text/x-cmml, encoded = false" and src caps "text/x-cmml, encoded = true" Removed skeldec as skeleton decoding is now in the gst-plugins-base patch. This patch also adds a "rate" parameter to speexenc's src caps to make speex streams annodexable by oggmux.
I forgot to say that the cmml tags are now defined in gst-plugins-good/gst-libs/gst/gsttagcmml.h. I put it in -good and not in -base as oggdemux doesn't use the tags anymore (so i see no point in having them in -base).
Created attachment 56528 [details] [review] gst-plugins-base (annodex encoding/decoding support) This patch fixes the bugs in oggdemux (re-enable typefinding and correct the typo). It also adds preliminary skeleton encoding support. The informations needed to create the skeleton fisbones are passed from the encoders to oggmux through caps. I patched vorbisenc and theoraenc to set the right parameters on caps so now vorbis, theora, speex and cmml streams are annodexable. For the moment the preroll field in fisbone packets is always set to 0, i don't know where to get the right values.
Whee! Finally got the (decode) patches in, with a bunch of minor changes so that things work right in (for instance) playbin. Thanks for being patient, Alessandro, and sorry for taking so long... Note that the encoding patches aren't all there: cmmlenc is, but not oggmux, nor the vorbis/theora patches.
Created attachment 60115 [details] [review] -good HEAD fix
This has all been committed and sorted out, hasn't it? If not, please re-open.