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 654799 - Add force-key-unit support to mpegtsmux
Add force-key-unit support to mpegtsmux
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 0.10.23
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 607742
Blocks:
 
 
Reported: 2011-07-17 22:20 UTC by Alessandro Decina
Modified: 2011-12-01 07:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add force-key-unit support to mpegtsmux (9.41 KB, patch)
2011-07-17 22:20 UTC, Alessandro Decina
none Details | Review
new patch with some tests (21.66 KB, patch)
2011-07-20 11:05 UTC, Alessandro Decina
none Details | Review
updated patch and tests (21.41 KB, patch)
2011-09-15 22:57 UTC, Alessandro Decina
none Details | Review

Description Alessandro Decina 2011-07-17 22:20:17 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.
Comment 1 Alessandro Decina 2011-07-17 22:24:28 UTC
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
Comment 2 Alessandro Decina 2011-07-20 11:05:36 UTC
Created attachment 192294 [details] [review]
new patch with some tests
Comment 3 Olivier Crête 2011-07-20 17:32:56 UTC
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.
Comment 4 Alessandro Decina 2011-07-20 20:13:10 UTC
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.
Comment 5 Alessandro Decina 2011-09-15 22:57:32 UTC
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.
Comment 6 Andoni Morales 2011-10-05 12:09:58 UTC
(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.
Comment 7 Alessandro Decina 2011-12-01 07:32:56 UTC
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