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 696549 - wavparse: add 'note' chunk support
wavparse: add 'note' chunk 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: 2013-03-25 11:42 UTC by Anton Belka
Modified: 2013-04-09 20:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] wavparse: add 'note' chunk support (3.72 KB, patch)
2013-03-25 11:42 UTC, Anton Belka
none Details | Review
[PATCH] wavparse: add 'note' chunk support (3.80 KB, patch)
2013-03-27 08:04 UTC, Anton Belka
reviewed Details | Review
[PATCH] wavparse: add 'note' chunk support (4.13 KB, patch)
2013-04-06 09:16 UTC, Anton Belka
reviewed Details | Review
[PATCH] wavparse: add 'note' chunk support (4.52 KB, patch)
2013-04-09 20:44 UTC, Anton Belka
committed Details | Review

Description Anton Belka 2013-03-25 11:42:35 UTC
Created attachment 239751 [details] [review]
[PATCH] wavparse: add 'note' chunk support

I'm added 'note' chunk support in TOC as GST_TAG_COMMENT. Not sure about commit message, please fix it. Also If you have any comments about patch, please leave.
Comment 1 Anton Belka 2013-03-27 07:53:58 UTC
This patch have mistake: if wav don't contain 'note' chunk, still appears comment. Trying fix this...
Comment 2 Anton Belka 2013-03-27 08:04:09 UTC
Created attachment 239928 [details] [review]
[PATCH] wavparse: add 'note' chunk support

Forgot break in switch, fixed. :) Now everything works nicely.
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2013-04-05 11:35:12 UTC
Review of attachment 239928 [details] [review]:

Looks good to me.

::: gst/wavparse/gstwavparse.c
@@ +1252,3 @@
+  note->cue_point_id = GST_READ_UINT32_LE (data);
+  note->text = (gchar *) g_new0 (gchar *, size - 4 + 1);
+{

you can use g_memdup() here to allocate and dup in one go.
Comment 4 Anton Belka 2013-04-06 09:16:33 UTC
Created attachment 240825 [details] [review]
[PATCH] wavparse: add 'note' chunk support

Used g_memdup() both in labl and in note parser.
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2013-04-09 19:29:46 UTC
Just read on the NOTE chunk again. This patch would concat all the notes into a single COMMENT chunk, not sure that would make sense.

For other reviewers:
http://www.sonicspot.com/guide/wavefiles.html#note
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2013-04-09 20:01:29 UTC
Review of attachment 240825 [details] [review]:

::: gst/wavparse/gstwavparse.c
@@ +1383,3 @@
+      }
+      gst_tag_list_add (tags, GST_TAG_MERGE_PREPEND, GST_TAG_COMMENT,
+    cur_subentry = gst_toc_find_entry (toc, id);

This looks like duplicated code. Could you extract this into a little helper and use this here two times instead?
Comment 7 Anton Belka 2013-04-09 20:44:51 UTC
Created attachment 241089 [details] [review]
[PATCH] wavparse: add 'note' chunk support

Extracted duplicate code as function.
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2013-04-09 20:57:13 UTC
Thanks!