After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 641047 - [mpegaudioparse] Multiple issues with new mpegaudioparse element from -bad, lower rank?
[mpegaudioparse] Multiple issues with new mpegaudioparse element from -bad, l...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal blocker
: 0.10.22
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 643972 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-01-31 16:45 UTC by Hans de Goede
Modified: 2011-03-27 15:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
baseparse: tune QUERY_SEEKING response (1.48 KB, patch)
2011-01-31 17:49 UTC, Mark Nauwelaerts
none Details | Review
tagdemux: also push cached events when operating in pull mode (3.61 KB, patch)
2011-02-02 15:53 UTC, Mark Nauwelaerts
committed Details | Review
baseparse: tune QUERY_SEEKING response (bis) (1.29 KB, patch)
2011-02-07 14:02 UTC, Mark Nauwelaerts
committed Details | Review

Description Hans de Goede 2011-01-31 16:45:19 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
Comment 1 Mark Nauwelaerts 2011-01-31 17:40:23 UTC
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).
Comment 2 Mark Nauwelaerts 2011-01-31 17:49:27 UTC
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).
Comment 3 Hans de Goede 2011-01-31 18:18:06 UTC
(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.
Comment 4 Mark Nauwelaerts 2011-02-01 16:38:15 UTC
(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 ...).
Comment 5 Tim-Philipp Müller 2011-02-01 17:12:00 UTC
> 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.
Comment 6 Mark Nauwelaerts 2011-02-01 18:58:20 UTC
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 ...
Comment 7 Mark Nauwelaerts 2011-02-02 15:53:42 UTC
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).
Comment 8 Hans de Goede 2011-02-02 18:19:10 UTC
Thanks for the quick fixes!
Comment 9 Tim-Philipp Müller 2011-02-03 18:33:27 UTC
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..
Comment 10 Tim-Philipp Müller 2011-02-03 18:42:47 UTC
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.
Comment 11 Mark Nauwelaerts 2011-02-07 13:43:15 UTC
(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).
Comment 12 Mark Nauwelaerts 2011-02-07 14:02:13 UTC
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).
Comment 13 Tim-Philipp Müller 2011-02-07 15:51:39 UTC
> 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.
Comment 14 Mark Nauwelaerts 2011-02-07 16:02:58 UTC
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).
Comment 15 Mark Nauwelaerts 2011-02-07 16:55:01 UTC
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.
Comment 16 Tim-Philipp Müller 2011-02-13 17:57:22 UTC
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?).
Comment 17 Mark Nauwelaerts 2011-02-13 20:18:21 UTC
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 ?).
Comment 18 Mark Nauwelaerts 2011-02-14 19:10:25 UTC
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)
Comment 19 Mark Nauwelaerts 2011-02-17 13:08:36 UTC
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.
Comment 20 Tim-Philipp Müller 2011-03-27 15:05:58 UTC
*** Bug 643972 has been marked as a duplicate of this bug. ***