GNOME Bugzilla – Bug 654799
Add force-key-unit support to mpegtsmux
Last modified: 2011-12-01 07:32:56 UTC
Created attachment 192150 [details] [review] add force-key-unit support to mpegtsmux As per summary. This is still WIP, but i'm attaching the patch in case people want to comment or pick up the work.
A couple of things that are still missing: - use seqnums to handle both downstream-only and upstream-downstream force-key-unit scenarios - drain current streams before sending force-key-unit downstream
Created attachment 192294 [details] [review] new patch with some tests
Why does the muxer deal with upstream events? ? Why not push it to non-video pads too ? What's the use case for remembering it and waiting for the same one to come back instead of a different one ? Seems overly complicated to me.
Thanks for looking at this. I don't like some aspects of the patch either. In fact I decided to try this since I'm not entirely convinced about the proposed force-key-unit API and wanted to see how it works out in practice. To answer your questions: - why the muxer handles upstream events Imagine: a number of inputs... ! mpegtsmux name=mux ! multifilesink next-file=key-unit-event Say you want to create a new file and you want the new file to start with PAT, PMT and keyframes. How would you do it? The way it's implemented right now you would just send an event to mux:src. The muxer forwards the event upstream, waits until it has a downstream force-key-unit queued for each stream and then it pushes a downstream force-key-unit event to multifilesink. multifilesink closes the current file, opens a new one and then the muxer starts pushing again, starting with PAT and a PMT for each stream. - why not forward to audio pads too Because that's how the proposed force-key-unit API works right now. I haven't followed the whole discussion about it but it seems it was decided to implement it for video only. The latest patch adds API to libgstvideo in fact. This is probably what I dislike the most about the proposed API. I would just make force-key-unit a core event. - what's the use case for storing the upstream event in the muxer and waiting for downstream events with the same seqnum Because if you send an upstream force-key-unit event to the muxer and then quickly send another before all the muxed streams have replied with matching downstream events, you are going to get a bunch of downstream force-key-unit events from encoders and you need to decide how to handle it: * One option would be to queue upstream events - I don't like this one. * Another would be to refuse an upstream event if there's one being handled already - I could live with this. * The way it's implemented is: sending upstream events never fails, if two events overlap, the latest replaces the previous one - this is the option i personally prefer but i'm not married to it. Finally, i have implemented this in mpegtsmux now but once we come up with a stable API, i'd like to move the code to collectpads. 99% of the code is generic in fact, the only thing mpegtsmux specific is setting the flags to force PAT and PMTs to be output. That could be done with a collectpads vfunc or something.
Created attachment 196678 [details] [review] updated patch and tests New patch. This one is much simpler now that I added a running_time field to upstream force-key-unit events. Now when a downstream event is received, if there is an upstream event being handled already, the downstream event is ignored, else it's handled. Upstream events are never ignored. This logic supports both the following cases: a) source ! element_that_schedules_periodic_downstream_events ! encoder ! muxer ! sink b) ...a number of sources and encoders... ! muxer ! sink - with the application scheduling upstream events by pushing them on muxer:src a) is how I think Andoni is doing it. b) is how i'm doing it and how RTP could work. In both cases encoders and the muxer use the running time to know when to emit a keyframe/new headers.
(In reply to comment #4) > - why not forward to audio pads too > > Because that's how the proposed force-key-unit API works right now. I haven't > followed the whole discussion about it but it seems it was decided to implement > it for video only. The latest patch adds API to libgstvideo in fact. > > This is probably what I dislike the most about the proposed API. I would just > make force-key-unit a core event. First I dind't though it would make sense using this event in audio streams, but now I also think it should be a core event too. For most formats (like webm or mpegts), the fragmentation is driven by one of the streams, the video stream, because audio and video are packed in the same fragment. But in FMP4 it is not the case because audio and video can be packed in different fragments, and therefore con have fragments with different sizes and split points so you need the GstForceKeyUnit event in the audio stream too (see: https://bugzilla.gnome.org/show_bug.cgi?id=660260) And I think this is going to be direction taken in DASH too, so this event should be a core event.
commit 777c1f034f7389f81c5e29acbc2b3c64515f4b41 Author: Alessandro Decina <alessandro.d@gmail.com> Date: Sun Aug 21 11:01:37 2011 +0200 mpegtsmux: add support for force key unit events Handle force key unit events outputting PAT and PMT when all_headers=TRUE