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 679768 - mpegaudioparse, baseparse: fix tag handling
mpegaudioparse, baseparse: fix tag handling
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other All
: Normal blocker
: 1.5.90
Assigned To: Tim-Philipp Müller
GStreamer Maintainers
: 711710 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-07-11 21:38 UTC by Matej Knopp
Modified: 2015-08-18 11:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (1.54 KB, patch)
2012-07-11 21:38 UTC, Matej Knopp
none Details | Review
Updated patch (1.53 KB, patch)
2012-11-18 18:51 UTC, Matej Knopp
needs-work Details | Review
baseparse: Remove duplicate code (1.12 KB, patch)
2013-11-29 22:31 UTC, Olivier Crête
committed Details | Review
baseparse: Add utility function to push queued events (3.69 KB, patch)
2013-11-29 22:31 UTC, Olivier Crête
rejected Details | Review
baseparse: Add Tag handling (7.16 KB, patch)
2013-11-29 22:31 UTC, Olivier Crête
needs-work Details | Review
mpegaudioparse: Use baseparse tag merging API (2.42 KB, patch)
2013-11-29 22:32 UTC, Olivier Crête
accepted-commit_now Details | Review
flacparse: Use baseparse tag merging API (3.78 KB, patch)
2013-11-29 22:32 UTC, Olivier Crête
accepted-commit_now Details | Review
baseparse: Add Tag handling (7.13 KB, patch)
2015-03-17 20:45 UTC, Olivier Crête
none Details | Review
mpegaudioparse: Use baseparse tag merging API (2.20 KB, patch)
2015-03-17 21:28 UTC, Olivier Crête
none Details | Review
flacparse: Use baseparse tag merging API (3.74 KB, patch)
2015-03-17 21:28 UTC, Olivier Crête
none Details | Review
audioparsers: Use gst_base_parse_merge_tag() API (4.43 KB, patch)
2015-03-17 21:52 UTC, Olivier Crête
committed Details | Review
videoparsers: Use gst_base_parse_merge_tags() (5.89 KB, patch)
2015-03-17 21:59 UTC, Olivier Crête
committed Details | Review
baseparse: Add Tag handling (7.51 KB, patch)
2015-03-17 22:58 UTC, Olivier Crête
none Details | Review
audiodecoder: fix tag handling (7.88 KB, patch)
2015-08-14 17:01 UTC, Tim-Philipp Müller
committed Details | Review
tests: audiodecoder: add unit test for tag handling (11.03 KB, patch)
2015-08-14 17:02 UTC, Tim-Philipp Müller
committed Details | Review
baseparse: save upstream stream tags (3.35 KB, patch)
2015-08-15 17:45 UTC, Tim-Philipp Müller
none Details | Review
WIP: baseparse: fix tag handling, step 1 (11.79 KB, patch)
2015-08-15 17:46 UTC, Tim-Philipp Müller
none Details | Review
baseparse: save upstream stream tags (3.54 KB, patch)
2015-08-16 13:34 UTC, Tim-Philipp Müller
committed Details | Review
baseparse: add API for subclass to set tags (4.64 KB, patch)
2015-08-16 13:36 UTC, Tim-Philipp Müller
committed Details | Review
baseparse: fix tag handling (12.10 KB, patch)
2015-08-16 13:36 UTC, Tim-Philipp Müller
committed Details | Review
flacparse: use new baseparse API and fix tag handling (2.58 KB, patch)
2015-08-16 13:39 UTC, Tim-Philipp Müller
committed Details | Review

Description Matej Knopp 2012-07-11 21:38:18 UTC
Created attachment 218589 [details] [review]
Patch

currently the parser immediately replaces the tag event containing nominal bitrate with another event.
Comment 1 Matej Knopp 2012-11-18 18:51:46 UTC
Created attachment 229300 [details] [review]
Updated patch
Comment 2 Tim-Philipp Müller 2012-11-18 20:22:52 UTC
I think this patch makes sense, but to me it looks like the tag handling overall is just as broken after this patch as it is before this patch.

For example, let's look at this:

gst-launch-1.0 filesrc location= /home/tpm/music/foo.mp3 ! id3demux ! mpegaudioparse ! fakesink  silent=false -v | grep event

