Created attachment 221215 [details] [review]
[PATCH] wavenc: add tags support

Fucking indent deleted blank lines. :/ Fixed.

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.

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

(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?

(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

Anton, I can try to pick this up. Could you attach an up-to-date version?

Created attachment 245457 [details] [review]
[PATCH] wavenc: add tags support

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

Created attachment 245593 [details] [review]
wavenc: add tags & toc support

It does now work for me. Will test for a few more days ...

Stefan, thanks a lot for finish and commit my patch!