[Bug 677306] [wavparse] TOC support

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #216812|none                        |needs-work
             status|                            |

--- Comment #4 from Sebastian Dröge <slomo at circular-chaos.org> 2012-06-20 15:35:32 UTC ---
Review of attachment 216812:
 --> (https://bugzilla.gnome.org/review?bug=677306&attachment=216812)

::: gst/wavparse/gstwavparse.c
@@ +1148,3 @@
+    guint32 block_start;
+    guint32 sample_offset;
+  } GstWavParseCue;

Don't put it in the function scope, some compilers don't like this

@@ +1163,3 @@
+
+  ncues = GST_READ_UINT32_LE (data);
+  if (size != 4 + ncues * sizeof (GstWavParseCue)) {

No sizeof here but 24, see below for reasoning

@@ +1165,3 @@
+  if (size != 4 + ncues * sizeof (GstWavParseCue)) {
+    GST_WARNING_OBJECT (wav, "broken file");
+    return FALSE;

It's not an error so just return TRUE here, right?

@@ +1172,3 @@
+  /* parse data */
+  for (i = 0; i < ncues; i++) {
+    cue[i].id = GST_READ_UINT32_LE (data + 4);

Not + 4 here from what I understand. Do + 4 outside the loop to skip over the
ncues though, and adjust all the offsets inside the loop accordingly

@@ +1178,3 @@
+    cue[i].block_start = GST_READ_UINT32_LE (data + 20);
+    cue[i].sample_offset = GST_READ_UINT32_LE (data + 24);
+    data += sizeof (GstWavParseCue);

No, += 24 please. Due to struct padding and other things sizeof(GstWavParseCue)
might have a different value

@@ +1185,3 @@
+
+  /* add edition */
+  if (!g_list_length (toc->entries)) {

As you just created the toc some lines above the list length will always be 0
:)

@@ +1203,3 @@
+      stop =
+          gst_util_uint64_scale_round (GST_SECOND, cue[i + 1].position,
+          wav->rate);

Reorder the arguments maybe. first position, then GST_SECOND, then wav->rate.
More intuitive

@@ +2680,3 @@
+        toc = wav->toc;
+      else
+        toc = gst_toc_new ();

If there's no TOC, better just set res=FALSE and don't set an empty TOC

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