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 432612 - [matroskamux] doesn't handle segments correctly
[matroskamux] doesn't handle segments correctly
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 631850 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2007-04-23 14:35 UTC by Sjoerd Simons
Modified: 2011-10-11 13:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
very naive patch :) (3.65 KB, patch)
2007-04-23 14:37 UTC, Sjoerd Simons
none Details | Review
set buffer timestamps to normalized buffer running time (3.25 KB, patch)
2011-02-16 19:35 UTC, Ognyan Tonchev (redstar_)
needs-work Details | Review

Description Sjoerd Simons 2007-04-23 14:35:16 UTC
Hi,

  Matroskamuxer converts the buffer of timestamps directly to the muxed stream time. Which leads to some issues when you feed it a stream that doesn't start at timestamp 0. Most notably, some frames can get timestamps which don't fit inside the files duration..

I'll attach a patch that handles this very naively, which makes stuff work for my application. But as i'm not sure how muxers should handle segments, it's probably wrong and there could be some regressions when you mixing multiple streams.

Is there any documentation about how muxers are supposed to handle segments?
Comment 1 Sjoerd Simons 2007-04-23 14:37:12 UTC
Created attachment 86848 [details] [review]
very naive patch :)
Comment 2 Mark Nauwelaerts 2007-08-05 10:38:49 UTC
IMO, muxers should do as little as possible (time) transformations behind the (hidden) scenes, but put streams into container as faithfully as possible (and out of them for demuxer).  Rather, gstreamer's pipeline architecture should be exploited to make sure the muxer's input is as "clean" as possible.
For example, rather than having a "naive patch" (or another one for that matter) (again) duplicate segment handling code in yet another element, it seems that the same effect (basically) can be achieved by using identity single-segment=true in the pipeline somewhere before matroskamux.  And if the effect that one wishes to achieve in a particular case may be different, than it would seem "cleaner" to put this in custom element (before muxer), or patch identity, or some other element, etc  [for example, for my general transcoding app, I have some element that (re)computes timestamps purely based on frame or sample count in stead of segments; should clearly not be in a muxer either].
In particular, pre-transforming a "strange" stream into a "nice" stream would settle this for any subsequent muxer, rather than trying to have all of them prepared for as many possible cases (with approximately duplicated code).
Comment 3 Tim-Philipp Müller 2007-11-13 17:26:54 UTC
I wonder it would make sense for muxers to keep track of the incoming newsegment event and then use gst_segment_to_stream_time().
Comment 4 Wim Taymans 2008-11-14 11:05:48 UTC
They should use the running_time even so that you can mux multiple segments one after another, like how a sink would play them.
Comment 5 Thiago Sousa Santos 2010-07-08 01:09:04 UTC
@Mark: It would indeed be nice to have a simpler timestamp handling on muxers level, but I don't like the idea of forcing people to add elements before muxers everytime, doesn't look intuitive to me. I think this is one of the things that could be done/handled at the GstPad level once we get tracking of GstSegments into it, or by a BaseMux class.
Comment 6 Sebastian Dröge (slomo) 2010-10-25 13:02:20 UTC
*** Bug 631850 has been marked as a duplicate of this bug. ***
Comment 7 Ognyan Tonchev (redstar_) 2011-02-16 19:35:45 UTC
Created attachment 181042 [details] [review]
set buffer timestamps to normalized buffer running time

This patch will set all buffer timestamps to normalized buffer running time.
Comment 8 Sebastian Dröge (slomo) 2011-03-03 14:23:17 UTC
Review of attachment 181042 [details] [review]:

As discussed on IRC using the running time for synchronization is the way to go. But no normalization of the running time should be done (to force it to start at 0) and the downstream buffers should have the timestamps stored inside matroska, i.e. the running time, instead of the upstream timestamps.

Also I'm not sure if we can change this behaviour now, people might assume the current behaviour. But at least for 0.11 this should definitely be done
Comment 9 Jonas Holmberg 2011-03-03 14:32:25 UTC
I guess most pipelines will not be affected by the change, since most src elements timestamp from 0 so running time equals buffer time, no?
Comment 10 Sebastian Dröge (slomo) 2011-03-03 14:36:25 UTC
No, for live pipelines this is not necessarily true or for pipelines that are running at a different playback rate or if you're concatenating two different streams for example.
Comment 11 Sebastian Dröge (slomo) 2011-03-03 14:39:54 UTC
I talked to Wim and yes, we can change this in 0.10. Pipelines where it makes a difference did not work without changing to the running time anyway
Comment 12 Mark Nauwelaerts 2011-03-22 20:12:20 UTC
commit dd19a7edad076535c917c3f5f3a6cda19804d6cf
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Tue Mar 22 19:36:21 2011 +0100

    matroskamux: use running time for synchronization
    
    Fixes #432612.
Comment 13 René Stadler 2011-10-11 13:02:00 UTC
This doesn't seem to work correctly. Fix coming...
Comment 14 René Stadler 2011-10-11 13:04:26 UTC
Now this should work as advertised:

commit 26d0812543dfc8ed9be018e69b860c39d9e0bded

  matroskamux: fix segment handling, so we actually use running time
    
  gst_matroska_mux_best_pad adjusts the buffer timestamp to running time using
  the segment stored in the pad's collect data. However, the event handler didn't
  pass the newsegment event on to collectpads' handler, so this segment was never
  updated at all.
    
  Re-fixes bug #432612.