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 762660 - flacparse: Pushing buffers before tags
flacparse: Pushing buffers before tags
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.5.90
Other Linux
: Normal blocker
: 1.6.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
1.6.4
Depends on:
Blocks:
 
 
Reported: 2016-02-25 03:01 UTC by thomas_d_j
Modified: 2016-04-14 17:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to fix the issue (1.01 KB, patch)
2016-03-01 15:06 UTC, Luis de Bethencourt
rejected Details | Review

Description thomas_d_j 2016-02-25 03:01:19 UTC
After this commit:
https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=a042a9815967b0a6c0dfb65f33d78e02ee1ffb43

... mopidy no longer recognises tag data when scanning some (but not all) flac files.  It works again after https://github.com/SeeSpotRun/mopidy/commit/ec52aeb558fb50a7ed2022e73584b8bc1410ce75 but I thought I should file here in case the gstreamer dev's foresee any related problems with this.
Comment 1 Tim-Philipp Müller 2016-02-25 07:25:16 UTC
Thanks for the report.

I'm not quite sure how or why this commit would change anything for your application, or why your commit fixes it.

I see two things in that mopidy commit:

1) if pipeline.set_state(PAUSED) == ASYNC
     pipeline.set_state(PLAYING)

So are you not getting tags in PAUSED state then? I would expect tags to be extracted and push to the sink before buffers, but maybe it only posts the tag events as sink messages in playing state. But then the question is why did you get tags before? On a side note, you might just as well always set the pipeline to playing state directly here, it will always be either async or live.

2) on MSG_ASYNC_DONE:
     if message.src == pipeline and tags:
       return tags, ..

Not sure what the 'and tags' addition helps here exactly, again it sounds like you're not getting tags before ASYNC_DONE any more, but before the change you did? (On a side note, you don't need to check for message.src == pipeline for the ASYNC_DONE message).
Comment 2 Tim-Philipp Müller 2016-02-25 07:28:07 UTC
Ah, I see what's going on, it looks like it sends a small 4-byte buffer with the fLaC marker before it sends the tag event, and that buffer prerolls the pipeline. Will investigate.
Comment 3 Sebastian Dröge (slomo) 2016-02-25 09:38:39 UTC
That would seem like a regression then.
Comment 4 thomas_d_j 2016-02-25 10:51:13 UTC
Thanks, I figured it would make more sense to you guys than to me.

I can't find a pattern as to which flac files are affected and which not, although it's repeatable as to which files do this and which not.

The mopidy fix was just shotgun, hopefully I can tidy it up a bit now and flag it as a temporary workaround.
Comment 5 Sebastian Dröge (slomo) 2016-02-29 08:07:47 UTC
Tim, any progress on this?
Comment 6 Luis de Bethencourt 2016-02-29 14:57:11 UTC
Thomas,

Is it possible that the flac files to which this happens are ones which have an image inside of them?

As specified in the metadata block for pictures:
https://xiph.org/flac/format.html#metadata_block_picture
Comment 7 Tim-Philipp Müller 2016-02-29 15:05:44 UTC
No, I have files without picture frame where this happens.
Comment 8 Luis de Bethencourt 2016-02-29 16:52:42 UTC
I have created a stand-alone version of the mopidy scan code.
http://pastebin.com/fedDfBA7

After downloading you can run with:
python file.py ~/path/to/sample.flac

In my machine both, running with GStreamer from git and from the latest stable release, it captures the tags of all the FLAC files I throw at it.
Comment 9 Tim-Philipp Müller 2016-02-29 17:09:05 UTC
Irrespective of any python code, you can see the issue with:

gst-launch-1.0 filesrc location=foo.flac ! flacparse ! fakesink num-buffers=2 silent=false -v -m

optionally add  | grep -e chain -e TagList -e GstMessageTag -e GstMessageAsync

A buffer is sent before the tag event, and the pipeline prerolls (AsyncDone message on the bus) before any Tag message is posted.
Comment 10 Luis de Bethencourt 2016-02-29 17:35:25 UTC
I got confused due to the origin of the bug being reported was that mopidy was having trouble scanning some FLAC files.

I understand that independently of mopidy's handling of the Tag Message. The regression here to fix is flacparse not sending this GstMessageTag before preroll. Will continue tomorrow looking into why this happens and hopefully find a fix.