GstEventStreamStart, stream-id=(string)992d662179435060860978a505a8f93abd08979f1b4728d45d4501c596db18f3;) 0x22f0860
GstEventCaps, caps=(GstCaps)audio/mpeg, mpegversion=(int)1, mpegaudioversion=(int)1, layer=(int)3, rate=(int)44100, channels=(int)2, parsed=(boolean)true;) 0x22f08c0
GstEventSegment, segment=(GstSegment)"GstSegment, flags=(GstSegmentFlags)GST_SEGMENT_FLAG_NONE, rate=(double)1, applied-rate=(double)1, format=(GstFormat)GST_FORMAT_TIME, base=(guint64)0, start=(guint64)0, stop=(guint64)18446744073709551615, time=(guint64)0, position=(guint64)0, duration=(guint64)18446744073709551615;";) 0x21649e0

GstTagList-stream, taglist=(taglist)"taglist\,\ bitrate\=\(uint\)171000\;";) 0x22f0980
GstTagList-stream, taglist=(taglist)"taglist\,\ audio-codec\=\(string\)\"MPEG\\\ 1\\\ Audio\\\,\\\ Layer\\\ 3\\\ \\\(MP3\\\)\"\;";) 0x22f09e0
GstTagList-stream, taglist=(taglist)"taglist\,\ has-crc\=\(boolean\)false\,\ channel-mode\=\(string\)joint-stereo\;";) 0x22f0a40
GstTagList-stream, taglist=(taglist)"taglist\,\ title\=\(string\)\"Title\"\,\ artist\=\(string\)\"Artist\"\,\ album\=\(string\)Album\,\ datetime\=\(datetime\)2010\,\ comment\=\(string\)-----\,\ track-number\=\(uint\)1\,\ genre\=\(string\)Indie\,\ container-format\=\(string\)\"ID3\\\ tag\"\,\ encoded-by\=\(string\)\"LAME\\\ Ain\\\'t\\\ an\\\ MP3\\\ Encoder\"\,\ private-id3v2-frame\=\(buffer\)544c414e00000008000000456e676c697368\;";) 0x22f0aa0
GstTagList-stream, taglist=(taglist)"taglist\,\ minimum-bitrate\=\(uint\)159862\,\ bitrate\=\(uint\)171000\,\ maximum-bitrate\=\(uint\)159862\;"
GstTagList-stream, taglist=(taglist)"taglist\,\ maximum-bitrate\=\(uint\)191712\;
GstTagList-stream, taglist=(taglist)"taglist\,\ maximum-bitrate\=\(uint\)223868\;
GstTagList-stream, taglist=(taglist)"taglist\,\ maximum-bitrate\=\(uint\)255718\;
GstTagList-stream, taglist=(taglist)"taglist\,\ maximum-bitrate\=\(uint\)319725\;
GstTagList-stream, taglist=(taglist)"taglist\,\ minimum-bitrate\=\(uint\)111781\;
GstTagList-stream, taglist=(taglist)"taglist\,\ minimum-bitrate\=\(uint\)95856\;"
GstTagList-stream, taglist=(taglist)"taglist\,\ minimum-bitrate\=\(uint\)79931\;"
GstTagList-stream, taglist=(taglist)"taglist\,\ minimum-bitrate\=\(uint\)63700\;"
GstTagList-stream, taglist=(taglist)"taglist\,\ minimum-bitrate\=\(uint\)31850\;"
eos

Since tag events are sticky now (stream tags and global tags separately), we need to confer the entire tag 'state' in tag lists, I think. We can't just send random tag lists and then expect downstream to merge them all.

In other words, whenever we want to send an update, we should take the stream tags we received from upstream, make a copy to make them writable, and then add *all* of our own tags (audio-codec, bitrates, crc, whatnot) to that, and then send the whole thing, so downstream has an accurate picture of the entire state.

