GNOME Bugzilla – Bug 754224
audioencoder: timestamp headers same as first buffer and use duration 0
Last modified: 2018-11-03 11:41:00 UTC
Created attachment 310164 [details] [review] patch It makes sense that the headers being pushed out just before the first buffer should be timestamped like the first buffer. As for duration 0, I think this makes more sense then -1, and plays nicer with muxers etc.
Comment on attachment 310164 [details] [review] patch Same should be done for videoencoder too commit dd4d6d9ed54c2a63a7e45661519d9965417707c5 Author: Havard Graff <havard.graff@gmail.com> Date: Fri Aug 28 11:44:19 2015 +0200 audioencoder: timestamp headers same as first buffer and use duration 0 https://bugzilla.gnome.org/show_bug.cgi?id=754224
tpm@xps:~/gst/glib-master/gst-plugins-base/tests/check$ make pipelines/vorbisenc.check Running suite(s): vorbisenc 0%: Checks: 3, Failures: 3, Errors: 0 pipelines/vorbisenc.c:33:F:general:test_granulepos_offset:0: expected timestamp 99:99:99.999999999, but got timestamp 0:00:03.249870963 pipelines/vorbisenc.c:33:F:general:test_timestamps:0: expected timestamp 99:99:99.999999999, but got timestamp 0:00:00.000000000 pipelines/vorbisenc.c:33:F:general:test_discontinuity:0: expected timestamp 99:99:99.999999999, but got timestamp 0:00:00.000000000
I'm not convinced yet by either change really, esp. not the 'duration 0' one. And I also wonder if you looked at all of our muxers to see if this affects any of them?
Reverted the patch for now until we know what to do :)
Havard, any ideas from your side? *Why* do you need them with a valid timestamp, what goes wrong otherwise?
The concrete example from our side was flvmux. It will treat a header just like a buffer and embed a FLV-timestamp based on the timestamp of the header, causing all sorts of issues with extended RTMP timestamping being used. (-1 being a very large number in guint32)
From another point of view, -1 timestamps are not really a problem and should ideally be handled by elements just fine. That is, flvmux could interpolate (or just use the previous timestamp) whenever it encounters a -1.
If the headers will contain information that is globally applicable to any part of the stream at any time then I agree that the timestamping information might not be important. However if, say, the headers will contain information that might potentially change during the lifetime of the stream (change of bitrate or clock-rate, coding-type etc.) then it makes complete sense to place them in relation to the stream, so that they can be sorted and the stream still makes sense. Now, being a base-class, this implementation might cater for a lot of codecs that only needs (and supports) one single configuration, but it might *also* need to cater for a codec where the header-info might change runtime, and hence the base-class *should* (IMHO) timestamp its headers. As for the duration, I have no strong opinions, so I am happy to keep this -1 for headers, since duration is arguably something that only applies to the original (decoded) data but can be kept intact by the encoded buffers as well.
Let's remember that this patch broke existing code, so whatever the theoretical merits of it may be, it would be good to find a solution that doesn't do that.
(In reply to Tim-Philipp Müller from comment #9) > Let's remember that this patch broke existing code, so whatever the > theoretical merits of it may be, it would be good to find a solution that > doesn't do that. Did it break more then unit-tests that expects the headers to be timestamped with -1? I am happy to look into fixing any code that relies on the headers being -1, but in the end it is a theoretical decision that needs to be made for the base-class of how we want it to be.
I think there is merit to the idea that headers should be timestamped with the timestamp of the first buffer (or start time) rather than CLOCK_TIME_NONE. Duration does not make sense for these buffers so they should have a duration of CLOCK_TIME_NONE and not 0 (irrespective of what the unit test expects, we can change unit tests if we think the new behaviour makes more sense and won't break anything)
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/219.