Thanks for the information Tim :)
Comment 11 Luis de Bethencourt 2016-03-01 15:06:44 UTC
Created attachment 322783 [details] [review]
patch to fix the issue

I have tested this and now we get the GstMessageTag before the AsyncDone.

Need reviewing since this is a blocker bug for 1.8
Comment 12 Tim-Philipp Müller 2016-03-01 15:19:00 UTC
Seems to work correctly for me (but should be pushed after 1.7.90 I think)
Comment 13 Luis de Bethencourt 2016-03-01 15:34:10 UTC
Review of attachment 322783 [details] [review]:

Will wait until 1.7.90 is out
Comment 14 Luis de Bethencourt 2016-03-01 19:31:10 UTC
Review of attachment 322783 [details] [review]:

With version 1.7.90 released, I am commiting the fix for this blocker bug.

Thanks for the help Tim :)
Comment 15 Luis de Bethencourt 2016-03-01 19:31:25 UTC
commit 4065fcb80a49924b70f0c8fc159dec0ff47943a1
Author: Luis de Bethencourt <luisbg@osg.samsung.com>
Date:   Tue Mar 1 15:01:22 2016 +0000

    flacparse: push tags in pre_push_frame

    Push a tag event before pre-roll if we have tags.

    https://bugzilla.gnome.org/show_bug.cgi?id=762660
Comment 16 thomas_d_j 2016-03-02 08:37:23 UTC
Thanks Tim and Luis for the prompt diagnosis and solution.

So I guess for the record:
* gst-plugins-good 1.5.90 through 1.7.90 are affected
* workaround for downstream applications relying on gstreamer to read flac tag data is to switch the track to PLAYING (at least briefly)
Comment 17 Sebastian Dröge (slomo) 2016-03-02 08:51:31 UTC
Yes, we should probably get the fix into 1.6 too so it appears in 1.6.4. This would also break tag reading via GstDiscoverer.

Btw, if you're using this only for file discovery and extracting of metadata, you might want to use GstDiscoverer instead of your custom pipeline.
Comment 19 Tim-Philipp Müller 2016-03-02 14:45:29 UTC
Will also land in 1.6.4 then if/when it gets released.
Comment 20 Sebastian Dröge (slomo) 2016-03-13 08:17:59 UTC
Review of attachment 322783 [details] [review]:

::: gst/audioparsers/gstflacparse.c
@@ +1718,3 @@
+  if (flacparse->tags) {
+    gst_pad_push_event (GST_BASE_PARSE_SRC_PAD (flacparse),
+        gst_event_new_tag (flacparse->tags));

Why is flacparse pushing tags directly and storing its own tags? Instead of using the baseparse API for exactly that, which also ensures that tags are properly merged?
Comment 21 Sebastian Dröge (slomo) 2016-03-13 08:34:33 UTC
Because the base class API only stores the taglist and mode, and does not merge it with the previous one. Why?!
Comment 22 Sebastian Dröge (slomo) 2016-03-13 08:37:46 UTC
See https://bugzilla.gnome.org/show_bug.cgi?id=763553#c3 which reverts this commit and fixes it properly (?) in baseparse.
Comment 23 Tim-Philipp Müller 2016-03-13 10:48:23 UTC
> Because the base class API only stores the taglist and mode,
> and does not merge it with the previous one. Why?!

Baseparse should merge it with any upstream one (of the same type, stream vs. global), but only when pushing it out. It needs to keep the taglist plus mode separate, so that it can re-create new updated tags when upstream tags change. (But maybe I misunderstood your question.)
Comment 24 Sebastian Dröge (slomo) 2016-03-13 11:06:31 UTC
Maybe :) My question is why gst_base_parse_merge_tags() directly stores the tags and the mode and overrides any previously stored tags. Instead of merging the new tags with the previously stored ones.

It wouldn't change anything with merging tags from upstream
Comment 25 Tim-Philipp Müller 2016-03-13 11:25:07 UTC
It seems right to me that it replaces the previously stored (by the subclass) parser tags. Only the subclass knows how to update these tags. If any merging with previously extracted tags needs to be done, the subclass needs to take care of that. The merge mode is vis-a-vis the *upstream* tags, not previously-stored parser tags.
Comment 26 Sebastian Dröge (slomo) 2016-03-14 11:16:49 UTC
Comment on attachment 322783 [details] [review]
patch to fix the issue

This commit was reverted now as part of bug #763553