Perhaps baseparse needs some API addition to make this easier (it could keep track of the subclass's additions and take care of the update + merging things with the upstream tags).
Comment 3 Sebastian Dröge (slomo) 2013-07-25 07:21:42 UTC
The video and audio decoder/encoder base classes have some API for this already. We might want to replicate something like that for all the other base classes and other cases.
Comment 4 Brendan Long 2013-11-09 18:03:20 UTC
You can have multiple sticky events of each type, so it seems like we should be able to handle this by just not removing tags when we get new ones (although we might need a way to say "discard the previous tags").
Comment 5 Brendan Long 2013-11-09 18:03:54 UTC
*** Bug 711710 has been marked as a duplicate of this bug. ***
Comment 6 Sebastian Dröge (slomo) 2013-11-11 14:11:56 UTC
(In reply to comment #4)
> You can have multiple sticky events of each type, so it seems like we should be
> able to handle this by just not removing tags when we get new ones (although we
> might need a way to say "discard the previous tags").

Yes, tags should be merged. See the video/audio decoder base classes :) They should probably be dropped whenever a stream-start event is received (or the element is shut down obviously).
Comment 7 Olivier Crête 2013-11-29 22:31:29 UTC
Created attachment 263179 [details] [review]
baseparse: Remove duplicate code

These are already free by gst_base_parse_clear_queues
Comment 8 Olivier Crête 2013-11-29 22:31:34 UTC
Created attachment 263180 [details] [review]
baseparse: Add utility function to push queued events

This code was present three times
Comment 9 Olivier Crête 2013-11-29 22:31:37 UTC
Created attachment 263181 [details] [review]
baseparse: Add Tag handling

Add API to make it easy for subclasses to add tags to the tag event

API: gst_base_parse_merge_tags
Comment 10 Olivier Crête 2013-11-29 22:32:02 UTC
Created attachment 263182 [details] [review]
mpegaudioparse: Use baseparse tag merging API

This way, existing tags are not overwritten, but correctly merged.
Comment 11 Olivier Crête 2013-11-29 22:32:05 UTC
Created attachment 263183 [details] [review]
flacparse: Use baseparse tag merging API

This way, existing tags are not overwritten, but correctly merged.
Comment 12 Sebastian Dröge (slomo) 2014-04-02 21:24:35 UTC
Comment on attachment 263180 [details] [review]
baseparse: Add utility function to push queued events

Why is the explicit sorting of events necessary here? If they are out of order here, some other code is probably wrong already and sorting here only hides that.
Comment 13 Sebastian Dröge (slomo) 2014-04-02 21:30:50 UTC
Review of attachment 263181 [details] [review]:

::: libs/gst/base/gstbaseparse.c
@@ +2259,3 @@
 
+  /* Push pending events, including SEGMENT events */
+  gst_base_parse_push_events (parse);

Why do you move this code?

@@ +4375,3 @@
   /* We only care about stream tags here */
   if (gst_tag_list_get_scope (taglist) != GST_TAG_SCOPE_STREAM)
+    return FALSE;

Do we? Shouldn't global scope tags just be forwarded?

@@ +4531,3 @@
+ * MT safe.
+ *
+ * Since 1.4

"Since: 1.4" with a colon

@@ +4544,3 @@
+  GST_OBJECT_LOCK (parse);
+  if (tags)
+    GST_DEBUG_OBJECT (parse, "merging tags %" GST_PTR_FORMAT, tags);

What's the point of allowing tags==NULL here?

::: win32/common/libgstbase.def
@@ +28,3 @@
 	gst_base_parse_frame_new
 	gst_base_parse_get_type
+	gst_base_parse_merge_tags

Also need to add that to the documentation
Comment 14 Sebastian Dröge (slomo) 2014-07-18 12:13:59 UTC
Olivier?
Comment 15 Tim-Philipp Müller 2015-03-17 20:25:42 UTC
Comment on attachment 263181 [details] [review]
baseparse: Add Tag handling

I think the way these tags are handled here is all wrong (I realise it's modelled on the audio/video decoder code, but that also doesn't seem right to me).

As I see it, we should maintain a copy of the upstream tags (latest version), and then the parser tags plus merge mode separately. The parser tags replace any previous parser tags if updated.

Whenever upstream tags change or the parser tags are set, we do tags_changed=TRUE (which should be renamed to update_tags or resend_tags or something).

Just merging continuously into previous merge results seems a bit weird to me.

>+void
>+gst_base_parse_merge_tags (GstBaseParse * parse, const GstTagList * tags,
>+    GstTagMergeMode mode)

I think something like gst_base_parse_set_parser_tags() would be clearer, but don't mind merge_tags() for consistency if others prefer that.
Comment 16 Olivier Crête 2015-03-17 20:32:33 UTC
Comment on attachment 263180 [details] [review]
baseparse: Add utility function to push queued events

Thiago wrote basically the same patch already as accaadf5 for bug #721350
Comment 17 Olivier Crête 2015-03-17 20:44:31 UTC
Review of attachment 263181 [details] [review]:

Step 1, updating the patch for Sebastian's comments.

::: libs/gst/base/gstbaseparse.c
@@ +2259,3 @@
 
+  /* Push pending events, including SEGMENT events */
+  gst_base_parse_push_events (parse);

It has to be after the call to gst_base_parse_update_bitrates()

@@ +4375,3 @@
   /* We only care about stream tags here */
   if (gst_tag_list_get_scope (taglist) != GST_TAG_SCOPE_STREAM)
+    return FALSE;

That's what returning false does.

@@ +4544,3 @@
+  GST_OBJECT_LOCK (parse);
+  if (tags)
+    GST_DEBUG_OBJECT (parse, "merging tags %" GST_PTR_FORMAT, tags);

No idea, it's what the encoder/decoder base classes do.
Comment 18 Olivier Crête 2015-03-17 20:45:12 UTC
Created attachment 299643 [details] [review]
baseparse: Add Tag handling

Add API to make it easy for subclasses to add tags to the tag event

API: gst_base_parse_merge_tags
Comment 19 Olivier Crête 2015-03-17 21:28:30 UTC
Created attachment 299651 [details] [review]
mpegaudioparse: Use baseparse tag merging API

This way, existing tags are not overwritten, but correctly merged.
Comment 20 Olivier Crête 2015-03-17 21:28:40 UTC
Created attachment 299652 [details] [review]
flacparse: Use baseparse tag merging API

This way, existing tags are not overwritten, but correctly merged.
Comment 21 Olivier Crête 2015-03-17 21:52:02 UTC
Created attachment 299654 [details] [review]
audioparsers: Use gst_base_parse_merge_tag() API
Comment 22 Olivier Crête 2015-03-17 21:59:03 UTC
Created attachment 299657 [details] [review]
videoparsers: Use gst_base_parse_merge_tags()

Instead of squashing all upstream tags
Comment 23 Olivier Crête 2015-03-17 22:58:51 UTC
Created attachment 299664 [details] [review]
baseparse: Add Tag handling

Add API to make it easy for subclasses to add tags to the tag event

API: gst_base_parse_merge_tags
Comment 24 betacentauri 2015-03-20 18:58:20 UTC
I have tested your patches, because of my filed bug: https://bugzilla.gnome.org/show_bug.cgi?id=746363 . The test file now shows correct language tags. But other test files still don't show language tags:
Tags read by matroskademux:
taglist, audio-codec=(string)"DTS\ audio", language-code=(string)de;
taglist, audio-codec=(string)"AC-3\ audio", language-code=(string)de;
taglist, audio-codec=(string)"DTS\ audio", language-code=(string)en;

But in c++ part of our application (see other bug) I see for the AC-3 stream no tags at all and for the 2 DTS streams I see no language tag. Only audio-codec, minimum-bitrate, bitrate and maximum-bitrate tags.

By the way the subtitle parsers also need to be adjusted.
Comment 25 Sebastian Dröge (slomo) 2015-03-29 20:12:27 UTC
(In reply to Tim-Philipp Müller from comment #15)
> Comment on attachment 263181 [details] [review] [review]
> baseparse: Add Tag handling
> 
> I think the way these tags are handled here is all wrong (I realise it's
> modelled on the audio/video decoder code, but that also doesn't seem right
> to me).
> 
> As I see it, we should maintain a copy of the upstream tags (latest
> version), and then the parser tags plus merge mode separately. The parser
> tags replace any previous parser tags if updated.
> 
> Whenever upstream tags change or the parser tags are set, we do
> tags_changed=TRUE (which should be renamed to update_tags or resend_tags or
> something).
> 
> Just merging continuously into previous merge results seems a bit weird to
> me.

Agreed, and we should fixup the codec base classes too for this.
Comment 26 Sebastian Dröge (slomo) 2015-07-03 09:14:43 UTC
Ping? Can we get this fixed for 1.6, what is missing here? :)
Comment 27 Tim-Philipp Müller 2015-07-03 09:46:56 UTC
Yes, I have this on my list for 1.6.
Comment 28 Tim-Philipp Müller 2015-08-14 17:01:20 UTC
Created attachment 309295 [details] [review]
audiodecoder: fix tag handling

    Before we just merged everything in pretty much random ways
    ad-hoc instead of keeping state properly. In 0.10 that was
    how it worked, but in 1.x the tag events sent should always
    reflect the latest state and replace any previous tags.
    
    So save the upstream (stream) tags, and save the tags set
    by the decoder subclass with merge mode, and then update
    the merged tags whenever either of those two changes.
    
    This slightly changes the behaviour of gst_audio_decoder_merge_tags()
    in case it is called multiple times, since now any call replaces
    the previously-set tags. However, it leads to much more predictable
    outcomes, and also we are not aware of any subclass which sets this
    multiple times and expects all the tags set to be merged.
    
    If more complex tag merging scenarios are required, we'll have
    to add a new vfunc for that or the subclass has to intercept
    the upstream tags itself and send merged tags itself.
Comment 29 Tim-Philipp Müller 2015-08-14 17:02:34 UTC
Created attachment 309296 [details] [review]
tests: audiodecoder: add unit test for tag handling

This is also how I think it should be done in baseparse, which I'll fix up in the same vein.
Comment 30 Sebastian Dröge (slomo) 2015-08-14 17:05:10 UTC
Comment on attachment 309295 [details] [review]
audiodecoder: fix tag handling

I think this is the only behaviour that really makes sense
Comment 31 Sebastian Dröge (slomo) 2015-08-14 17:06:39 UTC
Before merging this, we should make sure that all parsers and decoders at least are behaving the same
Comment 32 Tim-Philipp Müller 2015-08-15 17:32:16 UTC
Got some work-in-progress patches for baseparse, but there's still two issues that need to be ironed out.
Comment 33 Tim-Philipp Müller 2015-08-15 17:45:52 UTC
Created attachment 309331 [details] [review]
baseparse: save upstream stream tags

Some prep, will probably squash this in the end.
Comment 34 Tim-Philipp Müller 2015-08-15 17:46:51 UTC
Created attachment 309332 [details] [review]
WIP: baseparse: fix tag handling, step 1

    In 0.10 there were no sticky events, and all tag events
    sent would just be merged with the previously-received
    tags. In 1.x we have sticky events, and the tags in the
    tag event(s) should at all times carry the complete tags,
    so we can't just push some tags and then just push tags
    with just bitrates to update the bitrates, etc.
    
    Instead we need to keep track of the upstream stream tags
    received, of the tags set by the video decoder subclass,
    and send an updated tag event with the combined tags
    including our own bitrate tags (if applicable) whenever
    the upstream tags, the subclass tags or any of our bitrates
    change.
    
    MISSING:
    
    1) API for the subclass to set tags
    2) we now send updated tags way too often (~every frame)
Comment 35 Tim-Philipp Müller 2015-08-16 13:34:44 UTC
Created attachment 309357 [details] [review]
baseparse: save upstream stream tags
Comment 36 Tim-Philipp Müller 2015-08-16 13:36:05 UTC
Created attachment 309358 [details] [review]
baseparse: add API for subclass to set tags
Comment 37 Tim-Philipp Müller 2015-08-16 13:36:53 UTC
Created attachment 309359 [details] [review]
baseparse: fix tag handling
Comment 38 Tim-Philipp Müller 2015-08-16 13:39:17 UTC
Created attachment 309360 [details] [review]
flacparse: use new baseparse API and fix tag handling
Comment 39 Sebastian Dröge (slomo) 2015-08-16 15:27:18 UTC
Does it also have to be fixed in videodecoder, audioencoder, videoencoder?
Comment 40 Tim-Philipp Müller 2015-08-16 16:26:42 UTC
Comment on attachment 299651 [details] [review]
mpegaudioparse: Use baseparse tag merging API

Implemented this slightly differently, this patch assumes different merge_tags semantics.
Comment 41 Tim-Philipp Müller 2015-08-18 11:52:35 UTC
Also done for audio encoder, video encoder, video decoder. Let's close this.