[Bug 787922] splitmuxsink: add a "split-now" action signal

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=787922

--- Comment #22 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 363378
  --> https://bugzilla.gnome.org/attachment.cgi?id=363378
An implementation of the action signal described.

Some more nitpicking :)


>@@ -367,6 +389,7 @@ gst_splitmux_sink_finalize (GObject * object)
>   g_list_foreach (splitmux->contexts, (GFunc) mq_stream_ctx_unref, NULL);
>   g_list_free (splitmux->contexts);
> 
>+
>   G_OBJECT_CLASS (parent_class)->finalize (object);
> }

Please remove the extra newline added here.


>@@ -1227,7 +1250,11 @@ handle_gathered_gop (GstSplitMuxSink * splitmux)
>   /* Timecode-based threshold accounts for possible rounding errors:
>    * 5us should be bigger than all possible rounding errors but nowhere near
>    * big enough to skip to another frame */
>-  if ((splitmux->fragment_total_bytes > 0 &&
>+
>+  GST_SPLITMUX_LOCK (splitmux);
>+
>+  if (splitmux->split_now ||
>+      (splitmux->fragment_total_bytes > 0 &&
>           ((splitmux->threshold_bytes > 0 &&
>                   queued_bytes > splitmux->threshold_bytes) ||
>               (splitmux->threshold_time > 0 &&
>@@ -1236,6 +1263,7 @@ handle_gathered_gop (GstSplitMuxSink * splitmux)
>                   splitmux->reference_ctx->in_running_time >
>                   splitmux->next_max_tc_time + 5 * GST_USECOND)))) {
> 
>+    splitmux->split_now = FALSE;
>     /* Tell the output side to start a new fragment */
>     GST_INFO_OBJECT (splitmux,
>         "This GOP (dur %" GST_STIME_FORMAT
>@@ -1259,6 +1287,8 @@ handle_gathered_gop (GstSplitMuxSink * splitmux)
>     gst_buffer_replace (&splitmux->reference_ctx->prev_in_keyframe, NULL);
>   }
> 
>+  GST_SPLITMUX_UNLOCK (splitmux);

We're taking the object lock for a very long time here, and also while we might
call request_next_keyframe() which sends an event upstream. It's usually not a
good idea to hold locks while calling to outside code, although I didn't check
whether it is actually a problem here.

I wonder if using g_atomic_int_{set|get} might be easier here, instead of the
lock (gboolean is typedefed to int).

>@@ -2298,3 +2331,13 @@ gst_splitmux_sink_ensure_max_files (GstSplitMuxSink * splitmux)
>     splitmux->fragment_id = 0;
>   }
> }
>+
>+
>+
>+static void
>+split_now (GstSplitMuxSink * splitmux, gboolean split_now)
>+{

One empty line between the functions is enough :)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.