Created attachment 84177 [details] [review]
Patch to enable custom event handling in collectpads

This patch is updated version of the patch from bug #340060 It enables to set custom event calbacks to pads handled by collectpads. With this patch some ugly hacks can be cleaned-up.

Created attachment 84178 [details] [review]
Implement gst_collect_pads_set_waiting

This patch add new function gst_collect_pads_set_waiting to collectpads, it enables transparently set pad to a non-waiting mode, when collecpads is not blocked when the particular pad has not data. Backward compatibility is maintained.

Created attachment 84179 [details] [review]
GstMuxer class implementation

This is a new class that ought to be a replacement for GstCollectPads in all muxers.

Patch for matroska to make use of GstMuxer will follow soon.

Well, the locking of pad_data is still not 100% thread safe. To make it work, I would probably need to make ref_data and unref_data from gstcollectpads.c public

I have noticed the following in the patches:

1) custom event handling:
it contains
  pads->func = NULL;
  pads->user_data = NULL;
but this does not seem to be used elsewhere ?
Is this perhaps a leftover of an older destroy_notify implementation, which changed because it affected the ABI of GstCollectData.
This patch breaks the ABI of GstCollectPads itself, because event_func and event_user_data are not stored inside abidata.ABI (so affect the overall struct size).  Also, they won't both fit in there, because the padding is not that much (GST_PADDING=4).  It may have to be worked around as well (storing in the GObject data).

2) gst_collect_pads_set_waiting:
Similarly, it breaks GstCollectsData ABI; extends beyond the padding, as there is no more room in there (there was already none for destroy_notify).

3) GstBaseMux:
Some nitpicking, it seems to use collect_data->abidata.ABI.new_segment directly, but this is marked as private (though a borderline)
[will need to look some more at BaseMux, it is a quite a lot ;-) ]

As a design point, though, there is a lot of "pass-through" code in GstBaseMux, e.g. _stop, _start, _add_pad, _set_event_function, destroy_notify handling, etc that either all go straight to GstCollectPads or pretty much do the same = have similar code as GstCollectPads, causing a lot of "duplication".
The real added value/intelligence is in the (essentially) pads_collected function interplaying with the event handling callback (settable in the patched GstCollectPads).
This could be reduced by modelling (say) GstMuxPads as a subclass of GstCollectPads.  Such one could be pretty directly/readily used as a replacement/enhancement of GstCollectPads in existing muxers, if they so desire.
At present, using GstCollectPads already means a muxer accepts "ok, this thing will take care of _chain and so on for me".  Handing it to GstMuxPads would then in addition mean "ok, and this one even comes along with a typically useful, more 'intelligent' collect deciding function that is decently event-aware as well".
If the gst_collect_pads_set_event function were made sort-of "virtual" (in GstCollectPads), then the using Muxer element could still use this to indicate to GstBaseMuxer that it too wants to have a peek at the events (e.g. tags), in addition to GstBaseMuxer fancy intelligent considering of (certain) events
[other "methods" might be useful to make "virtual" as well].
Beyond that, GstMuxPads could then also offer e.g. a callback custom compare_func.
So, in short/overall, override/extend the GstCollectPads functionality by inheritance rather than duplicate/wrap around it [and the real "core intelligence code" in GstBaseMux would then stand out in a good place in such a GstMuxPads].

Oops, silly mistake:
1) func and user_data are obviously for the collected_pads callback :-(

Nevertheless, the other comments hold ...

Created attachment 84404 [details] [review]
Patch for a different GstBaseMux implementation

At some point in the past, I wrote a BaseMux class, but never finished it.  I'm attaching the patch for reference and perhaps ideas.  It's not intended to replace or compete with the work you're doing, since I have zero time to work on this.

Hi Mark,
I wanted to post reply together with updated patches, but I am little short of time these weeks. Anyway that you for your reply. The patch is by no means ready. I have curently discovered some memory corruptions, and my test muxer hangs from time to time. Take it more as an API proposal for discussion.

> 1 and 2) gst_collect_pads_set_waiting:
> Similarly, it breaks GstCollectsData ABI; extends beyond the padding, as there
> is no more room in there (there was already none for destroy_notify).

