Comment on attachment 182763 [details] [review]
Force a new cluster after each GstForceKeyUnit event

>Subject: [PATCH] matroskamux: force a new cluster after a GstForceKwyUnit event

Typo in commit message :)

>+    case GST_EVENT_CUSTOM_DOWNSTREAM:{
>+      const GstStructure *structure;
>+
>+      structure = gst_event_get_structure (event);
>+      if (!g_strcmp0 (gst_structure_get_name (structure), "GstForceKeyUnit"))
>+        gst_event_ref (event);
>+      mux->force_key_unit_event = event;
>+      break;
>+    }
>     default:
>       break;
>   }

The refcounting looks broken here, and why is event saved in mux->force_key_unit_event in any case, even for other CUSTOM_DOWNSTREAM events?

Shouldn't it be more like:

if (gst_structure_has_name (event->structure, "GstForceKeyUnit")) {
  gst_event_replace (&mux->force_key_unit_event, NULL);
  mux->force_key_unit_event = event;
  event = NULL;
}

? (with up-to-date git)


>+      /* Forward the GstForceKeyUnit event after finishing the cluster */
>+      if (mux->force_key_unit_event) {
>+        gst_pad_push_event (collect_pad->collect.pad,
>+            mux->force_key_unit_event);
>+        mux->force_key_unit_event = NULL;
>+      }

Need to make sure mux->force_key_unit_event is also freed in case we shut down early or don't get around to pushing it downstream for some other reason (e.g. in stop/reset/downward state change function or so).

Created attachment 183413 [details] [review]
0001-matroskamux-handle-GstForceKeyUnit-event-starting-a-.patch

Updated patch

Created attachment 183414 [details] [review]
0001-matroskamux-handle-GstForceKeyUnit-event-starting-a-.patch

Sorry for all the noise, don't pay attention to the last patch.

Committed with some modifications (e.g. cleanup finalize instead of adding dispose, and pushing event on srcpad rather than sinkpad).

commit 782fc78d573b54c79cc0af435560049f879a36f0
Author: Andoni Morales Alastruey <amorales@flumotion.com>
Date:   Tue Mar 15 11:03:53 2011 +0100

    matroskamux: handle GstForceKeyUnit event
    
    ... by starting a new cluster after forwarding event.
    
    Fixes #644154.