GNOME Bugzilla – Bug 337026
oggmux doesn't set EOS properly
Last modified: 2006-09-29 18:07:59 UTC
oggmux doesn't ever set EOS on the pages it should. As well as being out of spec in a minor way, this causes at least one very serious practical problem: Vorbis treats BOS/EOS specially, in order to clip the start/end of the stream to the correct length (so that your encoded file can be decoded to something exactly the same length as the original data). If the EOS flag isn't there, it doesn't (can't) know to do this. Result: ... ! vorbisenc ! oggmux ! filesink produces an output file that decodes more data than it's meant to, some of which has just been invented by the encoder.
According to bug #335635, this may also have the effect of truncating the track.
No, the truncation error was a different bug (fixed now, in cvs). This error result in TOO MUCH, not too little, data being decoded, and is repairable (the 'vorbiscomment' command line tool will, as a side effect, do so).
(In reply to comment #2) > No, the truncation error was a different bug (fixed now, in cvs). Do you know the bug number?
Was it this change, per chance? 2006-04-03 Michael Smith <msmith@fluendo.com> * ext/ogg/gstoggmux.c: (gst_ogg_mux_queue_pads): Oggmux sucks. Make it suck slightly less by writing out the final page. Still can't encode a vorbis-in-ogg file correctly, though.
There was no bug number that I know of; yes, that commit was what fixed the truncation problem.
Created attachment 67880 [details] [review] possible patch I've taken a look at this, and come up with this patch. I don't have a full understanding of oggmux, so it might be the wrong approach, but I think it makes sense. The patch is against cvs, but I'm having issues with oggdemux using core and -base from cvs so I've only tested against 0.10.8. Basically it changes oggmux from holding a single buffer for each pad, to holding two, and doesn't release the data to be muxed until there is two buffers (or EOS was received on the pad). When the first buffer is received for a pad it is put in next_buffer, and if that pad is the best, it will continue waiting for for data. When collect pads next calls the callback, pad->next_buffer will be moved across to pad->buffer and next_buffer filled. If the pad is now the best it can pull the data as current versions do. When EOS is reached pad->next_buffer is still moved across to pad->buffer, but next_buffer isn't filled (as there is no more data). This means that the code can check pad->next_buffer to see whether there ismore data, and set the packet's EOS flag appropriately. I haven't done any real testing, so this needs to be checked to ensure that it fixes the problem correctly and doesn't break any other situations, such as chained vorbis streams, theora, etc.
Created attachment 67881 [details] [review] fixed patch Oops, this one fixes a dumb mistake.
I've tried this patch against 0.10.5 (that's what I've got on Fedora 5 without having to build all of gstreamer from source). It causes Sound-juicer to freeze when ripping to ogg (zero length file is created, ripping to other formats is still okay). Should this patch be expected to work against 0.10.5? (I can confirm bug 337026 with unpatched gstreamer)
Created attachment 68392 [details] [review] better patch This patch fixes a s/||/&&/ mistake, and fixes some issues with the end-game, which hopefully fix the above reported issue. I've done some furthur testing, and this fixes the eos-not-set warning ogginfo reports with ogg vorbis files remuxed with the patched version, and it seemed to work on the one ogg theora file I tried - but that probabyl need more testing. The end-game changes are: * When gst_ogg_mux_release_pad is called, the pad isn't removed from collectpads immediately as it may still have data waiting. Instead a "removed" flag is set. * in gst_ogg_mux_queue_pads if a pad has it's "removed" flag set AND there are no buffers left on it, then it's cleaned up like it was in gst_ogg_mux_release_pad previously. * at the end of gst_ogg_mux_collected, we check whether there are still any connected sinkpads. If not, then gst_ogg_mux_collected will no longer be called (as they pads won't receive futhur data), so we recursively call it ourselved. This is needed to use the buffer we've removed but not handled.
Created attachment 68404 [details] rejected patch sections It might be that I'm trying to patch against an older version, 0.10.5, but I can't apply the latest patch. gstoggmux.c.rej is attached. Most of this I can do by hand (and I'm unsure why it won't apply), but the last "--- 807,815 ---- ogg_page page; GstFlowReturn ret; + /* it's no longer active */ + g_atomic_int_dec_and_test (&ogg_mux->active_pads); + /* Just gone to EOS. Flush existing page(s) */ pad->eos = TRUE;" context doesn't seem to exist in my gstoggmux.c [ian@atlas gst-plugins-base-0.10.5]$ patch -p0 < ../patch-2006-07-05 patching file ext/ogg/gstoggmux.c Hunk #1 succeeded at 64 (offset -1 lines). Hunk #3 succeeded at 392 (offset -6 lines). Hunk #4 FAILED at 436. Hunk #5 succeeded at 680 (offset -4 lines). Hunk #6 succeeded at 718 (offset -6 lines). Hunk #7 succeeded at 757 (offset -4 lines). Hunk #8 FAILED at 807. Hunk #9 succeeded at 807 with fuzz 1 (offset -22 lines). Hunk #10 succeeded at 973 (offset -8 lines). Hunk #11 succeeded at 995 (offset -22 lines). Hunk #12 succeeded at 1159 (offset -8 lines). Hunk #13 succeeded at 1182 (offset -22 lines). Hunk #14 succeeded at 1303 (offset -8 lines). Hunk #15 succeeded at 1450 (offset -22 lines). 2 out of 15 hunks FAILED -- saving rejects to file ext/ogg/gstoggmux.c.rej
Managed to manually merge it, although it seems the following: gst_ogg_mux_queue_pads (GstOggMux * ogg_ GstBuffer *buf; gboolean incaps; + if (pad->removed) { + ogg_stream_clear (&oggpad->stream); ^^^^^^ should be &pad->stream? A brief test confirms that: 1. soundjuicer works with this. 2. the Ogg/Vorbis produced is a conforming stream. 3. Decoding to wav will cmp with a rip to flac round- tripped through oggenc and oggdec at the same q setting. Bizarrely the decoded test file is slightly shorter if eos is not set. May have some time this weekend, any suggested tests?
(In reply to comment #11) > gst_ogg_mux_queue_pads (GstOggMux * ogg_ > GstBuffer *buf; > gboolean incaps; > > + if (pad->removed) { > + ogg_stream_clear (&oggpad->stream); > ^^^^^^ > should be &pad->stream? Looks like it. > A brief test confirms that: > 1. soundjuicer works with this. > 2. the Ogg/Vorbis produced is a conforming stream. > 3. Decoding to wav will cmp with a rip to flac round- > tripped through oggenc and oggdec at the same q setting. > > Bizarrely the decoded test file is slightly shorter if eos > is not set. May have some time this weekend, any suggested > tests? Probably the more important things to test would be more exotic things: 1) ogg theora - creating and remuxing 2) something to do with streaming 3) any other weird stuff
Created attachment 68765 [details] [review] fixed patch This fixes a silly s/buffer/next_buffer/ mixup in _compare_pads. For me, the output of the following looks fine: gst-launch-0.10 videotestsrc num-buffers=100 ! theoraenc ! queue ! oggmux name=m ! filesink location=/tmp/foo.ogg audiotestsrc num-buffer=200 ! audioconvert ! vorbisenc ! queue ! m.
I've also tested with the ogg remuxer example from -python, and if it's changed to do a normal seek, rather then a segment seek, then it works perfectly. Using a segment seek makes it not work because oggmux never receives EOS and doesn't have the sink pads removed. The remuxer using a segment seek is arguaby broken, and I'm not sure if there would be a safe way of dealing with it. Unless I think of some more tests, I'm fairly comfidant this now works.
Created attachment 69234 [details] [review] unit test This is a unit test for oggmux, and checks ogg pages' BOS and EOS flags and granulepos ordering. I'm not sure of the best way of checking what codec a logical stream is, so it doesn't check to ensure the theora BOS is before the vorbis BOS. Currently it tests simple vorbis encoding, simple theora encoding and has two tests for theora+vorbis (one with each longer). Testing chained vorbis streams, or other more complex things would be good, but I'll need to find some code to steal or write them.
Codec from BOS: Vorbis all packets begin: 1) [packet_type] : 8 bit value 2) 0x76, 0x6f, 0x72, 0x62, 0x69, 0x73: the characters 'v','o','r','b','i','s' as six octets Decode continues according to packet type; the identification header is type 1, the comment header type 3 and the setup header type 5 (these types are all odd as a packet with a leading single bit of '0' is an audio packet). The packets must occur in the order of identification, comment, setup. Theora: Read an 8-bit unsigned integer as HEADERTYPE. If the most significant bit of this integer is not set, then stop. This is not a header packet. 2. Read 6 8-bit unsigned integers. If these do not have the values 0x74, 0x68, 0x65, 0x6F, 0x72, and 0x61, respectively, then stop. This stream is not decodable by this specification. These values correspond to the ASCII values of the characters ‘t’, ‘h’, ‘e’, ‘o’, ‘r’, and ‘a’. Decode continues according to HEADERTYPE. The identification header is type 0x80, the comment header is type 0x81, and the setup header is type 0x82. (Sorry, haven't had time to do any more tests yet)
Created attachment 69501 [details] [review] improved test This version of the test checks that no vorbis/speex packets occur before the theora BOS. I've checked the case of the vorbis BOS being before the theora one by delibrately breaking oggmux, and the speex case by changing the test to use speexenc instead of vorbisenc. The unit test doesn't actually check the theora+speex case because speexenc isn't in -base, but the code handles it correctly.
+ gboolean removed; /* whether the pad have been removed */ GstBuffer *buffer; /* the queued buffer for this pad */ + GstBuffer *next_buffer; /* and the one after that */ 'has been removed' Also, can you change the comments on buffer/next_buffer to be clearer about what they are, exactly? Not clear why you're using g_atomic_ things. If that's neccesary, then probably it's not sufficient - you should be taking the object lock, perhaps? You move removing the pad from collectpads from the release_pad method to the main collectpads collected function. Why? Is this neccesary? Seems strange... Please don't add > 80 column lines. Recursively calling your collectpads collected function looks... ugly, and it's really hard to convince myself that it can't get things wrong. It's just to clear out the queues at EOS, right? Perhaps you can factor that out into another function, and just call that. Otherwise, the patch itself looks pretty good. I haven't checked your tests, though.
Created attachment 70380 [details] [review] improved patch (In reply to comment #18) > 'has been removed' > > Also, can you change the comments on buffer/next_buffer to be clearer about > what they are, exactly? > > Please don't add > 80 column lines. Done. > Not clear why you're using g_atomic_ things. If that's neccesary, then probably > it's not sufficient - you should be taking the object lock, perhaps? Changed back into normal artithmetic, as there shouldn't be any threading issues. > You move removing the pad from collectpads from the release_pad method to the > main collectpads collected function. Why? Is this neccesary? Seems strange... It was a leftover from an earlier attempt to fix a queue-flushing issue. It's now gone. > Recursively calling your collectpads collected function looks... ugly, and it's > really hard to convince myself that it can't get things wrong. It's just to > clear out the queues at EOS, right? Perhaps you can factor that out into > another function, and just call that. I've factored all the actual work out of the collectpads callback into a new function _process_best_pad, which gets called once normally from the callback, and in a loop when we're flushing the queues. I've re-looked over it after writing it the other day, and I'm fairly certain I handle things like flow-return properly. > Otherwise, the patch itself looks pretty good. I haven't checked your tests, > though. If you have any suggestions for more tests I should add, let me know.
Ping? Any chance this can go into CVS soon? It's blocking bug #335635 which would allow for OGG retagging.
I'll leave this for Mike, Andy and Wim to decide - they're the guys that know oggmux best
I would only like to confirm that the bug still exists in gst-plugins-base-0.10.9. However, it seems that drawing ogg files through vorbiscomment fixes them.
Committed the unit test only so far (minus the EOS checking): 2006-09-04 Tim-Philipp Müller <tim at centricular dot net> Patch by: James Livingston <doclivingston at gmail.com> * tests/check/Makefile.am: * tests/check/pipelines/.cvsignore: * tests/check/pipelines/oggmux.c: (get_page_codec), (check_chain_final_state), (fail_if_audio), (validate_ogg_page), (eos_buffer_probe), (start_pipeline), (stop_pipeline), (eos_watch), (test_pipeline), (test_vorbis), (test_theora), (test_vorbis_theora), (test_theora_vorbis), (oggmux_suite): Add simple unit test for oggmux from #337026 with checking for the EOS flags disabled for the time being. Current oggmux seems to fail even the simple test_vorbis_theora() test case, which is another good reason to look at this patch ...
+1 vote on getting a gstreamer developer checking the patch. ;) Now that the unit test is in, it should be easier to test the rest of it. It would be nice to get OGG retagging in soon before the next gst-plugins-base release.
Ok, reviewed, looks sane, I added a comment or two and cleaned up some typos. Committing soon, after I've done a little more testing.
Ok, closing after committing. Thanks very much for working on this (and your patience).
You guys are bigger legends than Boonie! Huzzah!