After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 629196 - oggmux: re-tagging an Ogg Vorbis file may corrupt audio data
oggmux: re-tagging an Ogg Vorbis file may corrupt audio data
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other Linux
: Normal blocker
: 0.10.33
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-09-09 17:41 UTC by timmiz92
Modified: 2011-04-26 18:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
output of ogginfo with the input file (718 bytes, text/plain)
2010-10-10 13:50 UTC, Colomban Wendling
  Details
output of ogginfo with the output file (965 bytes, text/plain)
2010-10-10 13:50 UTC, Colomban Wendling
  Details
oggmux: do not skip a pageno at start (2.91 KB, patch)
2011-01-21 11:00 UTC, Vincent Penquerc'h
committed Details | Review
oggmux: use oggstream for less brittleness in recognizing headers (12.80 KB, patch)
2011-01-21 16:09 UTC, Vincent Penquerc'h
committed Details | Review
oggmux: flush every VP8 packet in its own page (1.17 KB, patch)
2011-04-25 10:44 UTC, Vincent Penquerc'h
rejected Details | Review

Description timmiz92 2010-09-09 17:41:59 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
Comment 1 timmiz92 2010-09-30 16:28:51 UTC
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
Comment 2 Jonathan Matthew 2010-09-30 20:54:59 UTC
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.
Comment 3 timmiz92 2010-10-01 07:46:05 UTC
(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.
Comment 4 Colomban Wendling 2010-10-10 13:50:08 UTC
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.
Comment 5 Colomban Wendling 2010-10-10 13:50:53 UTC
Created attachment 172047 [details]
output of ogginfo with the output file
Comment 6 Vincent Penquerc'h 2011-01-21 11:00:18 UTC
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.
Comment 7 Vincent Penquerc'h 2011-01-21 11:01:22 UTC
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.
Comment 8 Vincent Penquerc'h 2011-01-21 16:09:26 UTC
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.
Comment 9 Vincent Penquerc'h 2011-01-21 16:10:28 UTC
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.
Comment 10 Colomban Wendling 2011-01-28 17:05:35 UTC
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 11 Tim-Philipp Müller 2011-02-01 17:28:04 UTC
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
Comment 12 Tim-Philipp Müller 2011-02-02 17:39:12 UTC
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
Comment 13 David Schleef 2011-04-24 22:48:31 UTC
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.
Comment 14 Vincent Penquerc'h 2011-04-25 10:44:37 UTC
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."
Comment 15 Vincent Penquerc'h 2011-04-25 10:49:20 UTC
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...
Comment 16 Vincent Penquerc'h 2011-04-25 10:50:23 UTC
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 ?
Comment 17 David Schleef 2011-04-25 18:06:38 UTC
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.
Comment 18 Sebastian Dröge (slomo) 2011-04-26 14:14:21 UTC
Is there still a problem in oggmux?
Comment 19 Tim-Philipp Müller 2011-04-26 14:48:00 UTC
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?
Comment 20 Vincent Penquerc'h 2011-04-26 15:18:17 UTC
Gah, confused packets and pages, didn't I.
Sorry.
Comment 21 David Schleef 2011-04-26 18:02:14 UTC
Re: #13,

I was complaining about the problem fixed in #647856.  Nothing to see here.  You may go about your business.