[Bug 540891] flacparse: handle toc-select event

[GStreamer (bugzilla.gnome.org)]

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

Tim-Philipp Müller <t.i.m> changed:

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

--- Comment #36 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2013-03-18 20:58:28 UTC ---
(From update of attachment 239192)
Thanks for working on this! Could you post your test program as well (however
messy it might be)?

>@@ -1694,7 +1697,6 @@ gst_flac_parse_pre_push_frame (GstBaseParse * parse, GstBaseParseFrame * frame)
>   if (flacparse->toc) {
>     gst_pad_push_event (GST_BASE_PARSE_SRC_PAD (flacparse),
>         gst_event_new_toc (flacparse->toc, FALSE));
>-    flacparse->toc = NULL;
>   }

I first thought this was wrong, assuming that gst_event_new_toc() takes
ownership of the TOC, but turns out that is not the case, so it's fine, and the
original code was leaking.


>+static gboolean
>+gst_flac_parse_src_event (GstBaseParse * parse, GstEvent * event)
>+{
>+  GstFlacParse *flacparse = GST_FLAC_PARSE (parse);
>+  gboolean res = FALSE;
>+
>+  switch (GST_EVENT_TYPE (event)) {
>+    case GST_EVENT_TOC_SELECT:
>+    {
>+      char *uid = NULL;
>+      GstTocEntry *entry = NULL;
>+      GstEvent *seek_event;
>+      gint64 start_pos;
>+
>+      if (!flacparse->toc) {
>+        GST_DEBUG_OBJECT (flacparse, "no TOC to select");
>+        return FALSE;

Two points here:

1) best not to directly return from a handler like this, it usually means one
forgets to clean up properly (e.g. here the event needs to be freed)

2)  I think we need some sort of locking for ->toc, even if it's probably a bit
of a stretch in practice. So maybe something like:

  GstToc *toc = NULL;

  GST_PAD_STREAM_LOCK (GST_BASE_PARSE_SINK_PAD(flacparse));
  if (flacparse->toc)
    toc = gst_toc_ref (flacparse->toc);
  GST_PAD_STREAM_UNLOCK (GST_BASE_PARSE_SINK_PAD(flacparse));
  ....
  if (toc == NULL) {
    ...
  } else {
    ...
  }
  gst_toc_unref (toc);


>+      } else {
>+        gst_event_parse_toc_select (event, &uid);
>+        if (uid != NULL) {
>+          GST_OBJECT_LOCK (flacparse);
>+          entry = gst_toc_find_entry (flacparse->toc, uid);
>+          if (entry == NULL) {
>+            GST_OBJECT_UNLOCK (flacparse);

No more locking needed here with the above locking, since the toc is immutable
and you have acquired your own ref.

>+            GST_WARNING_OBJECT (flacparse, "no TOC entry with given UID: %s",
>+                uid);
>+            res = FALSE;
>+          } else {
>+            gst_toc_entry_get_start_stop_times (entry, &start_pos, NULL);
>+            GST_OBJECT_UNLOCK (flacparse);
>+            seek_event = gst_event_new_seek (1.0,
>+                GST_FORMAT_TIME,
>+                GST_SEEK_FLAG_FLUSH,
>+                GST_SEEK_TYPE_SET, start_pos, GST_SEEK_TYPE_SET, -1);

last two should be "GST_SEEK_TYPE_NONE, -1" I think ?

Looks good otherwise.

-- 
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.