[Bug 680998] [wavenc] TOC support

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #8 from Sebastian Dröge <slomo at circular-chaos.org> 2012-08-16 08:34:08 UTC ---
Review of attachment 221187:
 --> (https://bugzilla.gnome.org/review?bug=680998&attachment=221187)

::: gst/wavenc/gstwavenc.c
@@ +702,3 @@
+  /* depend from cue point id */
+  if (ncues > 4294967295)
+    return FALSE;

As you store it in a guint32 this comparison can never be TRUE. Also GList does
not allow that large lists anyway

@@ +716,3 @@
+    id = g_ascii_strtoll (uid, NULL, 0);
+    /* check if id compatible with guint32 else generate random */
+    if (id <= 4294967295) {

Probably also check if it's unique

@@ +721,3 @@
+      rand = g_rand_new ();
+      cues[i].id = g_rand_int (rand);
+      g_rand_free (rand);

Use g_random_int() instead of creating a GRand() here and destroying it again
immediately

@@ +728,3 @@
+    /* FIXME: always 0, because wavparse don't support Wave List Chunk */
+    cues[i].chunk_start = 0;
+    cues[i].block_start = 0;

And this is correct anyway according to the spec.

@@ +730,3 @@
+    cues[i].block_start = 0;
+    /* FIXME: seems to be the same value as "position" */
+    cues[i].sample_offset = cues[i].position;

No, it should be the byte offset from the start of the data chunk to the
position. So number_of_samples*sample_size*number_of_channels

@@ +762,3 @@
+    GST_WRITE_UINT32_LE (map.data + 4, labls_size);
+    memcpy (map.data + 8, (char *) "adtl", 4);
+    map.data += 12;

Don't change map.data but get a different variable that you change here.
Otherwise gst_buffer_unmap() later can explode

@@ +768,3 @@
+
+  g_free (cues);
+  g_free (labls);

You must g_free() the labls[j].text here at latest

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