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 721422 - Negative pad offsets don't work
Negative pad offsets don't work
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 1.2.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-03 18:58 UTC by Pedro Corte-Real
Modified: 2014-01-15 10:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test case for the bug (7.88 KB, application/octet-stream)
2014-01-03 18:58 UTC, Pedro Corte-Real
Details

Description Pedro Corte-Real 2014-01-03 18:58:36 UTC
Created attachment 265243 [details]
Test case for the bug

Even though the pad offset API takes a gint64, GstSegment.base is a guint64 so negative pad offsets have weird results. A test program is attached, just run it as:

./testsave file:///path/to/some/file.mkv outputdir

The program will split the mkv file into several and if it works properly each file will have the proper timestamps (start from 0s). With the current pad offset code this doesn't happen.
Comment 1 Sebastian Dröge (slomo) 2014-01-05 09:14:02 UTC
For this we probably have to add yet another offset field to GstSegment, this time a signed one. However adding a field there now is arguably breaking ABI as every code that calculated with the GstSegment fields directly instead of the gst_segment_* functions won't work properly afterwards.


One corner case for negative pad offsets is btw that the overall calculations yield a negative time. In that case I think it's safe to say that it's outside the segment?
Comment 2 Wim Taymans 2014-01-08 10:16:42 UTC
In theory it should work:

 1) positive offsets adjust the base time
 2) negative offsets adjust the start/stop time and possibly base time if we can

We only do 1) currently.

After implementing 2) this example still doesn't work because of matroskamux. It converts the buffer timestamps to running time twice, first in gst_collect_pads_clip_running_time() and then in gst_matroska_mux_handle_buffer().
Comment 3 Sebastian Dröge (slomo) 2014-01-08 10:32:11 UTC
(In reply to comment #2)
> In theory it should work:
> 
>  1) positive offsets adjust the base time
>  2) negative offsets adjust the start/stop time and possibly base time if we
> can
> 
> We only do 1) currently.

2) sounds like a good plan for this, yes

> After implementing 2) this example still doesn't work because of matroskamux.
> It converts the buffer timestamps to running time twice, first in
> gst_collect_pads_clip_running_time() and then in
> gst_matroska_mux_handle_buffer().

Indeed, but gst_collect_pads_clip_running_time() only adjusts the PTS and DTS, not the duration. Also qtmux has the same problem. This warrants another bug...
Comment 4 Wim Taymans 2014-01-08 11:05:13 UTC
first part:

commit 7f8c4dceb449b5fbb8bd8a8cfc58d218d2e0cd00
Author: Wim Taymans <wtaymans@redhat.com>
Date:   Wed Jan 8 11:28:04 2014 +0100

    Revert "matroskamux: Use the running time for container timestamps, not buffer timestamps"
    
    This reverts commit b3aa8755fe07639f22e4104f4932d769d6c9075a.
    
    We are already using the running-time because they were placed on the
    buffers with gst_collect_pads_clip_running_time(). Arguably it would be
    better to not modify the incomming buffers but collectpads seems to want
    to use absolute timestamps from the buffers for finding the best buffer
    (this can be changed with a custom compare function..).
Comment 5 Wim Taymans 2014-01-08 14:07:52 UTC
commit 6d3fc584d5573ae99c090bf3d169d4edee6be89a
Author: Wim Taymans <wtaymans@redhat.com>
Date:   Wed Jan 8 14:57:04 2014 +0100

    pad: use new segment offset method to apply the offset
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=721422

commit dc0176e4a072888aec029f008e6ab4357ad09e96
Author: Wim Taymans <wtaymans@redhat.com>
Date:   Wed Jan 8 14:54:47 2014 +0100

    segment: add method to offset the segment running-time
    
    Add a method that can apply an offset to the calculated running-time of
    a segment.