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 644154 - [matroskamux] Force a new cluster after each GstForceKeyUnit event
[matroskamux] Force a new cluster after each GstForceKeyUnit event
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 607742
Blocks:
 
 
Reported: 2011-03-07 20:40 UTC by Andoni Morales
Modified: 2011-09-07 12:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Force a new cluster after each GstForceKeyUnit event (2.52 KB, patch)
2011-03-07 20:40 UTC, Andoni Morales
needs-work Details | Review
0001-matroskamux-handle-GstForceKeyUnit-event-starting-a-.patch (3.83 KB, patch)
2011-03-15 10:01 UTC, Andoni Morales
none Details | Review
0001-matroskamux-handle-GstForceKeyUnit-event-starting-a-.patch (3.83 KB, patch)
2011-03-15 10:08 UTC, Andoni Morales
none Details | Review

Description Andoni Morales 2011-03-07 20:40:16 UTC
Created attachment 182763 [details] [review]
Force a new cluster after each GstForceKeyUnit event

This patch makes the muxer to force a new cluster after each GstForceKeyUnit event
Comment 1 Tim-Philipp Müller 2011-03-14 23:40:54 UTC
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).
Comment 2 Andoni Morales 2011-03-15 10:01:24 UTC
Created attachment 183413 [details] [review]
0001-matroskamux-handle-GstForceKeyUnit-event-starting-a-.patch

Updated patch
Comment 3 Andoni Morales 2011-03-15 10:08:24 UTC
Created attachment 183414 [details] [review]
0001-matroskamux-handle-GstForceKeyUnit-event-starting-a-.patch
Comment 4 Andoni Morales 2011-03-15 10:10:11 UTC
Sorry for all the noise, don't pay attention to the last patch.
Comment 5 Mark Nauwelaerts 2011-09-07 12:54:44 UTC
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.