GNOME Bugzilla – Bug 627211
jpegformat: Push tags after setting srcpad caps
Last modified: 2010-11-19 18:03:48 UTC
This defers pushing tags till after srcpad caps are set so that tags are passed downstream. The only thing I'm concerned about is that while collecting the tags, we use GST_TAG_MERGE_REPLACE. I'm not sure what changes in behaviour this introduces, for example, if the same tag is found in EXIF and XMP
Created attachment 168137 [details] [review] jpegformat: Push tags after setting srcpad caps This patch defers emission of tag events till caps are set on the source pad of jpegparse, so that these tags can be seen downstream.
Review of attachment 168137 [details] [review]: ::: gst/jpegformat/gstjpegparse.c @@ +944,3 @@ gst_adapter_clear (parse->priv->adapter); + if (parse->priv->tags) + gst_tag_list_free (parse->priv->tags); Better set the tags to NULL here. It's almost impossible that something goes wrong that bad that the taglist would be accessed again after this point and without going through one of the other places to set it to NULL but it can't hurt ;) Apart from that you should also cache the tags that come from upstream. Other than that the patch is good :)
Created attachment 168562 [details] [review] jpegformat: Push tags after setting srcpad caps This patch defers emission of tag events till caps are set on the source pad of jpegparse, so that these tags can be seen downstream.
(In reply to comment #2) > Review of attachment 168137 [details] [review]: > > ::: gst/jpegformat/gstjpegparse.c > @@ +944,3 @@ > gst_adapter_clear (parse->priv->adapter); > + if (parse->priv->tags) > + gst_tag_list_free (parse->priv->tags); > > Better set the tags to NULL here. It's almost impossible that something goes > wrong that bad that the taglist would be accessed again after this point and > without going through one of the other places to set it to NULL but it can't > hurt ;) I just had this feeling that I shouldn't be tempting fate there, but I ignored it. Fixed now. > Apart from that you should also cache the tags that come from upstream. Also fixed. > Other than that the patch is good :) Thanks for the review!
Review of attachment 168562 [details] [review]: It seems that none of the functions were using take ownership of the taglist, so we might be leaking it here and there. FYI, this seems to be present even without the patch. ::: gst/jpegformat/gstjpegparse.c @@ +581,3 @@ + parse->priv->tags = gst_tag_list_new (); + gst_tag_list_insert (parse->priv->tags, tags, + GST_TAG_MERGE_REPLACE); Leaks the tags @@ +607,3 @@ + parse->priv->tags = gst_tag_list_new (); + gst_tag_list_insert (parse->priv->tags, tags, + GST_TAG_MERGE_REPLACE); Leaks the tags @@ +791,3 @@ + gst_element_found_tags_for_pad (GST_ELEMENT_CAST (parse), + parse->priv->srcpad, parse->priv->tags); + parse->priv->tags = NULL; found_tags_for_pad makes a copy from the taglist, so we need to free it here, too.
Other than these details it looks good to me.
(In reply to comment #5) > @@ +791,3 @@ > + gst_element_found_tags_for_pad (GST_ELEMENT_CAST (parse), > + parse->priv->srcpad, parse->priv->tags); > + parse->priv->tags = NULL; > > found_tags_for_pad makes a copy from the taglist, so we need to free it here, > too. No, it takes ownership of the tag list. The copy is taken for the tag event (which takes ownership itself) and then the messages takes the ownership of the passed taglist.
Comment on attachment 168562 [details] [review] jpegformat: Push tags after setting srcpad caps Attaching updated patch which frees the lists that are passed to gst_tag_list_insert().
Created attachment 174827 [details] [review] jpegformat: Push tags after setting srcpad caps This patch defers emission of tag events till caps are set on the source pad of jpegparse, so that these tags can be seen downstream.
Looks good to me.
Pushed as: commit 8ca66f336e23ff85d3ff22a03950fdfc25cb1a0f Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Wed Aug 18 02:00:10 2010 +0530 jpegformat: Push tags after setting srcpad caps This patch defers emission of tag events till caps are set on the source pad of jpegparse, so that these tags can be seen downstream.