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 627211 - jpegformat: Push tags after setting srcpad caps
jpegformat: Push tags after setting srcpad caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal normal
: 0.10.21
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-08-17 20:42 UTC by Arun Raghavan
Modified: 2010-11-19 18:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
jpegformat: Push tags after setting srcpad caps (4.84 KB, patch)
2010-08-17 20:42 UTC, Arun Raghavan
needs-work Details | Review
jpegformat: Push tags after setting srcpad caps (5.61 KB, patch)
2010-08-23 16:20 UTC, Arun Raghavan
needs-work Details | Review
jpegformat: Push tags after setting srcpad caps (5.69 KB, patch)
2010-11-19 08:41 UTC, Arun Raghavan
committed Details | Review

Description Arun Raghavan 2010-08-17 20:42:28 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
Comment 1 Arun Raghavan 2010-08-17 20:42:32 UTC
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.
Comment 2 Sebastian Dröge (slomo) 2010-08-19 16:15:21 UTC
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 :)
Comment 3 Arun Raghavan 2010-08-23 16:20:47 UTC
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.
Comment 4 Arun Raghavan 2010-08-23 16:22:02 UTC
(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!
Comment 5 Thiago Sousa Santos 2010-11-19 01:15:27 UTC
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.
Comment 6 Thiago Sousa Santos 2010-11-19 01:34:24 UTC
Other than these details it looks good to me.
Comment 7 Sebastian Dröge (slomo) 2010-11-19 08:19:41 UTC
(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 8 Arun Raghavan 2010-11-19 08:41:13 UTC
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().
Comment 9 Arun Raghavan 2010-11-19 08:41:18 UTC
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.
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2010-11-19 16:20:18 UTC
Looks good to me.
Comment 11 Thiago Sousa Santos 2010-11-19 18:03:21 UTC
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.