[Bug 660260] isomp4: Add support for GstForceKeyUnit events

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=660260
  GStreamer | gst-plugins-good | git

Thiago Sousa Santos <thiago.sousa.santos> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #252750|none                        |reviewed
             status|                            |

--- Comment #12 from Thiago Sousa Santos <thiago.sousa.santos at collabora.co.uk> 2013-09-26 23:08:49 UTC ---
Review of attachment 252750:
 --> (https://bugzilla.gnome.org/review?bug=660260&attachment=252750)

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?

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.