GNOME Bugzilla – Bug 641047
[mpegaudioparse] Multiple issues with new mpegaudioparse element from -bad, lower rank?
Last modified: 2011-03-27 15:05:58 UTC
Hi, Well I guess it is still in -bad for a reason :) I hit issues with the following 2 files (likely unrelated). For this file totem fails to show the artwork embedded as an id3 tag, when the new libgstaudioparsersbad.so with the new mpegaudioparse is installed: http://ia700209.us.archive.org/0/items/freemusiccharts.songs2010/2010-08-garmisch-glimmeralbumVersion.mp3 And in this file (when accessed from a local filesystem) seeking gets disabled when using the new mpegaudioparse: http://jwrdegoede.danny.cz/test.mp3 I should probably file 2 separate bugs for tracking these 2 issues (just say the word and I will). The main issue I want to discuss in this bug is the new mpegaudioparse from libgstaudioparsersbad.so having a higher rank then the mp3parse from libgstmpegaudioparse.so. These 2 issues give me the feeling that it would be better to give the parser a lower rank for now. Regards, Hans
For the seeking case there is actually an improvement in mpegaudioparse that makes it (baseparse) wait a (slight) while to come up with a reliable duration estimate. However, it happens that Totem queries for seekability during this short interval, and baseparse reports (cautiously) "not seekable" (since not knowing a duration yet). After initial query, Totem does not query again, and so the clip is believed not seekable. As such, it depends on the precise (undefined?) semantics of QUERY_SEEKING. Should an application query multiple times, or is the first TRUE answer to hold for ever? Anyway, possible patch to follow that will have baseparse return FALSE to the query as long as it does not have a duration and there might still be a chance it can handle seeking. It is then to be hoped that "failing to handle the query" does not confuse other players/frameworks out there and is actually a valid response in such case. As the artwork, that is actually extracted by another element (id3demux), and still works fine (using gst-launch-0.10) when mpegaudioparse is used: ---- Setting pipeline to PAUSED ... Pipeline is PREROLLING ... FOUND TAG : found by element "id3demux0". title: Glimmer (Album Version) artist: Garmisch album: Glimmer date: 2010-01-01 track number: 1 genre: Electropop container format: ID3 tag duration: 1537158784000000 ID3v2 frame: buffer of 30 bytes, type: application/x-gst-id3v2-tden-frame, version=(int)4 : buffer of 30 bytes, type: application/x-gst-id3v2-tdtg-frame, version=(int)4 track count: 6 copyright: This work is licensed under a Creative Commons Attribution-Noncommercial-No Derivative Works 3.0 License image: buffer of 913258 bytes, type: image/png, image-type=(GstTagImageType)GST_TAG_IMAGE_TYPE_UNDEFINED FOUND TAG : found by element "apedemux0". replaygain track gain: 0.925000 replaygain track peak: 0.630134 replaygain album gain: -0.695000 replaygain album peak: 0.649180 container format: APE tag FOUND TAG : found by element "mpegaudioparse0". bitrate: 192000 FOUND TAG : found by element "mpegaudioparse0". audio codec: MPEG 1 Audio, Layer 3 (MP3) FOUND TAG : found by element "mpegaudioparse0". has crc: FALSE channel mode: joint-stereo FOUND TAG : found by element "mad0". layer: 3 mode: joint emphasis: none bitrate: 192000 FOUND TAG : found by element "mpegaudioparse0". minimum bitrate: 4294967295 bitrate: 192000 maximum bitrate: 0 FOUND TAG : found by element "mpegaudioparse0". minimum bitrate: 192018 maximum bitrate: 192018 Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstAudioSinkClock FOUND TAG : found by element "mpegaudioparse0". minimum bitrate: 191712 --- Tags thrown by mpegaudioparse are slightly different than those by mp3parse, but Totem should still pick up what id3demux reports (which it seems to fail with more than only album art afaics).
Created attachment 179732 [details] [review] baseparse: tune QUERY_SEEKING response As mentioned above, return FALSE to QUERY_SEEKING when not entirely sure of all our facts yet (even if that means we really can't seek yet because we do not have all the facts yet). Either tweaking baseparse this way, or have Totem check back later on (rather than relying only on first answer).
(In reply to comment #1) > For the seeking case there is actually an improvement in mpegaudioparse that > makes it (baseparse) wait a (slight) while to come up with a reliable duration > estimate. Ok, but since the file in question is a fixed bitrate file (AFAIK), and the filesize is known this should not be necessary, or am I missing something here? However, it happens that Totem queries for seekability during this > short interval, and baseparse reports (cautiously) "not seekable" (since not > knowing a duration yet). After initial query, Totem does not query again, and > so the clip is believed not seekable. > > As such, it depends on the precise (undefined?) semantics of QUERY_SEEKING. > Should an application query multiple times, or is the first TRUE answer to hold > for ever? Anyway, possible patch to follow that will have baseparse return > FALSE to the query as long as it does not have a duration and there might still > be a chance it can handle seeking. It is then to be hoped that "failing to > handle the query" does not confuse other players/frameworks out there and is > actually a valid response in such case. Ok. > > As the artwork, that is actually extracted by another element (id3demux), and > still works fine (using gst-launch-0.10) when mpegaudioparse is used: > ---- > Setting pipeline to PAUSED ... > Pipeline is PREROLLING ... > FOUND TAG : found by element "id3demux0". > title: Glimmer (Album Version) > artist: Garmisch > album: Glimmer > date: 2010-01-01 > track number: 1 > genre: Electropop > container format: ID3 tag > duration: 1537158784000000 > ID3v2 frame: buffer of 30 bytes, type: application/x-gst-id3v2-tden-frame, > version=(int)4 > : buffer of 30 bytes, type: application/x-gst-id3v2-tdtg-frame, > version=(int)4 > track count: 6 > copyright: This work is licensed under a Creative Commons > Attribution-Noncommercial-No Derivative Works 3.0 License > image: buffer of 913258 bytes, type: image/png, > image-type=(GstTagImageType)GST_TAG_IMAGE_TYPE_UNDEFINED > FOUND TAG : found by element "apedemux0". > replaygain track gain: 0.925000 > replaygain track peak: 0.630134 > replaygain album gain: -0.695000 > replaygain album peak: 0.649180 > container format: APE tag > FOUND TAG : found by element "mpegaudioparse0". > bitrate: 192000 > FOUND TAG : found by element "mpegaudioparse0". > audio codec: MPEG 1 Audio, Layer 3 (MP3) > FOUND TAG : found by element "mpegaudioparse0". > has crc: FALSE > channel mode: joint-stereo > FOUND TAG : found by element "mad0". > layer: 3 > mode: joint > emphasis: none > bitrate: 192000 > FOUND TAG : found by element "mpegaudioparse0". > minimum bitrate: 4294967295 > bitrate: 192000 > maximum bitrate: 0 > FOUND TAG : found by element "mpegaudioparse0". > minimum bitrate: 192018 > maximum bitrate: 192018 > Pipeline is PREROLLED ... > Setting pipeline to PLAYING ... > New clock: GstAudioSinkClock > FOUND TAG : found by element "mpegaudioparse0". > minimum bitrate: 191712 > --- > Tags thrown by mpegaudioparse are slightly different than those by mp3parse, > but Totem should still pick up what id3demux reports (which it seems to fail > with more than only album art afaics). I'm not a gstreamer expert, but I do know that the image tag is found by id3demux :) However mpegaudioparse is doing something which has the end result of totem no longer showing the album art. It could very well be that the cause lies outside of mpegaudioparse, but if you play the file in question with a fully up2date gstreamer stack including libgstaudioparsersbad.so it does not get shown, remove libgstaudioparsersbad.so so that mp3parse gets used instead and the problem goes away.
(In reply to comment #3) > (In reply to comment #1) > > For the seeking case there is actually an improvement in mpegaudioparse that > > makes it (baseparse) wait a (slight) while to come up with a reliable duration > > estimate. > > Ok, but since the file in question is a fixed bitrate file (AFAIK), and the > filesize is known this should not be necessary, or am I missing something here? mpegaudioparse (or anything) has no way to detect (constant) bitrate instantaneously (though we may know that it is). Neither does mp3parse, it just happens to jump quicker to a conclusion (the downside of that being that it may be less reliable in some cases, though the examination period might be tweaked slightly in mpegaudioparse also). > > However, it happens that Totem queries for seekability during this > > short interval, and baseparse reports (cautiously) "not seekable" (since not > > knowing a duration yet). After initial query, Totem does not query again, and > > so the clip is believed not seekable. > > > > As such, it depends on the precise (undefined?) semantics of QUERY_SEEKING. > > Should an application query multiple times, or is the first TRUE answer to hold > > for ever? Anyway, possible patch to follow that will have baseparse return > > FALSE to the query as long as it does not have a duration and there might still > > be a chance it can handle seeking. It is then to be hoped that "failing to > > handle the query" does not confuse other players/frameworks out there and is > > actually a valid response in such case. > > Ok. > > > > > As the artwork, that is actually extracted by another element (id3demux), and > > still works fine (using gst-launch-0.10) when mpegaudioparse is used: > > ---- > > Setting pipeline to PAUSED ... > > Pipeline is PREROLLING ... > > FOUND TAG : found by element "id3demux0". > > title: Glimmer (Album Version) > > artist: Garmisch > > album: Glimmer > > date: 2010-01-01 > > track number: 1 > > genre: Electropop > > container format: ID3 tag > > duration: 1537158784000000 > > ID3v2 frame: buffer of 30 bytes, type: application/x-gst-id3v2-tden-frame, > > version=(int)4 > > : buffer of 30 bytes, type: application/x-gst-id3v2-tdtg-frame, > > version=(int)4 > > track count: 6 > > copyright: This work is licensed under a Creative Commons > > Attribution-Noncommercial-No Derivative Works 3.0 License > > image: buffer of 913258 bytes, type: image/png, > > image-type=(GstTagImageType)GST_TAG_IMAGE_TYPE_UNDEFINED > > FOUND TAG : found by element "apedemux0". > > replaygain track gain: 0.925000 > > replaygain track peak: 0.630134 > > replaygain album gain: -0.695000 > > replaygain album peak: 0.649180 > > container format: APE tag > > FOUND TAG : found by element "mpegaudioparse0". > > bitrate: 192000 > > FOUND TAG : found by element "mpegaudioparse0". > > audio codec: MPEG 1 Audio, Layer 3 (MP3) > > FOUND TAG : found by element "mpegaudioparse0". > > has crc: FALSE > > channel mode: joint-stereo > > FOUND TAG : found by element "mad0". > > layer: 3 > > mode: joint > > emphasis: none > > bitrate: 192000 > > FOUND TAG : found by element "mpegaudioparse0". > > minimum bitrate: 4294967295 > > bitrate: 192000 > > maximum bitrate: 0 > > FOUND TAG : found by element "mpegaudioparse0". > > minimum bitrate: 192018 > > maximum bitrate: 192018 > > Pipeline is PREROLLED ... > > Setting pipeline to PLAYING ... > > New clock: GstAudioSinkClock > > FOUND TAG : found by element "mpegaudioparse0". > > minimum bitrate: 191712 > > --- > > Tags thrown by mpegaudioparse are slightly different than those by mp3parse, > > but Totem should still pick up what id3demux reports (which it seems to fail > > with more than only album art afaics). > > I'm not a gstreamer expert, but I do know that the image tag is found by > id3demux :) However mpegaudioparse is doing something which has the end result > of totem no longer showing the album art. It could very well be that the cause > lies outside of mpegaudioparse, but if you play the file in question with a > fully up2date gstreamer stack including libgstaudioparsersbad.so it does not > get > shown, remove libgstaudioparsersbad.so so that mp3parse gets used instead and > the problem goes away. Yes, it seems mpegaudioparse affects the "mood" here, but little that it can do or fix about that (afaik), so assigning to Totem to figure out what is happening with tags (and the seeking part can tag along for now ...).
> Yes, it seems mpegaudioparse affects the "mood" here, but little > that it can do or fix about that (afaik), so assigning to Totem to > figure out what is happening with tags This does look like a tag event handling issue in the parser to me. Totem doesn't pick up tags via the bus, but instead gets them from playbin2, which collects them on a per-stream basis based on the tag events that come through the different pads. Compare: gst-launch-0.10 uridecodebin uri=file:///home/tpm/samples/641047-2010-08-garmisch-glimmeralbumVersion.mp3 ! fakesink -v 2>&1 | grep taglist using mpegaudioparse and mp3parse.
Ah, frequently forget that we have tags flying all over. Looks like mpegaudioparse never receives stuff from id3demux, probably due to switching to pull mode somewhere/sometime (rather than mp3parse's basic push mode). Not clear how/where yet though ...
Created attachment 179891 [details] [review] tagdemux: also push cached events when operating in pull mode Looks like the clip in question lead to (roughly) id3demux ! apedemux ! mpegaudioparse, and apdemux/tagdemux was basically blocking the tag event sent by id3demux/tagdemux (at least in pull mode, as used by baseparse based parsers, as opposed to legacy mp3parse push mode). Bearing no further observations/comments, those patches should take care of things (assuming/hoping that other QUERY_SEEKING users adopt Totem's interpretation of its semantics).
Thanks for the quick fixes!
Comment on attachment 179891 [details] [review] tagdemux: also push cached events when operating in pull mode >@@ -1333,6 +1322,13 @@ gst_tag_demux_src_getrange (GstPad * srcpad, > { > GstTagDemux *demux = GST_TAG_DEMUX (GST_PAD_PARENT (srcpad)); > >+ /* downstream in pull mode won't miss a newsegment event, >+ * but it likely appreciates other (tag) events */ >+ if (demux->priv->need_newseg) { >+ gst_tag_demux_send_pending_events (demux); >+ demux->priv->need_newseg = FALSE; >+ } Patch looks good to me, only thing I'm wondering is whether we need to make sure here (when pushing pending events in getrange) that we're not accidentally pushing a newsegment event that may have been queued before in a different mode. I don't know if this could happen or not though..
As for the seeking/duration query stuff: My feeling is that when the pipeline is prerolled (ie. baseparse has output some buffers), it should answer both SEEKING query and duration query in some way, even if the duration isn't very good yet at that point. We have a mechanism to let the application know that the duration estimate has changed (GST_MESSAGE_DURATION) and should be re-queried. We don't have such a mechanism for seekability. However, the seeking query handler has the option of saying 'seekable from [0 ; -1]' (I think). Not answering these queries until "enough" buffers have been processed sounds like a hack to make things easier for the likes of tagreadbin and gst-discoverer to me. I think ultimately we need better API for this, so we can communicate the reliability of the duration value returned to the app.
(In reply to comment #9) > (From update of attachment 179891 [details] [review]) > >@@ -1333,6 +1322,13 @@ gst_tag_demux_src_getrange (GstPad * srcpad, > > { > > GstTagDemux *demux = GST_TAG_DEMUX (GST_PAD_PARENT (srcpad)); > > > >+ /* downstream in pull mode won't miss a newsegment event, > >+ * but it likely appreciates other (tag) events */ > >+ if (demux->priv->need_newseg) { > >+ gst_tag_demux_send_pending_events (demux); > >+ demux->priv->need_newseg = FALSE; > >+ } > > Patch looks good to me, only thing I'm wondering is whether we need to make > sure here (when pushing pending events in getrange) that we're not accidentally > pushing a newsegment event that may have been queued before in a different > mode. I don't know if this could happen or not though.. Had similar concerns. I suppose we could add an if (event == tag) or if (event != newsegment).
Created attachment 180300 [details] [review] baseparse: tune QUERY_SEEKING response (bis) Alternative approach to answering seeking query along the lines of previous comment(s). Rather than not handling it, return TRUE if thing looks good (e.g. duration will likely become available etc).
> I suppose we could add an if (event == tag) or if (event != newsegment). We need different behaviour depending on the caller, don't we? I'd blacklist newsegment events in this case, since sources may send other custom events.
Upon further checking, it seems that tagdemux' sink_event never caches newsegment events, so current patch already implicitly blacklists newsegment events (and can do so regardless of caller).
commit b2389c2108b6f643e4c0879d22059530d4d734c5 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Wed Feb 2 16:49:04 2011 +0100 tagdemux: also push cached events downstream when operating in pull mode Otherwise, having 2 tagdemux in a row followed by an element operating in pull mode will make the second tagdemux implictly eat the first tagdemux' tag event(s). Fixes (part of) #641047.
Still need to fix the query. Makes things unseekable for me with 9/10 files in totem, fwiw. Not sure which of the approaches is best (why can't it return a bad estimate, and then post DURATION messages on the bus when the estimate changes, which should make the app re-query the seekability/duration?).
I suppose we could do "anything" here (like a bad estimate at start, though preferably avoid that, since it might cause initial bad UI feedback). Regardless of best approach, I would expect (and tested) either of these patches to have Totem seeking, particularly the latter should return affirmative to seeking (so what's going wrong if Totem does not seek; is it due to a momentary unknown duration, which should then resolve as duration becomes known ?).
In follow-up, afaics from totem code, it checks seekability as follows: - perform seeking query; if result == TRUE, then the response is a permanent respone (-> so the second patch should then arrange for an affirmative answer here and as such enable seeking) - query duration; if result == TRUE and > 0 duration, then considered seekable, otherwise not (though the answer is not permanent and will be re-checked) (-> so the first patch "fails" the seeking query, but it should shortly thereafter succeed and report seekable as well as valid duration)
commit 99baf8ae17f48cb34280374bcf1baaa3e68729ee Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Mon Feb 7 14:46:57 2011 +0100 baseparse: tune QUERY_SEEKING response Even if we currently do not have a duration yet, assume seekable if it looks like we'll likely be able to determine it later on (which coincides with needed information to perform seeking). Fixes #641047.
*** Bug 643972 has been marked as a duplicate of this bug. ***