GNOME Bugzilla – Bug 762793
rgvolume: modifies upstream tags in place without making them writable first
Last modified: 2016-04-14 17:42:42 UTC
Created attachment 322552 [details] [review] Patch: "When sending a tag event in GstTagInject, ensure that changes by other elements do not influence the tags sent in the future." taginject, a debugtools element, is configured to add specific tags to the pipeline. Upon init(), it converts this to a GstTagList* (self->tags), which it pushes towards the elements after it in gst_tag_inject_transform_ip(). However, since it does not make a copy of this list, any changes to the list by further elements effectively influence self->tags. Thus, upon pipeline changes, when it decides to re-send its tag list, it does not send the tags from the configured 'tags' property, but the tags as modified by further elements. The attached patch fixes this. I reported a bug resulting from this issue against GStreamer Editing Services at <https://phabricator.freedesktop.org/T7332>. There, taginject is used to inject replaygain peak, gain and reference levels, which are then interpreted by rgvolume to apply the correct volume normalizations and subsequently removed from the tag list. However, as soon as NLE makes a pipeline change, the tag list no longer contains the values to correctly compute the replay gain normalization, causing the normalization to fall away. More details can be found at the link above.
Can you provide a testcase for the bug you observe? No downstream elements should be able to modify the tags stored inside taginject... as taginject has one reference and downstream another, which means that the taglist is read-only now and you get assertions when trying to modify it.
I have a test case (attached) that works with GStreamer Editing Services, but did not succeed in extracting one that works with pure GStreamer. I was pretty sure the bug occurs when a pipeline is PLAYING with taginject+rgvolume, then set to READY and set to PLAYING, but this did not turn out to be enough to trigger the bug. Anyway, the creation of the GstTagList happens at gst_tag_inject_set_property, the sending happens at gst_tag_inject_transform_ip (note that no copy is made), and then the modification happens at the bottom of gst_rg_volume_tag_event, and I could confirm that at the end of gst_tag_inject_transform_ip the tags were indeed gone from the GstTagList. (I think that rgvolume removes the tags so that in the bin "rgvolume ! rgvolume ! rgvolume" only the first rgvolume actually normalizes, and the other two don't see any tags and fallback to a 1.0 gain. But, I haven't tested this.)
Created attachment 322574 [details] Test case Test case. Contains compilation instructions. It requires any foo.mp3 in the same directory to play, or just change the source. As said, this is a test case that requires GStreamer Editing Services, but the actual bug is in GStreamer (probably plugins-good), I just didn't succeed in writing a pure GStreamer test case. I'll shortly describe what this test case does for those not familiar with GES: * It initializes GStreamer and GES * It plays a GES_TYPE_TEST_CLIP, which is equivalent to GstAudioTestSrc, for 6 seconds, and right after that another for 6 seconds * It opens foo.mp3 and plays it for 20 seconds * It adds "taginject tags=\"..\" ! rgvolume" to the foo.mp3 clip * At this point, a GstAudioTestSrc and foo.mp3 are playing at the same time * At the moment the GstAudioTestSrc stops, at T+6 seconds, NLE rebuilds the pipeline causing the taginject!rgvolume bug described here.
Created attachment 322576 [details] [review] rgvolume: make tag list writable before modifying it Any chance you could test this patch?
Made a unit test, so confirmed rgvolume does this wrong: commit a4d64b5caa3fac5223e58b6d791fbd6fbe44ff72 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Feb 28 13:42:28 2016 +0000 rgvolume: make tag list writable before modifying it Making the event itself writable is not enough, it won't make the actual taglist in the event writable as well. Instead, just make a copy of the taglist and then create a new tag event from that if required, replacing the old one. Before we would inadvertently modify taglists upstream elements might still be holding on to. Add unit test for this as well. https://bugzilla.gnome.org/show_bug.cgi?id=762793 commit f02e52ba3ff11cdd72607591b878f1f4fa4e8faf Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Feb 28 13:59:48 2016 +0000 taglist: add guard to check writability when removing tags from a taglist https://bugzilla.gnome.org/show_bug.cgi?id=762793 Please re-open if you still have problems after these commits, thanks!
Comment on attachment 322552 [details] [review] Patch: "When sending a tag event in GstTagInject, ensure that changes by other elements do not influence the tags sent in the future." Thanks for the patch. While I think it would make this particular use case work, I don't think this is the place to fix it, the problem seems to be in rgvolume really.
Awesome! I did not know that a GstTagList was supposed to be read-only as soon as it is propagated. In that case, this is a more appropriate fix. My issue is indeed fixed with this. Thank you!