Good point, I did not knew that the padding was so small :( I will misuse gobject properties or find another hack.

> 3) GstBaseMux:
> Some nitpicking, it seems to use collect_data->abidata.ABI.new_segment
> directly, but this is marked as private (though a borderline)
> [will need to look some more at BaseMux, it is a quite a lot ;-) ]

I was treating these variables more like "protected", but GstMuxer handles segments anyway so it can remember them too.

> As a design point, though, there is a lot of "pass-through" code in GstBaseMux,
> e.g. _stop, _start, _add_pad, _set_event_function, destroy_notify handling, etc
> that either all go straight to GstCollectPads or pretty much do the same = have
> similar code as GstCollectPads, causing a lot of "duplication".
_set_event_function, destroy_notify
I do not like that either, but I do not want element using GstMuxer to call GstCollectPads functions directly. GstMuxer may need to do something more than GstCollectPads in _stop, _start, _add_pad methods in future. But in the next versin I will implement this as #defines not dummy wrappers.

As for _set_event_function and destroy_notify I really need to wrap them because GstMuxer needs to handle events.

> This could be reduced by modelling (say) GstMuxPads as a subclass of
> GstCollectPads.  Such one could be pretty directly/readily used as a
> replacement/enhancement of GstCollectPads in existing muxers, if they so
> desire.

Yes, it is so. GstMuxer is subclass of GstCollectPads. I was not sure about the name GstMuxPads came to my mind, but I have choosen GstMuxer. May be GstMuxPads is better.

> So, in short/overall, override/extend the GstCollectPads functionality by
> inheritance rather than duplicate/wrap around 

I agree. But still I need some wrappers as I have not found better way to implement this in gobject system.

> [and the real "core
> intelligence code" in GstBaseMux would then stand out in a good place in such a
> GstMuxPads].

See reply to David

(In reply to comment #8)
> Created an attachment (id=84404) [edit]
> Patch for a different GstBaseMux implementation

Thanks David, thanks for your patch. It is more or less orthogonal to mine. I am extending GstCollectPads to automatically sorting incomming buffer by their time while correctly handling sparse strams. You are creating new BaseClass handling automatically state changes, pad addition/removal and collect_pads callback. I think that (as Mark suggested) these patches can be used together. 

Just a small update (ping);
Hoping/expecting to have some time these weeks; I'll be looking at using the above work for use in subtitle (video) overlay, and as such "challenge" the API a bit and see where it leads.
Bearing no objections ;-), results (presumably changes/additions here and here to the above) will be posted later, probably including some fixes to things noted earlier (e.g. padding issues).

OK, it's been a bit longer than I expected, but I do now have some results.
Specifically, it involves updates and fixes to GstCollectPads and GstMuxer that will be attached (replacing previous patches), along with a (dvd) subtitle overlay element (to be housed in -bad) that makes use of GstMuxer, and a patch for matroskamux to support muxing subtitles.

