GNOME Bugzilla – Bug 680998
wavenc: add TOC support
Last modified: 2013-03-24 08:48:47 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.
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.
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.
Created attachment 220490 [details] [review] [PATCH] wavenc: add TOC support Add support labels.
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
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.
> @@ +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.
Now seems, that wavenc->toc leak memory. Where better free memory? Maybe in GST_EVENT_EOS after write TOC?
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
(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.
Created attachment 221772 [details] [review] [PATCH] wavenc: add TOC support
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
Created attachment 239400 [details] [review] [PATCH] wavenc: add TOC support
(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 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); }
(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?
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.
Created attachment 239479 [details] [review] [PATCH] wavenc: add TOC support
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.
Thanks, Tim! :)