GNOME Bugzilla – Bug 679768
mpegaudioparse, baseparse: fix tag handling
Last modified: 2015-08-18 11:52:35 UTC
Created attachment 218589 [details] [review] Patch currently the parser immediately replaces the tag event containing nominal bitrate with another event.
Created attachment 229300 [details] [review] Updated patch
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).
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.
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").
*** Bug 711710 has been marked as a duplicate of this bug. ***
(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).
Created attachment 263179 [details] [review] baseparse: Remove duplicate code These are already free by gst_base_parse_clear_queues
Created attachment 263180 [details] [review] baseparse: Add utility function to push queued events This code was present three times
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
Created attachment 263182 [details] [review] mpegaudioparse: Use baseparse tag merging API This way, existing tags are not overwritten, but correctly merged.
Created attachment 263183 [details] [review] flacparse: Use baseparse tag merging API This way, existing tags are not overwritten, but correctly merged.
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.
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
Olivier?
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 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
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.
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
Created attachment 299651 [details] [review] mpegaudioparse: Use baseparse tag merging API This way, existing tags are not overwritten, but correctly merged.
Created attachment 299652 [details] [review] flacparse: Use baseparse tag merging API This way, existing tags are not overwritten, but correctly merged.
Created attachment 299654 [details] [review] audioparsers: Use gst_base_parse_merge_tag() API
Created attachment 299657 [details] [review] videoparsers: Use gst_base_parse_merge_tags() Instead of squashing all upstream tags
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
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.
(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.
Ping? Can we get this fixed for 1.6, what is missing here? :)
Yes, I have this on my list for 1.6.
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.
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 on attachment 309295 [details] [review] audiodecoder: fix tag handling I think this is the only behaviour that really makes sense
Before merging this, we should make sure that all parsers and decoders at least are behaving the same
Got some work-in-progress patches for baseparse, but there's still two issues that need to be ironed out.
Created attachment 309331 [details] [review] baseparse: save upstream stream tags Some prep, will probably squash this in the end.
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)
Created attachment 309357 [details] [review] baseparse: save upstream stream tags
Created attachment 309358 [details] [review] baseparse: add API for subclass to set tags
Created attachment 309359 [details] [review] baseparse: fix tag handling
Created attachment 309360 [details] [review] flacparse: use new baseparse API and fix tag handling
Does it also have to be fixed in videodecoder, audioencoder, videoencoder?
Comment on attachment 299651 [details] [review] mpegaudioparse: Use baseparse tag merging API Implemented this slightly differently, this patch assumes different merge_tags semantics.
Also done for audio encoder, video encoder, video decoder. Let's close this.