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).

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.

That would seem like a regression then.

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.

Tim, any progress on this?

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

No, I have files without picture frame where this happens.

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.

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.

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 :)

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

Seems to work correctly for me (but should be pushed after 1.7.90 I think)

Review of attachment 322783 [details] [review]:

Will wait until 1.7.90 is out

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 :)

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

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)

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.

Pushed to 1.6

https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?h=1.6&id=2da8df0895dba8f84e5caac52c15e65fde4ac4cf

Will also land in 1.6.4 then if/when it gets released.

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?

Because the base class API only stores the taglist and mode, and does not merge it with the previous one. Why?!

See https://bugzilla.gnome.org/show_bug.cgi?id=763553#c3 which reverts this commit and fixes it properly (?) in baseparse.

> 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.)

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

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 on attachment 322783 [details] [review]
patch to fix the issue

This commit was reverted now as part of bug #763553