GNOME Bugzilla – Bug 540891
flacparse: handle toc-select event
Last modified: 2013-03-21 07:06:09 UTC
We should support embedded cuesheets in flac files. They're usually used to separate different tracks in a single flac file when encoding a complete CD into one file.
Yes, CUE files should be supported. They can actually contain various different codecs, but I guess MP3 and FLAC are the most common for audio only .cue sheets. It can be considered as a playlist file, so it might not be clear what's GStreamer's role when parsing/playing such files. http://en.wikipedia.org/wiki/Cue_sheet_(computing)
*** Bug 613319 has been marked as a duplicate of this bug. ***
Note that the METADATA_CUESHEET block seems to not actually be used that much for embedding cuesheets, because it can only store the track offsets and not any CDTEXT etc. that cue sheets also often contain. Instead the data is stored as a cuesheet= Vorbis comment, which can be accessed in raw form just by searching through the GST_TAG_EXTENDED_COMMENT.
*** Bug 679490 has been marked as a duplicate of this bug. ***
Created attachment 218171 [details] [review] flacparse: add TOC support Using tracks instead edition with chapters as previous. Thanks to Tim.
Review of attachment 218171 [details] [review]: ::: gst/audioparsers/gstflacparse.c @@ +1000,3 @@ + tags = gst_tag_list_new_empty (); + gst_tag_list_add (tags, GST_TAG_MERGE_APPEND, GST_TAG_EXTENDED_COMMENT, + catalog_number, NULL); This is not how it's supposed to be used. The EXTENDED_COMMENT tag should be a key=value list, see http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstTagList.html#GST-TAG-EXTENDED-COMMENT:CAPS Also note that you should have a 129 byte character array, where the last one is a \0 @@ +1021,3 @@ + if (!gst_byte_reader_skip (&reader, 12)) + goto error; + memcpy (isrc, map.data + gst_byte_reader_get_pos (&reader), 12); Note that you should have a 13 byte character array, where the last one is a \0 @@ +1635,3 @@ + gst_pad_push_event (GST_BASE_PARSE_SRC_PAD (flacparse), + gst_event_new_toc (flacparse->toc, FALSE)); + gst_element_post_message (GST_ELEMENT_CAST (flacparse), Only push the event, the message posting is handled by the sinks nowadays
Created attachment 218561 [details] [review] [PATCH] flacparse: add TOC support Skip parsing catalog number. Other things fixed. Sebastian, thanks for you review.
Comment on attachment 218561 [details] [review] [PATCH] flacparse: add TOC support commit b01cf1561c14dace6e6988668ceecda82a1aa7b2 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Tue Jul 17 10:01:54 2012 +0200 flacparse: Fix parsing of ISRC from the cuesheets commit ffc204e6bd734d12c4407f3700192a7831d34791 Author: Anton Belka <antonbelka@gmail.com> Date: Thu Jul 5 14:15:25 2012 +0300 flacparse: add TOC support Add support embedded cuesheets in flac files. Parsing METADATA_BLOCK_CUESHEET as TOC. https://bugzilla.gnome.org/show_bug.cgi?id=540891
Only thing missing here now is the support for the toc-select event... which can easily be done by converting it to a seek event sent to the element itself.
Created attachment 219775 [details] [review] [PATCH] flacenc: add TOC support
Review of attachment 219775 [details] [review]: ::: ext/flac/gstflacenc.c @@ +510,3 @@ + return FALSE; + + for (i = 0; i < n_tracks; i++) { Iterating over GList should be done like for (l = list; l; l = l->next) { GstTocEntry *entry = l->data; [...] } @@ +512,3 @@ + for (i = 0; i < n_tracks; i++) { + entry = list->data; + gst_toc_entry_get_start_stop_times (entry, &start, &stop); You need to check that the TOC event is of a format you can actually support, i.e. just a flat TOC with a number of title/chapter entries. Everything else can't be handled by cuesheets.
Or a number of "track" entries (e.g. when ripping a CD in continuous mode where all songs are just one long stream).
Created attachment 219883 [details] [review] [PATCH] flacenc: add TOC support I hope that I understand your comment about the title/track/chapter. Now I check if entry type sequence.
Review of attachment 219883 [details] [review]: ::: ext/flac/gstflacenc.c @@ +515,3 @@ + entry = list->data; + if (!gst_toc_entry_is_sequence (entry)) + return FALSE; You might've added cuesheets to the encoder already for the first few elements and then return FALSE here. You must first check if the TOC is valid in all elements, and only afterwards add the cuesheet tracks.
Created attachment 220020 [details] [review] [PATCH] flacenc: add TOC support Fixed check the TOC before adding tracks.
Review of attachment 220020 [details] [review]: ::: ext/flac/gstflacenc.c @@ +449,3 @@ + if (flacenc->meta[3]) + FLAC__metadata_object_delete (flacenc->meta[3]); In the place where flacenc->meta is allocated you need to allocate one more element now @@ +597,3 @@ gst_tag_list_foreach (copy, add_one_tag, flacenc); + toc = gst_toc_setter_get_toc (GST_TOC_SETTER (flacenc)); You need to unref this toc later when you don't use it anymore
(In reply to comment #16) > Review of attachment 220020 [details] [review]: > > ::: ext/flac/gstflacenc.c > @@ +449,3 @@ > > + if (flacenc->meta[3]) > + FLAC__metadata_object_delete (flacenc->meta[3]); > > In the place where flacenc->meta is allocated you need to allocate one more > element now I already did this. See this lines in patch: @@ -502,12 +590,28 @@ gst_flac_enc_set_metadata (GstFlacEnc * flacenc, guint64 total_samples) n_preview_images = gst_tag_list_get_tag_size (copy, GST_TAG_PREVIEW_IMAGE); flacenc->meta = - g_new0 (FLAC__StreamMetadata *, 3 + n_images + n_preview_images); + g_new0 (FLAC__StreamMetadata *, 4 + n_images + n_preview_images); > @@ +597,3 @@ > gst_tag_list_foreach (copy, add_one_tag, flacenc); > > + toc = gst_toc_setter_get_toc (GST_TOC_SETTER (flacenc)); > > You need to unref this toc later when you don't use it anymore Yes, my fail.
Created attachment 220085 [details] [review] [PATCH] flacenc: add TOC support Fixed memory leak.
Created attachment 220087 [details] [review] [PATCH] flacenc: add TOC support Improved patch over 9000%.
Review of attachment 220087 [details] [review]: ::: ext/flac/gstflacenc.c @@ +692,3 @@ } + FLAC__stream_encoder_set_verify (flacenc->encoder, FALSE); Why do you set this to FALSE here? Please add a comment at least :)
(In reply to comment #20) > Review of attachment 220087 [details] [review]: > > ::: ext/flac/gstflacenc.c > @@ +692,3 @@ > } > > + FLAC__stream_encoder_set_verify (flacenc->encoder, FALSE); > > Why do you set this to FALSE here? Please add a comment at least :) This line must be deleted. Probably I spent some tests. I forgot for what this. :D
Created attachment 220230 [details] [review] [PATCH] flacenc: add TOC support Sure, that now all ok. ;)
commit fa86bf26df04e5396d6b3fe3009992024e4f2fd8 Author: Anton Belka <antonbelka@gmail.com> Date: Thu Jul 26 16:19:57 2012 +0300 flacenc: add TOC support Add TOC as embedded cuesheets in flac files. https://bugzilla.gnome.org/show_bug.cgi?id=54089 Now only the TOC_SELECT event support in flacparse is missing here.
Review of attachment 220230 [details] [review]: ::: ext/flac/gstflacenc.c @@ +511,3 @@ + + /* check if the TOC entries is valid */ + list = gst_toc_get_entries (toc); You should also allow a TOC here that has a single alternative top-level entry with multiple sequence sub-entries
(In reply to comment #24) > Review of attachment 220230 [details] [review]: > > ::: ext/flac/gstflacenc.c > @@ +511,3 @@ > + > + /* check if the TOC entries is valid */ > + list = gst_toc_get_entries (toc); > > You should also allow a TOC here that has a single alternative top-level entry > with multiple sequence sub-entries Why you think so? Flac files can store TOC only in cuesheet block and for me this mean that we mustn't create alternative top-level entry (in flacparse) and also allow it in flacenc.
That's what flacparse creates but it would make sense to allow the same here as you do in wavenc. Alternatively don't allow it in both :)
(In reply to comment #26) > That's what flacparse creates but it would make sense to allow the same here as > you do in wavenc. Alternatively don't allow it in both :) Yes, I did this in wavenc, but wavparse support alternative entries. For wavparse/wavenc this make sense, because riff-wav have cue chunk and smpl chunk which we must present as alternative entries. In flac format we have only one cuesheet block and don't have alternative for it. In my mind right TOC in flac mustn't have alternative entries and this reason why we mustn't allow it in flacparse/flacenc. If I'm wrong, please say why. I also wait opinion other GStreamer developers, such as Stefan and Tim. :)
How would you differentiate between the cue and smpl GstTocEntries in wavenc? I guess this would make sense as long as we document that there must not be a top-level alternative entry if it's only a single one that contains a sequence.
(In reply to comment #28) > How would you differentiate between the cue and smpl GstTocEntries in wavenc? By uid entries. :) In wavparse for cue chunk I create edition entry with uid "cue". For smpl chunk uid will be "smpl". > I guess this would make sense as long as we document that there must not be a > top-level alternative entry if it's only a single one that contains a sequence. Ok, now whether to allow alternative top-level entry in flacenc or not? :)
(In reply to comment #29) > (In reply to comment #28) > > How would you differentiate between the cue and smpl GstTocEntries in wavenc? > By uid entries. :) In wavparse for cue chunk I create edition entry with uid > "cue". For smpl chunk uid will be "smpl". Wrong assumption. The uid is nothing that should be interpreted, it's just a random string that uniquely identifies the entry. > > I guess this would make sense as long as we document that there must not be a > > top-level alternative entry if it's only a single one that contains a sequence. > Ok, now whether to allow alternative top-level entry in flacenc or not? :) IMHO it would make sense. In that case you could have ... ! wavparse ! flacenc ! ... working
Created attachment 220807 [details] [review] [PATCH] flacenc: allow a TOC with single alternative top-level entry Tested with ! wavparse ! flacenc ! ... I'm not sure, but maybe we can to improve patch. If you have idea, please write how.
Looks good to me :) Now only thing missing for flac is the TOC_SELECT event handling in flacparse (by simply issuing a seek on the element from the event handler). commit 59186f970d322d3094796ca5d337f3a129e4d23f Author: Anton Belka <antonbelka@gmail.com> Date: Thu Aug 9 19:41:34 2012 +0300 flacenc: allow a TOC with single alternative top-level entry Allow a TOC that has a single alternative top-level entry with multiple sequence sub-entries https://bugzilla.gnome.org/show_bug.cgi?id=540891
(In reply to comment #32) > Looks good to me :) Now only thing missing for flac is the TOC_SELECT event > handling in flacparse (by simply issuing a seek on the element from the event > handler). I know about this. :)
Retitling - is toc-select handling all that's left to do?
Created attachment 239192 [details] [review] [PATCH] flacparse: add handle toc-select event
Comment on attachment 239192 [details] [review] [PATCH] flacparse: add handle toc-select event Thanks for working on this! Could you post your test program as well (however messy it might be)? >@@ -1694,7 +1697,6 @@ gst_flac_parse_pre_push_frame (GstBaseParse * parse, GstBaseParseFrame * frame) > if (flacparse->toc) { > gst_pad_push_event (GST_BASE_PARSE_SRC_PAD (flacparse), > gst_event_new_toc (flacparse->toc, FALSE)); >- flacparse->toc = NULL; > } I first thought this was wrong, assuming that gst_event_new_toc() takes ownership of the TOC, but turns out that is not the case, so it's fine, and the original code was leaking. >+static gboolean >+gst_flac_parse_src_event (GstBaseParse * parse, GstEvent * event) >+{ >+ GstFlacParse *flacparse = GST_FLAC_PARSE (parse); >+ gboolean res = FALSE; >+ >+ switch (GST_EVENT_TYPE (event)) { >+ case GST_EVENT_TOC_SELECT: >+ { >+ char *uid = NULL; >+ GstTocEntry *entry = NULL; >+ GstEvent *seek_event; >+ gint64 start_pos; >+ >+ if (!flacparse->toc) { >+ GST_DEBUG_OBJECT (flacparse, "no TOC to select"); >+ return FALSE; Two points here: 1) best not to directly return from a handler like this, it usually means one forgets to clean up properly (e.g. here the event needs to be freed) 2) I think we need some sort of locking for ->toc, even if it's probably a bit of a stretch in practice. So maybe something like: GstToc *toc = NULL; GST_PAD_STREAM_LOCK (GST_BASE_PARSE_SINK_PAD(flacparse)); if (flacparse->toc) toc = gst_toc_ref (flacparse->toc); GST_PAD_STREAM_UNLOCK (GST_BASE_PARSE_SINK_PAD(flacparse)); .... if (toc == NULL) { ... } else { ... } gst_toc_unref (toc); >+ } else { >+ gst_event_parse_toc_select (event, &uid); >+ if (uid != NULL) { >+ GST_OBJECT_LOCK (flacparse); >+ entry = gst_toc_find_entry (flacparse->toc, uid); >+ if (entry == NULL) { >+ GST_OBJECT_UNLOCK (flacparse); No more locking needed here with the above locking, since the toc is immutable and you have acquired your own ref. >+ GST_WARNING_OBJECT (flacparse, "no TOC entry with given UID: %s", >+ uid); >+ res = FALSE; >+ } else { >+ gst_toc_entry_get_start_stop_times (entry, &start_pos, NULL); >+ GST_OBJECT_UNLOCK (flacparse); >+ seek_event = gst_event_new_seek (1.0, >+ GST_FORMAT_TIME, >+ GST_SEEK_FLAG_FLUSH, >+ GST_SEEK_TYPE_SET, start_pos, GST_SEEK_TYPE_SET, -1); last two should be "GST_SEEK_TYPE_NONE, -1" I think ? Looks good otherwise.
Created attachment 239228 [details] Test GST_EVENT_TOC_SELECT (In reply to comment #36) > (From update of attachment 239192 [details] [review]) > Thanks for working on this! Could you post your test program as well (however > messy it might be)? Sure. ;)
Created attachment 239354 [details] [review] [PATCH] flacparse: add handle toc-select event
Comment on attachment 239354 [details] [review] [PATCH] flacparse: add handle toc-select event Thanks! Pushed with minor changes (cosmetic mostly, plus a missing gst_toc_unref(toc))
This works really nicely now, great work!
(In reply to comment #39) > (From update of attachment 239354 [details] [review]) > Thanks! Pushed with minor changes (cosmetic mostly, plus a missing > gst_toc_unref(toc)) Yes, I forgot unref toc, my fail. Thanks for you! :)