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 680998 - wavenc: add TOC support
wavenc: add TOC support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-08-01 16:26 UTC by Anton Belka
Modified: 2013-03-24 08:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] wavenc: add TOC support (5.84 KB, patch)
2012-08-01 16:26 UTC, Anton Belka
reviewed Details | Review
[PATCH] wavenc: add TOC support (7.19 KB, patch)
2012-08-06 19:24 UTC, Anton Belka
needs-work Details | Review
[PATCH] wavenc: add TOC support (7.71 KB, patch)
2012-08-14 19:41 UTC, Anton Belka
needs-work Details | Review
[PATCH] wavenc: add TOC support (8.12 KB, patch)
2012-08-19 23:19 UTC, Anton Belka
needs-work Details | Review
[PATCH] wavenc: add TOC support (8.09 KB, patch)
2013-03-20 18:46 UTC, Anton Belka
reviewed Details | Review
[PATCH] wavenc: add TOC support (8.21 KB, patch)
2013-03-21 16:38 UTC, Anton Belka
committed Details | Review

Description Anton Belka 2012-08-01 16:26:34 UTC
Created attachment 220079 [details] [review]
[PATCH] wavenc: add TOC support

wavparse support TOC and wavenc must too. :) This first patch, which currently don't support labl's. Also Block Start, Sample Offset shouldn’t be 0 (I'm not understand how calculate this values). Just say what I need to fix. Thanks.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2012-08-02 11:29:58 UTC
Review of attachment 220079 [details] [review]:

::: gst/wavenc/gstwavenc.c
@@ +654,3 @@
+  list = gst_toc_entry_get_sub_entries (entry);
+  ncues = g_list_length (list);
+

just use g_new() if you set *all* fields

@@ +657,3 @@
+
+  memcpy (cue_chunk.chunk_id, "cue ", 4);
+  toc = wavenc->toc;

if possible please use sizeof(xx) instead of 24

@@ +664,3 @@
+    subentry = list->data;
+    if (!gst_toc_entry_is_sequence (subentry))
+  if (!gst_toc_entry_is_alternative (entry))

this leaks the memory pointer to by 'cues'

@@ +677,3 @@
+        gst_util_uint64_scale_round (start, wavenc->rate, GST_SECOND);
+    memcpy (cue_chunk.cue_points[i].data_chunk_id, "data", 4);
+

I think this is fine for now.
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2012-08-02 11:46:03 UTC
Review of attachment 220079 [details] [review]:

::: gst/wavenc/gstwavenc.c
@@ +678,3 @@
+    memcpy (cue_chunk.cue_points[i].data_chunk_id, "data", 4);
+    /* FIXME: always 0, because wavparse don't support Wave List Chunk */
+  list = gst_toc_entry_get_sub_entries (entry);

This I believe we can leave as 0.

@@ +679,3 @@
+    /* FIXME: always 0, because wavparse don't support Wave List Chunk */
+    cue_chunk.cue_points[i].chunk_start = 0;
+  list = gst_toc_entry_get_sub_entries (entry);

For PCM formats it should be 0, for compressed formats it would be the "File position of the enclosing block relative to the start of the 'data' chunk. The software can begin the decompression at this point." As we only support alaw/mulaw which use 1 byte per sample you should be able to calculate it.

@@ +680,3 @@
+    cue_chunk.cue_points[i].chunk_start = 0;
+    cue_chunk.cue_points[i].block_start = 0;
+  ncues = g_list_length (list);

This seems to be the same value as "position". Maybe you can generate a few files and verify.
Comment 3 Anton Belka 2012-08-06 19:24:21 UTC
Created attachment 220490 [details] [review]
[PATCH] wavenc: add TOC support

Add support labels.
Comment 4 Sebastian Dröge (slomo) 2012-08-07 16:16:06 UTC
Review of attachment 220490 [details] [review]:

::: gst/wavenc/gstwavenc.c
@@ +677,3 @@
+  entry = list->data;
+
+  if (!gst_toc_entry_is_alternative (entry))

You should also allow a TOC with only a list of sequence-entries without a top-level alternative.

@@ +681,3 @@
+
+  uid = gst_toc_entry_get_uid (entry);
+  if (g_strcmp0 ("cue", uid))

Don't do string comparisons with the uids. They're just meant to identify entries, nothing else. What do you want to do here?

@@ +707,3 @@
+      gst_tag_list_get_string (tags, GST_TAG_TITLE, &title);
+    uid = gst_toc_entry_get_uid (subentry);
+    cues[i].id = (guint32) g_ascii_strtoll (uid, NULL, 0);

Don't do that. Check if the uid is a 32 bit integer and otherwise create random numbers.

The uids can be of any format

@@ +714,3 @@
+    cues[i].chunk_start = 0;
+    cues[i].block_start = 0;
+    cues[i].sample_offset = 0;

This doesn't look correct, you should be able to calculate it

@@ +715,3 @@
+    cues[i].block_start = 0;
+    cues[i].sample_offset = 0;
+    if (title) {

You need to g_free() title when you're done with it
Comment 5 Anton Belka 2012-08-14 19:41:01 UTC
Created attachment 221187 [details] [review]
[PATCH] wavenc: add TOC support

I tested look at different wav files: alaw/mulaw/pcm with mono/stereo and see that Chunk Start = 0, Block Start = 0, Sample Offset = Position. All test files created with GoldWave (windows) software.
Comment 6 Anton Belka 2012-08-14 19:47:49 UTC
> @@ +715,3 @@
> +    cues[i].block_start = 0;
> +    cues[i].sample_offset = 0;
> +    if (title) {
> 
> You need to g_free() title when you're done with it

I mustn't do it, because labls[j].tex it's pointer.
Comment 7 Anton Belka 2012-08-14 23:06:07 UTC
Now seems, that wavenc->toc leak memory. Where better free memory? Maybe in  GST_EVENT_EOS after write TOC?
Comment 8 Sebastian Dröge (slomo) 2012-08-16 08:34:08 UTC
Review of attachment 221187 [details] [review]:

::: 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
Comment 9 Sebastian Dröge (slomo) 2012-08-16 08:34:30 UTC
(In reply to comment #7)
> Now seems, that wavenc->toc leak memory. Where better free memory? Maybe in 
> GST_EVENT_EOS after write TOC?

In gst_wavenc_change_state() when going from PAUSED to READY.
Comment 10 Anton Belka 2012-08-19 23:19:03 UTC
Created attachment 221772 [details] [review]
[PATCH] wavenc: add TOC support
Comment 11 Sebastian Dröge (slomo) 2012-10-14 08:51:06 UTC
Review of attachment 221772 [details] [review]:

Looks almost ready except

::: gst/wavenc/gstwavenc.c
@@ +714,3 @@
+  ncues = g_list_length (list);
+  cues = g_new (struct cue_point, ncues);
+  cues_size += (ncues * sizeof (struct cue_point));

This is not safe, sizeof(struct cue_point) can be different from what you think because of struct padding. Calculate the sizeof manually
Comment 12 Anton Belka 2013-03-20 18:46:25 UTC
Created attachment 239400 [details] [review]
[PATCH] wavenc: add TOC support
Comment 13 Anton Belka 2013-03-21 08:05:26 UTC
(In reply to comment #12)
> Created an attachment (id=239400) [details] [review]
> [PATCH] wavenc: add TOC support

Not sure that right write toc when GST_EVENT_EOS get. Can anybody comment this?
Comment 14 Tim-Philipp Müller 2013-03-21 09:32:38 UTC
Comment on attachment 239400 [details] [review]
[PATCH] wavenc: add TOC support

>+static void
>+gst_wavenc_write_cues (guint8 ** data, struct cue_point *cues, guint32 ncues)
>+{
>+  guint32 i;
>+
>+  for (i = 0; i < ncues; i++) {
>+    GST_WRITE_UINT32_LE (*data, cues[i].id);
>+    GST_WRITE_UINT32_LE (*data + 4, cues[i].position);
>+    memcpy (*data + 8, (char *) cues[i].data_chunk_id, 4);
>+    GST_WRITE_UINT32_LE (*data + 12, cues[i].chunk_start);
>+    GST_WRITE_UINT32_LE (*data + 16, cues[i].block_start);
>+    GST_WRITE_UINT32_LE (*data + 20, cues[i].sample_offset);
>+    *data += 24;
>+  }
>+}

I wonder if GstByteWriter wouldn't be more convenient here (and in the other functions)..

>+    uid = gst_toc_entry_get_uid (entry);
>+    id = g_ascii_strtoll (uid, NULL, 0);
>+    /* check if id unique compatible with guint32 else generate random */
>+    if (id <= 4294967295 && gst_wavenc_check_cue_id (cues, i, id)) {
>+      cues[i].id = (guint32) id;
>+    } else {
>+      cues[i].id = g_random_int ();
>+    }

id is a signed int64 here, so might want to check for >=0 as well or use _stroull instead. Are these CUE IDs here supposed to have any semantics (are they supposed to be like track/chapter numbers or just random identifiers?) Even if you make a random one, you probably still want to make sure it doesn't exist yet..


>     case GST_EVENT_EOS:{
>       GST_DEBUG_OBJECT (wavenc, "got EOS");
>+      if (wavenc->toc) {
>+        gst_wavenc_write_toc (wavenc);
>+      }

Is the TOC supposed to be at the and of the WAV file?


>+    case GST_EVENT_TOC:
>+      gst_event_parse_toc (event, &toc, NULL);
>+      if (toc) {
>+        if (wavenc->toc != toc) {
>+          if (wavenc->toc)
>+            gst_toc_unref (wavenc->toc);
>+          wavenc->toc = toc;
>+        }

  else {
    gst_toc_unref (toc);
  }
Comment 15 Anton Belka 2013-03-21 15:29:52 UTC
(In reply to comment #14)
> (From update of attachment 239400 [details] [review])
> >+static void
> >+gst_wavenc_write_cues (guint8 ** data, struct cue_point *cues, guint32 ncues)
> >+{
> >+  guint32 i;
> >+
> >+  for (i = 0; i < ncues; i++) {
> >+    GST_WRITE_UINT32_LE (*data, cues[i].id);
> >+    GST_WRITE_UINT32_LE (*data + 4, cues[i].position);
> >+    memcpy (*data + 8, (char *) cues[i].data_chunk_id, 4);
> >+    GST_WRITE_UINT32_LE (*data + 12, cues[i].chunk_start);
> >+    GST_WRITE_UINT32_LE (*data + 16, cues[i].block_start);
> >+    GST_WRITE_UINT32_LE (*data + 20, cues[i].sample_offset);
> >+    *data += 24;
> >+  }
> >+}
> 
> I wonder if GstByteWriter wouldn't be more convenient here (and in the other
> functions)..
> 

I use GST_WRITE_UINT32_LE there, because it's used in other places at wavenc (to keep the overall coding style).

> >+    uid = gst_toc_entry_get_uid (entry);
> >+    id = g_ascii_strtoll (uid, NULL, 0);
> >+    /* check if id unique compatible with guint32 else generate random */
> >+    if (id <= 4294967295 && gst_wavenc_check_cue_id (cues, i, id)) {
> >+      cues[i].id = (guint32) id;
> >+    } else {
> >+      cues[i].id = g_random_int ();
> >+    }
> 
> id is a signed int64 here, so might want to check for >=0 as well or use
> _stroull instead. Are these CUE IDs here supposed to have any semantics (are
> they supposed to be like track/chapter numbers or just random identifiers?)
> Even if you make a random one, you probably still want to make sure it doesn't
> exist yet..
> 

Oh, here really better use _stroll and check if >=0, because look better. :) CUE IDs don't have any semantic. From specification:
Each cue point has a unique identification value used to associate cue points with information in other chunks. For example, a Label chunk contains text that describes a point in the wave file by referencing the associated cue point.

> 
> >     case GST_EVENT_EOS:{
> >       GST_DEBUG_OBJECT (wavenc, "got EOS");
> >+      if (wavenc->toc) {
> >+        gst_wavenc_write_toc (wavenc);
> >+      }
> 
> Is the TOC supposed to be at the and of the WAV file?
>

Yes, CUEs stored at the end of the WAV file.

> 
> >+    case GST_EVENT_TOC:
> >+      gst_event_parse_toc (event, &toc, NULL);
> >+      if (toc) {
> >+        if (wavenc->toc != toc) {
> >+          if (wavenc->toc)
> >+            gst_toc_unref (wavenc->toc);
> >+          wavenc->toc = toc;
> >+        }
> 
>   else {
>     gst_toc_unref (toc);
>   }

Why we need unref TOC here if it will be NULL?
Comment 16 Tim-Philipp Müller 2013-03-21 15:37:35 UTC
You need to unref if wavenc->toc == toc.

If CUEs go at the end of a WAV file, they should be written on EVENT_EOS, yes.
Comment 17 Anton Belka 2013-03-21 16:38:04 UTC
Created attachment 239479 [details] [review]
[PATCH] wavenc: add TOC support
Comment 18 Tim-Philipp Müller 2013-03-23 13:01:40 UTC
Pushed, thanks:

 commit e80817348323646a8d159f2e1c19d32fc7db0fca
 Author: Anton Belka <antonbelka@gmail.com>
 Date:   Wed Mar 20 21:38:40 2013 +0300

    wavenc: add TOC support
    
    https://bugzilla.gnome.org/show_bug.cgi?id=680998

I made one more minor change - I moved the gst_buffer_unmap() before the gst_pad_push().

Seems to work nicely, but see my comments about the TOC table extracted by wavparse in bug #677306.
Comment 19 Anton Belka 2013-03-24 08:48:47 UTC
Thanks, Tim! :)