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 743900 - oggdemux gets first packet timestamp wrong - theora
oggdemux gets first packet timestamp wrong - theora
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other Linux
: Normal blocker
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 729198
 
 
Reported: 2015-02-03 00:10 UTC by Jan Schmidt
Modified: 2015-03-16 12:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Example file (5.79 KB, video/ogg)
2015-02-03 00:10 UTC, Jan Schmidt
  Details
fix wrong first granule on streams with a hole at start (4.83 KB, patch)
2015-02-04 17:18 UTC, Vincent Penquerc'h
none Details | Review
fix wrong first granule on streams with a hole at start (4.92 KB, patch)
2015-03-11 09:14 UTC, Vincent Penquerc'h
committed Details | Review
fix regression on streams with clipped start data (1.24 KB, patch)
2015-03-16 11:58 UTC, Vincent Penquerc'h
committed Details | Review

Description Jan Schmidt 2015-02-03 00:10:33 UTC
Created attachment 295987 [details]
Example file

For files which start with a non-zero granulepos, oggdemux outputs the first packet with timestamp 00:00:00.

Attaching an example file for which oggdemux correctly calculates a segment of 0:00:02.000000000 - 0:00:03.000000000, but then outputs buffers with 0sec, 2.2sec, 2.4s, 2.6, 2.8.
Comment 1 Vincent Penquerc'h 2015-02-04 17:18:21 UTC
Created attachment 296144 [details] [review]
fix wrong first granule on streams with a hole at start
Comment 2 Jan Schmidt 2015-02-05 04:34:32 UTC
Review of attachment 296144 [details] [review]:

Thanks for the patch! This fixes the timestamping for my test file, but seems to break vorbis timestamping by modifying the duration on the first outgoing packet.

Before:
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (30 bytes, dts: none, pts: none, duration: none, offset: 0, offset_end: -1, flags: 00000440 discont header ) 0x7fedcc007220
New clock: GstSystemClock
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (165 bytes, dts: none, pts: none, duration: none, offset: 0, offset_end: -1, flags: 00000400 header ) 0x7fedcc007110
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (3714 bytes, dts: none, pts: none, duration: none, offset: 0, offset_end: -1, flags: 00000400 header ) 0x7fedcc007000
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1 bytes, dts: none, pts: 0:00:00.000000000, duration: 0:00:00.000000000, offset: 0, offset_end: 0, flags: 00000000 ) 0x7fedcc007110
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1 bytes, dts: none, pts: 0:00:00.000000000, duration: 0:00:00.002902494, offset: 2902494, offset_end: 128, flags: 00000000 ) 0x7fedcc007220

After:
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (30 bytes, dts: none, pts: none, duration: none, offset: 0, offset_end: -1, flags: 00000440 discont header ) 0x7f931c007220
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (165 bytes, dts: none, pts: none, duration: none, offset: 0, offset_end: -1, flags: 00000400 header ) 0x7f931c007110
New clock: GstSystemClock
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (3714 bytes, dts: none, pts: none, duration: none, offset: 0, offset_end: -1, flags: 00000400 header ) 0x7f931c007000
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1 bytes, dts: none, pts: 0:00:00.000000000, duration: 0:00:00.013061224, offset: 13061224, offset_end: 576, flags: 00000000 ) 0x7f931c007110
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1 bytes, dts: none, pts: 0:00:00.013061224, duration: 0:00:00.002902494, offset: 15963718, offset_end: 704, flags: 00000000 ) 0x7f931c007220

This causes the decoded vorbis to start 13ms late:

/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1024 bytes, dts: none, pts: 0:00:00.013061224, duration: 0:00:00.002902494, offset: -1, offset_end: -1, flags: 00000040 discont ) 0x7f751c0252f0
/GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain   ******* (fakesink0:sink) (1024 bytes, dts: none, pts: 0:00:00.015963718, duration: 0:00:00.002902494, offset: -1, offset_end: -1, flags: 00000000 ) 0x7f752001c6d0
Comment 3 Vincent Penquerc'h 2015-02-16 10:34:39 UTC
Oops, fixing.
Comment 4 Vincent Penquerc'h 2015-02-16 17:22:50 UTC
I think this is actually correct.

On a sample file here, the first data page contains 30 packets, and has a granpos of 15360. The sum of the durations of the 30 packets is 15360.
The first packet has duration... well, this one's a bit odd. The packet itself has duration 128, but oggstream says 0, as it's the first packet in the stream (see ext/ogg/oggstream.c, in packet_duration_vorbis, for the use of pad->last_size). I assume that's per the Vorbis spec. The 15360 total duration is counting that first duration as 0.

Now, the first packet (with duration 0) gets a pts of 0, and a duration of 0. The second packet has a pts of 0 too, an a duration of 576-0, 576, which is 0.13ish at 44100 Hz. This appears to be correct to me.
Comment 5 Vincent Penquerc'h 2015-03-11 09:14:23 UTC
Created attachment 299080 [details] [review]
fix wrong first granule on streams with a hole at start

I think I got it. Some state in oggstream that was overriding the first packet's duration. I think this is because the first packet initializes the decoder's state. It was correctly set to 0 when accumulating the packet lengths, but then using the duration from the stream when it got to the original code.
Comment 6 Vincent Penquerc'h 2015-03-11 14:46:16 UTC
That takes care of the issue you found. Please reopen if there's still something.

commit ca136e3648d13fd81c54dc0fe7f5d018018a379d
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Wed Feb 4 17:13:44 2015 +0000

    oggdemux: fix wrong first granule
    
    The code was using the first nonnegative granulepos to seed the
    granule tracking, which appeared to work since headers have zero
    granulepos. However, this does not work for files with a hole at
    start, which are common in live streaming.
    
    The correct behavior is to look for the first granule, and subtract
    the duration of all the packets finishing on this page.
    
    The function which does this relies on the fact that the ogg_stream
    structure can be duplicated by shallow copy, in order to pull the
    packets from the first page(s) on the copy without affecting the
    original stream state.
Comment 7 Jan Schmidt 2015-03-11 14:59:09 UTC
Thanks, I was just about to test that patch.

Don't forget to add 'https://bugzilla.gnome.org/show_bug.cgi?id=743900' or so to your commits when you're closing bugs...
Comment 8 Jan Schmidt 2015-03-11 15:10:34 UTC
Seems to work perfectly. Thanks for hunting it down :)
Comment 9 Sebastian Dröge (slomo) 2015-03-15 15:33:40 UTC
This breaks playback of short Ogg/Vorbis files like this: https://github.com/sdroege/gst-player/blob/master/tests/media/audio-short.ogg

Or also Ogg/Vorbis/theora files that are short:
https://github.com/sdroege/gst-player/blob/master/tests/media/audio-video-short.ogg

It complains "No valid frames decoded before end of stream" :)
Comment 10 Vincent Penquerc'h 2015-03-16 11:58:29 UTC
Created attachment 299502 [details] [review]
fix regression on streams with clipped start data
Comment 11 Vincent Penquerc'h 2015-03-16 12:02:00 UTC
This patch fixes the regression on these two files.