GNOME Bugzilla – Bug 630205
[icydemux] Forward tag events downstrem
Last modified: 2010-11-04 09:53:34 UTC
Created attachment 170708 [details] [review] icydemux: forward tag events downstream The icy demuxer doesn't forward tag events downstream. If it's linked after the souphttpsrc element with 'iradio-mode=true' it will swallow tags such as organization, location, genre or encoding, which might be useful downstream.
Created attachment 170712 [details] [review] icydemux: forward tag events downstream Replace MERGE_ALL with MERGE.
Review of attachment 170712 [details] [review]: ::: gst/icydemux/gsticydemux.c @@ +381,3 @@ return gst_pad_event_default (pad, event); + case GST_EVENT_TAG: + if (icydemux->srcpad) { That's almost exactly the same code as in gst_icydemux_parse_and_send_tags(), The code could be made clearer by having a gst_icydemux_found_tags(icydemux, tags) method. @@ +383,3 @@ + if (icydemux->srcpad) { + GST_DEBUG_OBJECT (icydemux, "Forwarding tag event on src pad"); + gst_pad_push_event (icydemux->srcpad, event); You lose the reference on the event here. Either you keep one or you add a break to avoid storing the event below. @@ +393,3 @@ + gst_tag_list_insert (icydemux->cached_tags, tags, + GST_TAG_MERGE_REPLACE); + } Looks like you're missing a break.
Created attachment 170866 [details] [review] icydemux: forward tag events downstream argg, you are right, this patch is crap :p Attaching a new one with your suggestions
Created attachment 170867 [details] [review] icydemux: forward tag events downstream Fix previous patch (return the value from gst_icydemux_tag_found in handle_event)
Review of attachment 170867 [details] [review]: Apart from that minor comment, looks good. ::: gst/icydemux/gsticydemux.c @@ +382,3 @@ + gst_event_parse_tag (event, &tags); + return gst_icydemux_tag_found (icydemux, tags); + } You're leaking the event.
Created attachment 171706 [details] [review] 0001-icydemux-forward-tag-events.patch Fix the event leak
Pushed, thx commit 4c2f5333bbd1766bfb1a3ce30a5ca34195e323d7 Author: Andoni Morales Alastruey <amorales@flumotion.com> Date: Mon Sep 20 19:44:09 2010 +0200 icydemux: forward tag events https://bugzilla.gnome.org/show_bug.cgi?id=630205
Note that this broke tag handling a bit, see bug #633970. Easy to reproduce with: gst-launch uridecodebin uri=http://mp3-vr-32.smgradio.com ! fakesink -v 2>&1 | more
I'm sorry for that, I added the fix for the event leak without testing the patch again.