GNOME Bugzilla – Bug 763553
baseparse: Not pushing tags before PREROLL
Last modified: 2016-03-15 07:42:07 UTC
I think this may be a similar bug to https://bugzilla.gnome.org/show_bug.cgi?id=762660 but affecting other codec's. For example: aac: https://github.com/mopidy/mopidy/issues/935 mp3: https://github.com/mopidy/mopidy/issues/1474 It seems likely that <codec>_parse_pre_push_frame() may need a similar patch for all audioparsers in https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/audioparsers.
Yes: gst-launch-1.0 filesrc location=/path/to/mp3 ! id3demux ! mpegaudioparse ! fakesink silent=false -v | grep -e "tag" -e "PREROLLED" > Pipeline is PREROLLED ... > /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = event ******* (fakesink0:sink) E (type: tag (20510), GstTagList-stream, taglist=(taglist)"taglist\,\ title\=.... Without mpegaudioparse the tags are before "Pipeline is PREROLLED".
Created attachment 323780 [details] [review] baseparse: Recheck after pre_push_frame() if there are tags pending Many parsers are storing tags only in pre_push_frame(), if we wouldn't check afterwards we would push buffers before those tags and a lot of code assumes that tags are available before preroll.
Created attachment 323781 [details] [review] Revert "flacparse: push tags in pre_push_frame" This reverts commit 4065fcb80a49924b70f0c8fc159dec0ff47943a1. flacparse should not push tags by itself, the base class is going to do that while properly merging in upstream tags. It just didn't because of a bug in the base class, which was hidden by this commit.
(In reply to Sebastian Dröge (slomo) from comment #2) > Created attachment 323780 [details] [review] [review] > baseparse: Recheck after pre_push_frame() if there are tags pending > > Many parsers are storing tags only in pre_push_frame(), if we wouldn't check > afterwards we would push buffers before those tags and a lot of code assumes > that tags are available before preroll. The main problem with this is now that we might push taglists twice. Once a non-complete one before pre_push_frame(), then a complete one after pre_push_frame(). I don't see a way to solve this as pre_push_frame() on its own might already push a buffer via gst_pad_push() and we have no way to intercept that.
Maybe we need a way to 'queue' frames to push out in baseparse. That way you could queue header buffers to be pushed, and when you reach then end of the flac headers they can all be pushed in one go (after any events; and the tag events could be de-duplicated).
> Maybe we need a way to 'queue' frames to push out in baseparse. Actually we already seem to have that via GST_BASE_PARSE_FRAME_FLAG_QUEUE.
While not ideal, I guess multiple pushes are preferable to no pushes. Downstream apps can cope if they are aware of this possibility. For example mopidy (if I'm reading its code correctly) already accumulates pushed taglists: https://github.com/mopidy/mopidy/blob/af43612630892fc3cc8be9b0f13109b1a89b1198/mopidy/audio/scan.py#L212-L218 But if GST_BASE_PARSE_FRAME_FLAG_QUEUE can provide a neater solution then so much the better.
GST_BASE_PARSE_FRAME_FLAG_QUEUE does not help here, at least I don't see how they could be used in h264parse/h265parse (only parsers I see currently that directly push buffers, and vc1parse but ...).
In general, I think these patches make things better and should go in. We can fix parsers that are pushing buffers themselves by giving them a way to do it via baseparse as a further improvement later
Let's go with that then for now. commit 66e9e4c20291b91038a4dc2a112ca04fd6533192 Author: Sebastian Dröge <sebastian@centricular.com> Date: Sun Mar 13 10:33:13 2016 +0200 Revert "flacparse: push tags in pre_push_frame" This reverts commit 4065fcb80a49924b70f0c8fc159dec0ff47943a1. flacparse should not push tags by itself, the base class is going to do that while properly merging in upstream tags. It just didn't because of a bug in the base class, which was hidden by this commit. https://bugzilla.gnome.org/show_bug.cgi?id=763553 commit 87c0513569b92a34ca325ff1af61f9cbe3b29886 Author: Sebastian Dröge <sebastian@centricular.com> Date: Sun Mar 13 10:33:53 2016 +0200 baseparse: Recheck after pre_push_frame() if there are tags pending Many parsers are storing tags only in pre_push_frame(), if we wouldn't check afterwards we would push buffers before those tags and a lot of code assumes that tags are available before preroll. https://bugzilla.gnome.org/show_bug.cgi?id=763553
Thanks all. Should the same also be applied to the 1.6 branch?
Done :) Thanks for reporting & testing