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 681891 - wavenc: support LIST INFO chunk
wavenc: support LIST INFO chunk
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-15 01:44 UTC by Anton Belka
Modified: 2013-06-01 23:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] wavenc: add tags support (7.81 KB, patch)
2012-08-15 01:44 UTC, Anton Belka
none Details | Review
[PATCH] wavenc: add tags support (5.40 KB, patch)
2012-08-15 01:51 UTC, Anton Belka
needs-work Details | Review
[PATCH] wavenc: add tags support (6.25 KB, patch)
2013-05-28 15:30 UTC, Anton Belka
none Details | Review
wavenc: add tags & toc support (12.90 KB, patch)
2013-05-29 20:47 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review

Description Anton Belka 2012-08-15 01:44:02 UTC
Created attachment 221214 [details] [review]
[PATCH] wavenc: add tags support

This patch for support tags as LIST INFO chunk in wav format. Idea and some code coped from avimux. I'm not sure about:
1. GST_EVENT_TAG
2. GST_RIFF_INFO_ICRD == GST_TAG_DATE (maybe here need GST_TAG_DATETIME)
3. Memory leaks. (Where I must free wavenc->tags?)
Waiting comments... Thanks.
Comment 1 Anton Belka 2012-08-15 01:51:12 UTC
Created attachment 221215 [details] [review]
[PATCH] wavenc: add tags support

Fucking indent deleted blank lines. :/ Fixed.
Comment 2 Sebastian Dröge (slomo) 2012-08-16 08:21:34 UTC
Review of attachment 221215 [details] [review]:

::: gst/wavenc/gstwavenc.c
@@ +631,3 @@
+        if (gst_tag_list_get_date (tags, tag, &date)) {
+          str =
+              g_strdup_printf ("%04d:%02d:%02d", g_date_get_year (date),

With : as separator? Are you sure?

@@ +640,3 @@
+      if (str) {
+        gst_byte_writer_put_uint32_le (bw, rifftags[n].fcc);
+        gst_byte_writer_put_uint32_le (bw, GST_ROUND_UP_2 (strlen (str)));

You should store the strlen() in a variable and use that here, GST_ROUND_UP_2() and gst_byte_writer_put_uint32_le() are macros that evaluate their parameters multiple times, i.e. you will have multiple strlen() calls

Also add a comment that rounding up to a multiple of two is safe here because either we write until the last character of the string or the \0 terminator at the end.

@@ +745,3 @@
+      gst_event_parse_tag (event, &tags);
+      if (tags) {
+        if (wavenc->tags != tags) {

This is not ideal, you need to differentiate between stream-scope and global-scope taglists here.
Comment 3 Anton Belka 2012-08-16 09:42:26 UTC
(In reply to comment #2)
> Review of attachment 221215 [details] [review]:
> 
> ::: gst/wavenc/gstwavenc.c
> @@ +631,3 @@
> +        if (gst_tag_list_get_date (tags, tag, &date)) {
> +          str =
> +              g_strdup_printf ("%04d:%02d:%02d", g_date_get_year (date),
> 
> With : as separator? Are you sure?

This coped from avimux. I will recheck this.
Comment 4 Anton Belka 2012-08-27 16:19:47 UTC
(In reply to comment #2)
> @@ +745,3 @@
> +      gst_event_parse_tag (event, &tags);
> +      if (tags) {
> +        if (wavenc->tags != tags) {
> 
> This is not ideal, you need to differentiate between stream-scope and
> global-scope taglists here.

How in general GST_EVENT_TAG must look?
Comment 5 Sebastian Dröge (slomo) 2012-10-14 08:51:53 UTC
(In reply to comment #4)
> (In reply to comment #2)
> > @@ +745,3 @@
> > +      gst_event_parse_tag (event, &tags);
> > +      if (tags) {
> > +        if (wavenc->tags != tags) {
> > 
> > This is not ideal, you need to differentiate between stream-scope and
> > global-scope taglists here.
> 
> How in general GST_EVENT_TAG must look?

It must be global scope here
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2013-04-18 18:03:02 UTC
Anton, I can try to pick this up. Could you attach an up-to-date version?
Comment 7 Anton Belka 2013-05-28 15:30:46 UTC
Created attachment 245457 [details] [review]
[PATCH] wavenc: add tags support
Comment 8 Sebastian Dröge (slomo) 2013-05-29 07:14:29 UTC
Stefan mentioned on IRC that WAV of course only has a single stream, and as such merging the global and stream tags is fine. Might make sense to have some kind of order for that though, e.g. let all stream tags replace tags of the same name from the global tags (prefer stream tags over global tags).
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2013-05-29 20:47:04 UTC
Created attachment 245593 [details] [review]
wavenc: add tags & toc support

It does now work for me. Will test for a few more days ...
Comment 10 Anton Belka 2013-06-01 23:19:11 UTC
Stefan, thanks a lot for finish and commit my patch!