GNOME Bugzilla – Bug 756564
segment: Don't return -1 for out-of-segment values in running/stream-time conversion functions
Last modified: 2015-10-26 08:54:47 UTC
+++ This bug was initially created as a clone of Bug #748316 +++ For 2.0, we should make the times signed and always return a value in those functions even if they are out of segment. gst_segment_to_running_time_full() already does this, but the same is needed in general for other situations too. Also returning -1 there already let to quite a few places where we then calculated with -1
Something similar can already be added for 1.8 IMHO. Just add _full() variants for the other three functions and make the non-full (empty?
Yes, but for 2.0 it should be done for the default functions so that every code has to be aware of it. Might also want to do that together with switching from plain integers to fractions.
.... Oh dear. I tried adding an unicode grinning face and Bugzilla found it wise to erase the rest of my comment. I found a bug in Bugzilla itself. Anyway, I was saying that the non-full variants will use the full ones. Then, the code for 2.0 will only need to be simplified a bit, which shouldn't be hard. IMHO even plain signed integers give us enough range (292 years or something? I don't think there could be a pipeline that would want to run for longer than that?), but it's definitely something to be discussed.
Created attachment 313469 [details] [review] First version of the _full variants patch for 1.8 Hi, I'm halfway through the patch - basically it only needs addition of the _full variant unit tests for stream time. But I did add all three remaining _full variants and made the non-full ones use those, and nothing is breaking. Please let me know if you have any comments so far, and meanwhile I'll add the missing unit tests. BTW, it looks very weird that we can return a negative position/running_time/stream_time but not feed it into the opposite function. OTOH we can't feed signed numbers into the functions without losing accuracy/consistency and/or breaking the API - because we already have one _full variant. A better solution is definitely needed for 2.0, but do you have any ideas about now...?
Created attachment 313734 [details] [review] _full variants and corresponding unit tests added Attaching the patch with all three _full variants and their corresponding unit tests. Fixes from https://bugzilla.gnome.org/show_bug.cgi?id=756810 have been taken into account. I am also committing some formula clarifications in the design docs. The code ended up being almost impossible to follow without these clarifications, and kind of difficult to follow even with these clarifications. In 2.0, the ability to return a negative number here will *greatly, greatly, greatly* simplify the code (get rid of all the res = -1 cases).
> Might also want to do that together with switching from plain integers to fractions I'd welcome that. IMO that would make working with NTSC-like frame rates much easier and also more accurate (e.g. 1001/30000 frame duration).
This change is causing errors in pipeline tests Running suite(s): GstPipeline 0%: Checks: 1, Failures: 1, Errors: 0 gst/gstpipeline.c:631:F:pipeline tests:test_pipeline_reset_start_time:0: Assertion 'position >= 50 * GST_MSECOND' failed In paused state, the position is coming as 0.
commit 66c0879908cb057990ff2eb628db857c651bb116 Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Oct 26 10:53:35 2015 +0200 segment: Return -1 if gst_segment_to_stream_time_full() considers the position not inside the segment Fixes GstPipeline unit test. https://bugzilla.gnome.org/show_bug.cgi?id=756564