GNOME Bugzilla – Bug 660260
isomp4: Add support for GstForceKeyUnit events
Last modified: 2018-11-03 14:44:51 UTC
Created attachment 197584 [details] [review] 0002-Add-support-for-GstForceKeyUnit-events.patch The flowing patch adds support for GstForceKeyUnit events to ismlmux. I have added a new property called 'fragment-method': * NONE: do not fragment (default value for muxers that are not the ismlmux) * TIME: fragment by time, set in the fragment-duration property (default value for the ismlmux, keeping the old behaviour) * EVENT: Uses the GstForceKeyUnit event to generate fragments. This muxer is different from the other ones (webm or mpegts) in the sense that it can mux several video qualities and the audio track is packed in a separate fragment. So to make it work the audio pad should be receiving GstForceKeyUnits too. I have tested this patch using Flumotion's smooth streamer[1] with a pipeline similar to: vsource ! keyunitsscheduler ! tee name=t ! h264enc ! ismlmux name=mux ! streamer t. ! h264enc ! mux. t. ! h264enc ! mux. asource ! keyunitsscheduler ! aacenc ! mux. This patch relies in the new API for GstForceKeyUnit events [1] https://code.flumotion.com/cgit/flumotion-fragmented-streaming/
Created attachment 200872 [details] [review] Fix smooth muxer for live streams The muxer rewrites timestamps from 0, so the timestamp check is not valid.
Created attachment 200971 [details] [review] Make sure event fragments starts with a key frame Make sure that fragments for GstForceKeyUnit event starts with a keyframe.
Created attachment 202539 [details] [review] Add support for GstForceKeyUnit events Updated patch that applies with current master
Created attachment 202582 [details] [review] Make sure fragments start with a keyframe And make sure fragments start with a keyframe
Review of attachment 202539 [details] [review]: Generally looks good but: ::: gst/isomp4/gstqtmux.c @@ +2014,3 @@ + pad_sync = qtmux->fragment_method != FRAGMENT_METHOD_EVENT && (sync + && pad->sync); + event_sync = pad->forcekeyunit_event != NULL && pad->sync; Note that there can be multiple forcekeyunit events (on different pads but also on the same), and that they can also contain a timestamp when this should happen. Also you should probably handle upstream forcekeyunit events too here
Thanks for the comments, I will work on queuing multiple events
Created attachment 252611 [details] [review] mp4mux: Add support for GstForceKeyUnit events
Thiago, what do you think?
It needs just a few small corrections, I am finishing the last tests.
Created attachment 252750 [details] [review] mp4mux: Add support for GstForceKeyUnit events
The muxer has been test in the following scnenario: x264enc ! qtmux ! dashsink name=sink lamemp3enc ! qtmux ! sink. Which is, with encoders that forwards back the event downstream, like the video one, and encoders that don't, like the audio one. This is important because events must be queued both in the src and sinkpads and there is some logic to handle duplicates based in the event "count". It has also been tested with the sink sending a burst of GstForceKeyUnit events (min-cached-fragments >= 1) at the start to test that queueing is done properly too.
Review of attachment 252750 [details] [review]: Looks good overall, a few questions below. ::: gst/isomp4/gstqtmux.c @@ +391,1 @@ Does this need to be available for all muxers? Isn't it better to register it only for the ones that actually can fragment? I don't think we have any specific properties right now but shouldn't it make sense to have? @@ +2082,3 @@ + if (GST_BUFFER_FLAG_IS_SET (buf, GST_BUFFER_FLAG_DELTA_UNIT)) { + GST_WARNING ("Next fragment will not start with a keyframe"); Does this make sense at all? Shouldn't we wait for a keyframe to start the next fragment? @@ -3086,0 +3169,42 @@ +static gint +_sort_fku_events (GstEvent * e1, GstEvent * e2) +{ ... 39 more ... Took me some time to realize that this function will enqueue on all pads if NULL is passed as the pad. Maybe add a comment about it? Otherwise quickly reading this may look like the loop is kind of useless. Is this useful somehow? I don't see any code passing NULL as the pad. If it should always enqueue to a pad I'd prefer to have the loop only to search for the correct sinkpad and have the actual enqueueing outside of the loop. What do you think? @@ +3294,3 @@ + gst_qt_mux_enqueue_force_key_unit_event (qtmux, + gst_event_ref (event), qtpad); + } How does this work exactly? Isn't enqueueing only done for sink pads?
Review of attachment 252750 [details] [review]: ::: gst/isomp4/gstqtmux.c @@ +391,1 @@ The patch needs to be rebased. This property is now deprecated for regular mp4 muxers. @@ +2082,3 @@ + if (GST_BUFFER_FLAG_IS_SET (buf, GST_BUFFER_FLAG_DELTA_UNIT)) { + GST_WARNING ("Next fragment will not start with a keyframe"); I would leave it there just to make sure that everything is ok, I found it very useful for debugging (maybe make it GST_DEBUG instead of GST_WARNING?) @@ -3086,0 +3169,42 @@ +static gint +_sort_fku_events (GstEvent * e1, GstEvent * e2) +{ ... 39 more ... This code tries to handle the scenario in which the muxer is muxing several tracks (it does not happen in DASH were you have a single track but it's a valid FMP4 use case). If the event is an upstream FKU (hence commming from the src pad), it enqueues the event in each sink pad, the first pad to reach the fragment limit creates the new fragment and the event is dequeued from the all the pads. If it's a downstream event its only queued in the corresponding sink pad. It could probably be documented in a better way :)
What happened??? the review got posted 4 times :(
Review of attachment 252750 [details] [review]: ::: gst/isomp4/gstqtmux.c @@ -3086,0 +3169,42 @@ +static gint +_sort_fku_events (GstEvent * e1, GstEvent * e2) +{ ... 39 more ... My point here is that I see that the enqueue done in the srcpad event handler doesn't pass null, but passes the src pad itself. Shouldn't it be passing null, instead?
Branch finally updated, rebased with master and passing NULL for the source pad (it was already implicitly passing NULL call gst_pad_get_element_private on the src pad). Could you please give a quick review?
Created attachment 259059 [details] [review] mp4mux: Add support for GstForceKeyUnit events
Created attachment 259061 [details] [review] mp4mux: Add support for GstForceKeyUnit events
Review of attachment 259061 [details] [review]: Patch looks good overall, just a few simple remarks below. ::: gst/isomp4/gstqtmux.c @@ +385,3 @@ g_param_spec_uint ("fragment-duration", "Fragment duration", + "Fragment durations in ms (used when the fragment-method='time')", + 0, G_MAXUINT32, 2000, Use the DEFAULT_FRAGMENT_DURATION here? @@ +398,3 @@ + GST_QT_MUX_FORMAT_ISML ? FRAGMENT_METHOD_TIME : + DEFAULT_FRAGMENT_METHOD, + G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS)); Can't we install properties conditionally so we only get those fragmented options on formats that accept fragmented output? @@ +2142,3 @@ } + gst_qt_mux_dequeue_force_key_unit_event (qtmux, pad); Why is it always removed? Can't it happen that some scenario other than the event caused the new fragment? Any scenario with a force=True? Additionally it causes a GST_WARNING when no events are queued, maybe only call if we know there is an event or if the event caused the fragment?
Andoni?
I have the patches ready, but I have to spend all the day in Windows, so I won't have access until later tonight
Created attachment 259773 [details] [review] mp4mux: Add support for GstForceKeyUnit events
Created attachment 259775 [details] [review] mp4mux: Add support for GstForceKeyUnit events
Created attachment 259776 [details] [review] mp4mux: deprecate the fragment-duration property for regular MP4
I think I have addressed all the issues now: * fragment-method property is only installed for fragmented formats * fragment-duration is deprecated for the other formats >Why is it always removed? Can't it happen that some scenario other than the event caused the new fragment? Any scenario with a force=True? With FRAGMENT_METHOD_EVENT, it can only happen with force=True, in which case we have been passed a null buffer, which I think only happens in some conditions with the first buffer.
Review of attachment 259775 [details] [review]: Thanks for the updates!
Review of attachment 259776 [details] [review]: Looks good
Is this still in progress?
These patches won't apply any-more. Please rebase and resubmit.
I think thiagoss rebased everything here https://bugzilla.gnome.org/show_bug.cgi?id=668094
What should we do with this? Is anybody going to move it forward or should we just close it?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/50.