GNOME Bugzilla – Bug 677306
[wavparse] TOC support
Last modified: 2013-03-23 14:12:46 UTC
Needed add TOC support in riff-wave format. I will parse cue chunks in the wavparse plugin. So, I work on it. :)
Created attachment 216572 [details] [review] [PATCH] wavparse: add TOC support This patch add TOC support in wavparse. Also need add tags in TOC from adtl list and I will do it. I hope, that my patch will be commited if all ok. If not, please write what I must fix. Thanks.
Review of attachment 216572 [details] [review]: * In the for loop for parsing all the cue entries, just increment the data pointer instead of alwayas doing the i*sizeof(bla) calculation * g_free() the result of g_strdup_printf() * Use const guint8* for the data pointer of the cue parsing function * You can use the chunk start, block start, sample offset fields to seek more efficiently to the TOC entries * You need to check that the data chunk is the one given in the cues * Didn't you want to add labl/etc parsing too? In that case you should parse everything into some private data structure and build the TOC before parsing of the data chunk starts. You would then connect the information of the cue chunk(s) and the labl/etc chunks there to build the TOC * Put the struct not into the header but into the .c file * + if (!wav->toc) + wav->toc = toc; You should probably merge the cues. A new cue chunk would be a new edition inside the same TOC. Also the way you do it now you're leaking the newly created TOC
Created attachment 216812 [details] [review] [PATCH] wavparse: add TOC support
Review of attachment 216812 [details] [review]: ::: gst/wavparse/gstwavparse.c @@ +1148,3 @@ + guint32 block_start; + guint32 sample_offset; + } GstWavParseCue; Don't put it in the function scope, some compilers don't like this @@ +1163,3 @@ + + ncues = GST_READ_UINT32_LE (data); + if (size != 4 + ncues * sizeof (GstWavParseCue)) { No sizeof here but 24, see below for reasoning @@ +1165,3 @@ + if (size != 4 + ncues * sizeof (GstWavParseCue)) { + GST_WARNING_OBJECT (wav, "broken file"); + return FALSE; It's not an error so just return TRUE here, right? @@ +1172,3 @@ + /* parse data */ + for (i = 0; i < ncues; i++) { + cue[i].id = GST_READ_UINT32_LE (data + 4); Not + 4 here from what I understand. Do + 4 outside the loop to skip over the ncues though, and adjust all the offsets inside the loop accordingly @@ +1178,3 @@ + cue[i].block_start = GST_READ_UINT32_LE (data + 20); + cue[i].sample_offset = GST_READ_UINT32_LE (data + 24); + data += sizeof (GstWavParseCue); No, += 24 please. Due to struct padding and other things sizeof(GstWavParseCue) might have a different value @@ +1185,3 @@ + + /* add edition */ + if (!g_list_length (toc->entries)) { As you just created the toc some lines above the list length will always be 0 :) @@ +1203,3 @@ + stop = + gst_util_uint64_scale_round (GST_SECOND, cue[i + 1].position, + wav->rate); Reorder the arguments maybe. first position, then GST_SECOND, then wav->rate. More intuitive @@ +2680,3 @@ + toc = wav->toc; + else + toc = gst_toc_new (); If there's no TOC, better just set res=FALSE and don't set an empty TOC
Created attachment 216853 [details] [review] [PATCH] wavparse: add TOC support Reworked. About: @@ +2680,3 @@ + toc = wav->toc; + else + toc = gst_toc_new (); If there's no TOC, better just set res=FALSE and don't set an empty TOC case GST_QUERY_TOC was copied from matroska-demux. Fixed as you said, hope this better.
Review of attachment 216853 [details] [review]: ::: gst/wavparse/gstwavparse.c @@ +1159,3 @@ + if (wav->toc) { + GST_WARNING_OBJECT (wav, "found another cue chunk"); + return FALSE; Still returning FALSE here @@ +1167,3 @@ + return TRUE; + } + cue = g_new (GstWavParseCue, ncues); You have to free this later @@ +2683,3 @@ + res = TRUE; + if (!wav->toc) + gst_toc_free (toc); The freeing here is wrong now. Remove the two lines. Other than that it's better. Want to provide the same change for matroskademux too? :)
Created attachment 216857 [details] [review] [PATCH] wavparse: add TOC support Fixed.
(In reply to comment #4) > Review of attachment 216812 [details] [review]: > > > @@ +1165,3 @@ > + if (size != 4 + ncues * sizeof (GstWavParseCue)) { > + GST_WARNING_OBJECT (wav, "broken file"); > + return FALSE; > > It's not an error so just return TRUE here, right? why is that not an error? I mean it is not fatal as the cue part is optional.
(In reply to comment #8) > (In reply to comment #4) > > Review of attachment 216812 [details] [review] [details]: > > > > > > > @@ +1165,3 @@ > > + if (size != 4 + ncues * sizeof (GstWavParseCue)) { > > + GST_WARNING_OBJECT (wav, "broken file"); > > + return FALSE; > > > > It's not an error so just return TRUE here, right? > > why is that not an error? I mean it is not fatal as the cue part is optional. Actually you're right, this is a non-fatal error but an error nonetheless
Created attachment 216905 [details] [review] [PATCH] wavparse: add TOC support Small fix. I hope now everything is really good. :)
Review of attachment 216905 [details] [review]: ::: gst/wavparse/gstwavparse.c @@ +1165,3 @@ + if (size != 4 + ncues * 24) { + GST_WARNING_OBJECT (wav, "broken file"); + return TRUE; return FALSE here please, it actually is an error.
Created attachment 217214 [details] [review] [PATCH] wavparse: add TOC support Now only support parsing cue chunks.
Review of attachment 217214 [details] [review]: ::: gst/wavparse/gstwavparse.c @@ +1157,3 @@ + GstTocEntry *entry = NULL, *subentry = NULL; + + if (wav->toc) { Protect this with the GstObject lock, see comment below for more info @@ +1162,3 @@ + } + + ncues = GST_READ_UINT32_LE (data); Also check before this that at least 4 bytes are available @@ +1211,3 @@ + + /* send data as TOC */ + wav->toc = toc; Protect access to wav->toc with the GstObject lock (GST_OBJECT_LOCK / _UNLOCK). Don't hold this lock while calling any GStreamer functions like gst_pad_push_event(), gst_element_post_message(), etc. but instead get a new reference of the TOC from inside the locked section and release the new reference after being done. @@ +2683,3 @@ + res = FALSE; + } + gst_query_set_toc (query, toc, 0); Protect with the GstObject lock too here @@ +2717,3 @@ + gint64 start_pos; + + if (!wavparse->toc) { And here, but don't call gst_wavparse_perform_seek() while holding the lock
Created attachment 219236 [details] [review] [PATCH] wavparse: add TOC support After many changes... Thanks to Sebastian! :)
Review of attachment 219236 [details] [review]: ::: gst/wavparse/gstwavparse.c @@ +235,3 @@ + wav->toc = NULL; + if (wav->cues) + g_list_free (wav->cues); Doesn't this leak GstWavParseCue entries? @@ +238,3 @@ + wav->cues = NULL; + if (wav->labls) + g_list_free (wav->labls); Doesn't this leak GstWavParseLabl enttries?
Created attachment 219272 [details] [review] [PATCH] wavparse: add TOC support Fixed memory leaks.
commit cc6d533521841bc949136e969d96230ad7a30bd2 Author: Anton Belka <antonbelka@gmail.com> Date: Sun Jul 8 20:36:22 2012 +0300 wavparse: Add TOC support Add support for: * Cue Chunk * Associated Data List Chunk * Label Chunk https://bugzilla.gnome.org/show_bug.cgi?id=677306
Many thanks! :)
I find the way we present cuesheets as TOC a bit weird: $ gst-launch-1.0 playbin uri=file:///tracks.wav --toc FOUND TOC edition: start: 0:00:00.000000000 stop: 0:49:22.760000000 chapter: start: 0:00:00.440000000 stop: 0:06:57.733333333 chapter: start: 0:06:57.733333333 stop: 0:11:06.933333333 chapter: start: 0:11:06.933333333 stop: 0:19:54.706666667 chapter: start: 0:19:54.706666667 stop: 0:26:15.973333333 chapter: start: 0:26:15.973333333 stop: 0:33:10.640000000 chapter: start: 0:33:10.640000000 stop: 0:40:13.200000000 chapter: start: 0:40:13.200000000 stop: 0:49:22.7600 a) why 'chapter' ? Any reason not to switch it to 'track' ? (I presume this code predates my overhaul of the API and the addition of more entry types?) b) do we really need/want a single top-level 'edition' here? Why? If yes, then we should probably add a new 'cue sheet' type.
(In reply to comment #19) > I find the way we present cuesheets as TOC a bit weird: > > $ gst-launch-1.0 playbin uri=file:///tracks.wav --toc > FOUND TOC > edition: start: 0:00:00.000000000 stop: 0:49:22.760000000 > chapter: start: 0:00:00.440000000 stop: 0:06:57.733333333 > chapter: start: 0:06:57.733333333 stop: 0:11:06.933333333 > chapter: start: 0:11:06.933333333 stop: 0:19:54.706666667 > chapter: start: 0:19:54.706666667 stop: 0:26:15.973333333 > chapter: start: 0:26:15.973333333 stop: 0:33:10.640000000 > chapter: start: 0:33:10.640000000 stop: 0:40:13.200000000 > chapter: start: 0:40:13.200000000 stop: 0:49:22.7600 > > > a) why 'chapter' ? Any reason not to switch it to 'track' ? (I presume this > code predates my overhaul of the API and the addition of more entry types?) > > b) do we really need/want a single top-level 'edition' here? Why? If yes, then > we should probably add a new 'cue sheet' type. 1. Yes, code written when TOC api has only two entries: chapter and edition. Now we really must change chapter to track. 2. We really need a single top-level 'edition' here, because riff-wav can store TOC in both: Cue Chunk and Sampler Chunk.