It has all been tested pretty well, be it either through some live playing (with a simple navseek based pipeline), or transcoding e.g. from dvd into mkv.
Moreover, it all actually works well ;-)  (if also bug #438610 is taken into account).  In fact, the only (remaining) problems ever experienced are:
- some stalling after a seek during playback, which is confirmed due to dvddemux failing to send newsegment updates for subtitles (so the stalling can't be helped in such a case)
- most subtitle packets are split across 2-3 buffers and dvddemux only sends the first fragment with a valid timestamp, so matroskamux can't be blamed for dropping the subsequent fragments (of course ending up with a corrupted subtitle stream) (will probably come up with a dvdsubparse element or so to handle this)

Well, enough moaning about dvddemux, back to GstMuxer.
Unless there are objections, next steps (in next days/weeks) will be:
- renaming GstMuxer to GstMuxPads; because GstCollectPads is a parent after all.
If one were to come up with a base class for a Muxer then Gst(Base)Mux(er) or    so would be a good name for it (see also above).  This would then be confusing and conflicting with the current GstMuxer naming.  Hence, GstMuxPads.
- perhaps eliminate some of the duplicate (copy-and-paste) documentation for functions by references to the corresponding GstCollectPads counterpart

As also said above, any comments etc are welcome.  It's more than a few lines, but the results should be worth it.

Created attachment 88215 [details] [review]
Implement collect_pads enhancements (event handling and waiting)

[replaces previous collectpads patches here which I can't obsolete]
* fix ABI breaking of GstCollectPads or GstCollectData;
  store in associated data (as with destroy_notify) 
  and use a bitfield for state rather than quite some (spacy) booleans
* modify some set_ signatures to be consistent with existing ones
* some (minor) changes for coding guideline and added some debug statements

Also includes some updates to (global section) documentation.

Created attachment 88217 [details] [review]
GstMuxer class implementation (revised)

[replaces/updates previous GstMuxer patch, which I can't obsolete]
* some additional API (e.g. allow a pad to be kept in waiting mode)
* perform an additional check that makes sure we don't run ahead of something we should switch to waiting for before handling a buffer
* re-factoring and small fixes
* some revised locking (and a race fix)
* changes for coding guidelines and some debug statements
* additional internal comments

Created attachment 88219 [details] [review]
Implement subtitle handling in matroskamux

[maybe this should go into a separate bug (or not?)]
* use new GstMuxer class to implement subtitle handling in matroska

Note that the new class takes care of most details, but muxer must still be aware that caps info on subtitles may come a lot later (after writing headers), and must cater for this delay.

Created attachment 88220 [details] [review]
Implement dvd subtitle overlay element

[maybe this should go into a separate bug (or not?)]
* Overlays output of dvdsubdec (dvd subtitle) onto (equally sized) video
* Uses new GstMuxer class

In the long(er) run, perhaps there could be some unification with textoverlay & co ?

Hi Mark, thanks for your excellent work. So ar I have only shortly peeked at the patches and they seem good. I will try do some more review ASAP. The idea to rename the class to GstMuxPads seems sensible. I am for it.

I would also suggest to spam^H^H^H^Hinform the mailing list about this API addition proposal to get more audience.

Created attachment 89207 [details] [review]
Implement dvdsubparse element

As mentioned above, dvd subtitle streams may be demuxed in fragmented form, but they can't be transcoded/muxed that way.  This element parses the stream into the proper packets that should go e.g. into an individual Matroska block.

btw; some further testing has lead to a few fixes in GstMuxer and subtitle overlay element; they will be incorporated shortly in the renamed versions (after the release of core due soon).

Created attachment 91113 [details] [review]
Implement collect_pads enhancements (event handling and waiting)

* Basically previous patch, now upgraded to latest (0.10.13) release

Created attachment 91116 [details] [review]
GstMuxPads implementation

* renamed GstMuxer to GstMuxPads
* some minor fixes
* changes in documentation descriptions

Created attachment 91118 [details] [review]
Implement subtitle handling in matroskamux

* adjusted to use the renamed GstMuxPads
* now also puts some data in the CodecPrivate section of VOBSUB stream
  (only palette for now)
[presumably, size info is also interesting here, and it would make sense for  video/x-dvd-subpicture caps to also carry width/height info rather than have this hardcoded to a single size in dvdsubdec]

Created attachment 91119 [details] [review]
Implement (dvd) subtitle overlay element

* adjusted to use the renamed GstMuxPads
* minor fixes (prevent segfault at EOS)

Created attachment 91123 [details] [review]
Implement dvdsubparse element

* latest version of dvdsubparse element

[technically independent from other elements here, but needed to have the subtitle stream parsed into proper chunks to be muxed]

At last, the above patches take care of some next steps indicated earlier.
Specifically;
- renamed GstMuxer to GstMuxPads
- changed some documentation here and there
[- and some minor fixes as well]

As such, at this time, the above patches put together can be used to (e.g.):
- transcode DVD into Matroska, including (1 or more/all) subtitle streams
- playback (using e.g. gst-launch pipeline) of (resulting) Matroska file with subtitle overlay support
[- btw; resulting file also plays fine with e.g. mplayer]
- similar support for subtitle overlay for DVD playback (though seeking may be problematic in this case if dvddemux sometimes fails in providing segment updates)

So much for the "easy part", now to get this committed (with give or take some comments/feedback along the way) ...

I understand that tehre is somoe resitance to apply this quite huge CollectPads patch. Would it be more acceptable to create a new CollectPads2 element which would have the same API (but different ABI to avoid the ugly hacks) as patched CollectPads?

Would it increase chance to have this patch commited if a separate class CollectPads2 would be created?

Ok, what about making GstMuxer (IMHO bad name, sounds like a base class for muxers) GstCollectPads2, i.e. stop inheriting GstCollectPads and have it all in GstMuxer. Then get it ready and usable, get an API review and let's get it in core (+ use it every possible)?

AFAIK Wim talked about redesigning/dropping GstCollectPads for 0.11 anyway so why not just create something good separate from GstCollectPads now already? :)

What do you think?

What would it mean (or does it mean) to (possibly) drop GstCollectPads for 0.11?
Although it has indeed evolved into an implementation/design with some ugly (ABI) hacks and some other (lock?) bumps, the basic "collect and call-back" principle seems pretty OK and serves a number of elements (notably muxers) well.
Assuming sufficiently similar streaming (API) etc in 0.11, so it should serve there as well.

So, if not GstCollectPads (principle) for 0.11, how should muxers (a.o.) get by in re-usable fashion?

If, however, still a GstCollectPads approach, there is (as suggested) indeed a great deal to be said for a (e.g.) GstCollectPads2, which (unimaginatively keep-it-simple) could be a "copied", cleaned and (MuxPads) enhanced version of its predecessor.  I may then make a stab at this (not right now ;-) ), though it then evidently remains to be seen whether that will be up to core taste/specs.

It would mean to replace it with something better designed without hacks that would have the same use case but hopefully would be much better ;) The principle etc should be the same I guess as it's really useful.

Creating a GstCollectPads2 with the GstMuxer stuff would IMHO the way to go for now, with removing all hacks from GstCollectPads2 and making stuff a bit cleaner

Cleaning up CollectPads is clearly a goal for 0.11, possibly merging in a lot of features from the muxer class. I can think of some things that are needed:

 - tracking of time on the sink pads
 - automatically selecting the data from the pad with earliest time (for muxers)
 - some things to make adder do synchronized mixing

Nothing is stopping us from moving CollectPads2 (or MixerPads) next to the existing object and see where we go, no need to wait for 0.11.

Created attachment 106641 [details] [review]
Implement subtitle handling in matroskamux

[just for the record for now, pending GstCollectPads2 ...]

* Same functionality as previous (matroskamux) patch.
* Updated to latest 0.10.7 release.

Created attachment 107392 [details] [review]
GstCollectPads2 proposal

As alluded to above, this is a stab at GstCollectPads2, basically a copy of the old one, merged with GstMuxPads, along with various cleanups.  In particular, locking has been revised for MT-safety.

Some other thoughts:
* the GstCollectPads unimplemented API part might be removed altogether (given that it still hasn't happened anyway after quite some time)
* the "higher level" helper API methods (e.g. _read_buffer) might not be essential
* minor "API contract" change in _set_flushing (due to locking and MT-safety); this could be avoided, but then a lot more properties would also need LOCK protection (and LOCK to be taken much more)
* see also comments and NOTEs in files

Created attachment 107393 [details] [review]
GstCollectPads2 based subtitle handling in matroskamux

Same as similar earlier patches, but now using GstCollectPads2
(rather than using GstMuxPads based on a patched GstCollectPads)

Created attachment 130347 [details] [review]
GstCollectPads2 based subtitle handling in matroskamux

Long time no see, but still there ... updated to latest release.

Mark, I don't know if you have already done this but I decided to start a branch at http://cgit.freedesktop.org/~thiagoss/ to start porting muxers to GstCollectPads2.

oggmux is already there (maybe some minor stuff to be changed to add subtitles, but it seems to be working as the 'old' one)

I'll grab your matroska patch and push there too.

I've tried the oggmux patch from Thiago and it works fine here.
There's only one thing I'm not sure about: in order to set a subtitles
pad to non waiting mode, the pad has to be unlocked, and newly created
pads are set to locked by default. Mark, this comes from your patch,
what is this lock used for ? It seems to be used only for allowing or
not the change in waiting state. Is it there to prevent some kind of
race condition ?

GstCollectPads2 comes with a default algorithm that sets pads to waiting or non-waiting based on incoming timestamps and new-segment events.  Since non-waiting should typically only apply to subtitle pads, a muxer can set non-subtitle locked (so it will never be set to non-waiting) (bearing in mind here that GstCollectPads2 itself has no notion of subtitle pad or not).

In particular, a straight port from GstCollectPads to GstCollectPads2 will never have pads go non-waiting (since as you note the default is locked in waiting).

Sebastian, IIRC you recently worked on this?  Any updates (git repo) ?

Created attachment 193190 [details] [review]
Copy GstCollectPads2 files from gst-plugins-good, add to gstreamer library

Add GstCollectPads2 to gstreamer library, in preparation for porting mpegtsmux to use it.

There may be a better way to do this using a git command that would preserve some history, but I simply copied files from gst-plugins-good/gst/videomixer into gstreamer/libs/gst/base/, and modifying Makefile.am to include it in library build.

Created attachment 193192 [details] [review]
Port mpegtsmux to use GstCollectPads2

This ports mpegtsmux to use GstCollectPads2.

This doesn't work as well as older GstCollectPads, yet, for my particular application, trying to mux H264 and AAC streams from independent sources ximagesrc and autoaudiosrc.

I still need to do something to support gaps in ximagesrc (probably a plugin to add newsegments), but even with something always moving on the desktop display (so no gaps in ximagesrc), this port locks up within a second or two, whereas old mpegtsmux with old GstCollectPads keeps on going.

I'll keep poking at this, but wanted to share now in case someone else wanted to try it in different sort of application, and/or otherwise debug or finish any porting work that might be needed.

-- Ben --

Created attachment 193450 [details] [review]
Corrected patch to port mpegtsmux to GstCollectPads2

Found a couple things that were in the original mpegtsmux_choose_best_stream(), that did not get moved to GstCollectPads2, so adding them with this patch.  These include prepare_func() support and some timestamp manipulation.

This works much better!

-- Ben --

We should replace GstCollectPads with GstCollectPads2 in 0.11/1.0. GstCollectPads2 might not be perfect yet but it's definitely an improvement over the old one and it doesn't make much sense to keep the old one in another stable release.

Created attachment 193647 [details] [review]
Port mpegtsmux to use GstCollectPads2, 3rd version

Ooops, yet again found a bug, this time some confusion on my part about unreffing best->queued_buf (from original mpegtsmux).  best->queued_buf is not necessary anymore, since GstCollectPads2 passes the buffer as a call parameter to mpegtsmux_buffer_cb().

I removed queued_buf from struct MpegTsPadData, changed name of "buffer" passed from GstCollectPads2 to "queued_buf" to describe its role, and fixed the unref bug.

Created attachment 194131 [details] [review]
New plugin to insert newsegment to support sparse streams

This is an attempt at a very simple plugin to insert a newsegment, with start time "infinitely" (well, almost 300 years) in the future, just after the very first buffer passes through it.

This supports a use case such as a video stream sourced from ximagesrc, which can have sporadic gaps when the screen doesn't change.  We want the accompanying audio stream to continue to be muxed and sent downstream, even when there are gaps in the video.

This new plugin approach follows some discussion on IRC on 7/29/11 with __tim and mnauw.  There may be a more sophisticated way for inserting newsegments, but this "brain-dead" approach seems to work well for me for now.

Actually, this is only part of the solution that I've found to work for me.  After adding this sparsesegment plugin, I discovered that GstCollectPads2 still locked up, and blocked audio ... reason was that the sparse stream was chosen as "best" one time, which made the continuous stream WAIT, and there was nothing to wake up the continuous stream in a timely way (there was only one other stream, and it was sparse).

I've put together a new algo for GstCollectPads2 to determine when "collection" occurs, and to separately determine whether to make a buffer/pad WAIT (making sure there's another pad that can wake it!).  New algo is based on bitfields, rather than totals, so it keeps track of *which* pads are queued/sparse/eos (rather than simply how many in each category).  As an extra benefit, it is simpler than the current/old algo.  Will post patch here soon.

Created attachment 194175 [details] [review]
Change GstCollectPad2 collection and WAIT algos to use bitfields

Use bitfield algo to determine pads are collected in GstCollectPads2 ...

Instead of using totals (e.g. numpads, queuedpads), use bitfields to preserve
info about *which* pads have characteristics of queued (has data), sparse
(other pads should not wait for this pad), and eos.

The bitfield provides a simpler, more robust way to determine when "collection"
occurs, with no need for tricky maneuvering of "waiting" status to properly
increment/decrement the previously-used totals.

The bitfield also provides a robust way to decide whether to WAIT a pad,
making sure that another pad provides a reliable wakeup in a timely way.

This solves a problem, especially when multiplexing only 2 streams, in which
a continuous stream could be put on WAIT when the only other stream was a
sparse stream.  For example, multiplexing a continuous audio stream with
a gap-filled video stream sourced from ximagesrc, which has gaps when the
screen does not change ... if an audio buffer got WAITed because the "best"
buffer was video, it might be a long time before that audio buffer got awoken
by arrival of another video buffer.

Created attachment 194178 [details] [review]
Allow sparse (non-waiting) mode for any pad in mpegtsmux

Change mpegtsmux's call to gst_collect_pads2_add_pad(), so wait-lock is FALSE.  This enables "sparse" mode for any pad, controlled by the relationship of newsegment start times vs. buffer timestamps for a given stream.

W.r.t "a problem" as mentioned above it seems (FTR) that it is due to the way GstCollectPads2 is being (ab)used here, in particular the newsegment handling approach, or rather lack thereof.

Specifically, the (as said) simple approach of the "newsegment plugin" consists of only sending a single newsegment event.  As such, there is indeed and evidently "a long time before that audio buffer got awoken" as it is generally meant to be awoken by newsegment event activity that is totally discarded here.

However, as it stands, afaics it can be pretty easily patched to handle such use as well, e.g. add a gst_collect_pads2_recalculate_full (pads) at the end of gst_collect_pads2_default_collected (and it should again release the sparse stream from being waited on).

As such, the 'bitfield algo' is not so much an alternative algo, but an alternative implementation that (incidentally) happens to sort-of implicitly include the above easy fix (as side-effect).

Comments/thoughts that spring to mind regarding it:
* afaics (quickly) it should work
* ... except e.g. for only being able to handle a finite number of pads in a bitfield.  And as such there is some problematic code in _add_pad that dumps another pad if overrun.  Probably there will never be so many pads, but that's the tricky stuff with generic library code (trade off between fast/simple and general/complicated).
* don't really like or see the need to rename _set_waiting to _set_sparse.  A stream is (considered) sparse or it is not (typically so designated in developer speak).  OTOH, the _set_waiting (likely) really regularly changes a transient state, i.e. whether it needs to be waited for or not (it does not flip sparse or not back and forth).

Also, btw, while it may (or not?) work for mpegtsmux, it is generally not recommended to have all pads considered as sparse.  This could cause buffer collection to start before all pads have a buffer, so before all pads have been negotiated.  Hence, a muxer that needs to write some header may have a lot of info for doing so still missing when it needs it.  Therefore, it is best to have some streams as locked/continuous (e.g. video/audio) and others not-locked/sparse along with special purpose code that can handle header re-writing.

Of course, if no "header", then likely not so much an issue (at all).

Thanks for comments, Mark.

What would you recommend as good behavior for newsegments in my case of a sparse video stream, in which we never know, in advance, how much time will be between frames?

(In reply to comment #46)
> However, as it stands, afaics it can be pretty easily patched to handle such
> use as well, e.g. add a gst_collect_pads2_recalculate_full (pads) at the end of
> gst_collect_pads2_default_collected (and it should again release the sparse
> stream from being waited on).

It works!  I'm happy with this solution ... wish you'd mentioned it a while ago ;-) ... would you be willing to submit a patch?  If not, I could, but I think you would do a better job of understanding any side effects, etc.

Thanks for your comments about fast/simple vs. generic/complicated code ... I tried using GArray to make the bitmasks extendable ... and of course it blew up to be bigger than I would have liked, so I'm happy you figured out another way.

I'm still wondering how to use newsegments in a more intelligent way than I have, but I'm completely baffled by my use case of a live source with non-predictable frame time/duration, AND trying to minimize latency ... the "brain-dead" approach in my sparsesegments plugin works for me (and it does wait for the first frame before inserting the far-in-the-future newsegment, so header is complete), but please let me know if you have a better suggestion.

IIRC, newsegment events are usually sent (e.g. by demuxers) with a 0.5s interval (as "magic value"), which happens to work/interact well with usual queues etc.

But there is not really a right or wrong here.  With the "fix" (or whatever) in place, collectpads2 should be able to handle the "brain-dead" approach as well (so it appears) and that may be good enough.

The only "real" difference/impact with using an "infinite" segment is that it prevents collectpads2 from (ever) waiting on (sparse) video, which could lead to some out-of-order timestamping.  That is, audio with ts X might already be picked, and only then video with ts Y < X might arrive and be picked.  In a live use-case such as this, however, X - Y is probably small and the container format and/or subsequent playback are (probably?) not going to trip over that.

So, this simple (newsegment) approach may very well suffice in this particular scenario, whereas other cases (e.g. non-live (re)muxing) require a more subtle one.

Created attachment 197572 [details] [review]
collectpads2: avoid hanging in case of sparse newsegment event

This the (give or take) one-line patch to current GstCollectPads2 as alluded to above that should have it handle less (than so far expected) newsegment events as well.

AFAICS, should not have any negative side-effects, other than some extra cpu cycles.

Review of attachment 197572 [details] [review]:

Looks great, thanks.

collectpads2 is now in libgstbase. We should create new bugs for the mpegtsmux port and other muxer ports to collectpads2.


commit ce88f417b1c6ebb450e53efd272cf3649cf894c0
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Tue Sep 27 15:48:52 2011 +0200

    collectpads2: avoid hanging in case of sparse newsegment events
    
    ... in the extent that a non-waiting pad (so indicated by newsegment)
    turns out to provide the best buffer, which is then forced to waiting
    for book-keeping purposes, but that should only be temporary.
    
    See bug #415754.

commit 2a132759855b5f27388ea005d4be3511eefae0a7
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Oct 28 09:38:35 2011 +0200

    collectpads2: Use G_DEFINE_TYPE instead of GST_BOILERPLATE

commit 1b1b7931b09205778b04c843f0726bc882f63843
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Oct 28 09:35:50 2011 +0200

    collectpads2: Add to the documentation

commit 5b12790c26378bdd377f5ddf1b341799a3fdea54
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Oct 28 09:26:11 2011 +0200

    win32: Add new collectpads2 API

commit 6f231f89d6eb814872b4773d1e7cc3835cd3fa93
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Oct 28 09:18:55 2011 +0200

    base: Add collectpads2
    
    This handles muxing of sparse/subtitle streams and has
    lots of cleanup. Still missing is special support for
    live streams but this can be added later without breaking
    API/ABI.
    
    Based on the version from the videomixer plugin.
    https://bugzilla.gnome.org/show_bug.cgi?id=415754