GNOME Bugzilla – Bug 629196
oggmux: re-tagging an Ogg Vorbis file may corrupt audio data
Last modified: 2011-04-26 18:02:14 UTC
When tagging some Ogg Vorbis files the audio data might get corrupted. Tagging should leave the actual audio data unchanged. This bug can be reproduced for example with the Big Buck Bunny audio track found here (BigBuckBunny-stereo.flac): http://media.xiph.org/BBB/ Step 1: Encode the file to Ogg Vorbis using your encoder of choice. Step 2: Copy the file to somewhere and import the copy to Rhythmbox. Step 3: Tag the copied file in any of the tag fields. Step 4: Open up both the tagged copy and the original encoded file in a wave editor, such as Audacity. Step 5: Zoom in at the end of the files and compare the lengths of them. If you can reproduce this bug, the length of the two files should differ. Additionally, if you play the corrupted file in Rhythmbox, you might hear stutter at some parts. System: Ubuntu 10.04 (but the bug also happens on Ubuntu 10.10 beta) Rhythmbox 0.12.8
Tagged the bug with Rhythmbox version 0.13.x because the bug is still present in Ubuntu 10.10 RC with Rhythmbox 0.13.1
Does this command: $ gst-launch-0.10 filesrc location=input.ogg ! oggdemux ! vorbisparse ! vorbistag ! oggmux ! filesink location=output.ogg produce the same result? In any case, I think this is a GStreamer problem so I'm reassigning there.
(In reply to comment #2) > Does this command: > > $ gst-launch-0.10 filesrc location=input.ogg ! oggdemux ! vorbisparse ! > vorbistag ! oggmux ! filesink location=output.ogg > > produce the same result? > > In any case, I think this is a GStreamer problem so I'm reassigning there. I can confirm that it is a GStreamer bug since the command above produces the same result as tagging the file in Rhythmbox.
Created attachment 172046 [details] output of ogginfo with the input file I've found this bug while searching for related bugs before reporting a similar one, so I add the information here since I think it's closely related: When re-tagging OGG files, GStreamer seems to break a bit the tag data. This is visible with ogginfo (from the vorbis-tools) as some warnings, and actually results on reading problems with some applications (for example a Sansa Clip+ media player can't read the output file, but plays the original well). This bug happens with Rhythbox as well as with the above suggested GStreamer pipeline: gst-launch-0.10 filesrc location=input.ogg ! oggdemux ! vorbisparse ! vorbistag ! oggmux ! filesink location=output.ogg I join the output of ogginfo on both input and output files as examples.
Created attachment 172047 [details] output of ogginfo with the output file
Created attachment 178921 [details] [review] oggmux: do not skip a pageno at start Discontinuities are automatically signalled by oggdemux at the start of a new stream. When oggmux is yet to output actual data pages, do not signal these discontinuities. This patch may miss some actual discontinuities at the very start of a stream, but avoids the spurious missing pages when encoding happens normally. A better fix might involve finding a way to distinguish between actual data discontinuities and discontinuities merely marking the start of a new stream.
Note: this gets rid of the "WARNING: sequence number gap in stream 1. Got page 3 when expecting page 2. Indicates missing data." problem only.
Created attachment 178948 [details] [review] oggmux: use oggstream for less brittleness in recognizing headers Using the IN_CAPS flag for this is brittle, and will fail if either vorbisparse or vorbistag (which is itself based on vorbisparse) is inserted between oggdemux and oggmux. Possibly other elements too (eg, theoraparse, etc). Using oggstream ensures we Get It Right More Often Than Not.
This one fixes the "Negative or zero granulepos (0) on Vorbis stream outside of headers. This file was created by a buggy encoder" problem. Headers were being output twice when using vorbisparse or vorbistag.
I just built GStreamer and base plugins from Git with the two patches applied, and it works! Thanks for your work :) ogginfo don't show any error anymore (as you already reported), and a Sansa Clip+ reads the file perfectly well now -- as well as GStreamer (^^), MPlayer and VLC, but they never had problems. Looking forward for the fix in a release :)
Comment on attachment 178921 [details] [review] oggmux: do not skip a pageno at start Committed with small addition to improve re-usability (state change PLAYING -> NULL ->PLAYING): @@ -1607,6 +1613,7 @@ gst_ogg_mux_init_collectpads (GstCollectPads * collect) oggpad->new_page = TRUE; oggpad->first_delta = FALSE; oggpad->prev_delta = FALSE; + oggpad->data_pushed = FALSE; oggpad->pagebuffers = g_queue_new (); walk = g_slist_next (walk); commit b7664eae716a7e0026c3d4d7ca10897a30f6a45e Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Jan 21 10:56:00 2011 +0000 oggmux: do not skip a pageno at start Discontinuities are automatically signalled by oggdemux at the start of a new stream. When oggmux is yet to output actual data pages, do not signal these discontinuities in the ogg stream. This patch may miss some actual discontinuities at the very start of a stream, but avoids the spurious missing pages when encoding happens normally. A better fix might involve finding a way to distinguish between actual data discontinuities and discontinuities merely marking the start of a new stream. Fixes an issue with ogg page numbering (would skip a number for no reason, which then looks like a packet was lost somewhere) when re-muxing an ogg stream, e.g. when re-tagging in rhythmbox. https://bugzilla.gnome.org/show_bug.cgi?id=629196
commit 54c19ba6def4f9cead7e18a893c981e425f9405c Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Feb 2 17:30:15 2011 +0000 oggmux: free stream map caps when done commit 2eac43bd73ff405c6883c1c2476a90a123b52159 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Feb 2 17:23:43 2011 +0000 oggmux: keep IN_CAPS flag check for header buffers as fallback In case the ogg mapper doesn't handle all the accepted input formats (although it really should). Saves us error handling for that case though. Also log caps properly. https://bugzilla.gnome.org/show_bug.cgi?id=629196 commit 440002a137640db2c5d6613d9bf56dfff997eb07 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Jan 21 16:05:46 2011 +0000 oggmux: use oggstream for less brittleness in recognizing headers Using the IN_CAPS flag for this is brittle, and will fail if either vorbisparse or vorbistag (which is itself based on vorbisparse) is inserted between oggdemux and oggmux. Possibly other elements too (eg, theoraparse, etc). Using oggstream ensures we Get It Right More Often Than Not. https://bugzilla.gnome.org/show_bug.cgi?id=629196
This broke vp8 muxing. gst-launch videotestsrc num-buffers=30 ! vp8enc ! oggmux ! filesink location=out.ogv $ oggz-validate out.ogv out.ogv: Error: serialno 2144299614: Terminal header page contains non-header packet Problem appears to be because VP8 buffers in gstreamer do not exactly correspond to VP8 buffers in Ogg.
Created attachment 186581 [details] [review] oggmux: flush every VP8 packet in its own page http://people.collabora.co.uk/~slomo/webm/ogg-vp8.pdf says: "[...] Every Ogg packet of an On2 VP8 logical stream must contain exactly one VP8 encoded frame, no matter if the frame is a visible or invisible VP8 frame. A packet must not contain any additional data, multiple VP8 frames or parts of one or more VP8 frames."
There is a bug in liboggz additionally. If it does not know about a stream type (eg, VP8), it will default to believe it has 3 headers, as Vorbis/Theora do. This causes some of the logic to detect whether a header page contains non headers packets to fail. Still, the fix I made conveniently causes my copy of liboggz to not warn. Which might be another liboggz bug in itself, as it ought to moan we have headers with non zero granpos...
Though I do not see what you meant by "Problem appears to be because VP8 buffers in gstreamer do not exactly correspond to VP8 buffers in Ogg.", so there might be another problem here ?
The decoder creates an oggstream by the contents of the header buffers. The encoder should (and used to) create an oggstream by matching caps. Also, there's no reason to flush ogg pages for VP8. The spec doesn't say that.
Is there still a problem in oggmux?
David: you seem to have an idea what's wrong or should be done - could you elaborate please? What does "the encoder should (and used to) create an oggstream by matching caps" mean in this context?
Gah, confused packets and pages, didn't I. Sorry.
Re: #13, I was complaining about the problem fixed in #647856. Nothing to see here. You may go about your business.