GNOME Bugzilla – Bug 701915
wrappercamerabinsrc: Don't unset the video-source property on pipeline start
Last modified: 2013-08-31 14:24:40 UTC
Created attachment 246384 [details] [review] [PATCH] wrappercamerabinsrc: Don't unset the video-source property on pipeline start [PATCH] wrappercamerabinsrc: Don't unset the video-source property on pipeline start check_and_replace_src() was setting self->app_vid_src to NULL, which means that an app setting the video-source property, and then starting, stopping and re-starting the pipeline (ie to make changes to the video-source-filter property) would after the restart no longer have a video-source. Also if the try_element check on app_vid_src in gst_camerabin_setup_default_element() fails, then gst_camerabin_setup_default_element() can still return a success status and in that case app_vid_src would be set to NULL without its ref being passed, thus leaking memory..
Note, the patch is against the 1.0 branch.
Review of attachment 246384 [details] [review]: ::: gst/camerabin2/gstwrappercamerabinsrc.c @@ -491,3 @@ } - /* we lost the reference */ - self->app_vid_src = NULL; This is correct of course
Review of attachment 246384 [details] [review]: ::: gst/camerabin2/gstwrappercamerabinsrc.c @@ +480,3 @@ + /* If we're using app_vid_src, take a ref to pass on to the bin */ + if (self->src_vid_src == self->app_vid_src) + gst_object_ref (self->app_vid_src); Erm, first review got lost somehow. I think gst_camerabin_setup_default_element() should just return a new reference if it uses the user element, that is more intuitive and causes consistent behaviour of that function.
Created attachment 246403 [details] [review] [PATCH] wrappercamerabinsrc: Don't unset the video-source property on pipeline start (In reply to comment #3) > Review of attachment 246384 [details] [review]: > > ::: gst/camerabin2/gstwrappercamerabinsrc.c > @@ +480,3 @@ > + /* If we're using app_vid_src, take a ref to pass on to the bin */ > + if (self->src_vid_src == self->app_vid_src) > + gst_object_ref (self->app_vid_src); > > Erm, first review got lost somehow. I think > gst_camerabin_setup_default_element() should just return a new reference if it > uses the user element, that is more intuitive and causes consistent behaviour > of that function. Ah good idea, here is a new version.
commit 8dc7fae455266f8e93101fb83edcd9bf41e97544 Author: Hans de Goede <hdegoede@redhat.com> Date: Mon Jun 10 10:05:56 2013 +0200 wrappercamerabinsrc: Don't unset the video-source property on pipeline start check_and_replace_src() was setting self->app_vid_src to NULL, which means that an app setting the video-source property, and then starting, stopping and re-starting the pipeline (ie to make changes to the video-source-filter property) would after the restart no longer have a video-source. This patch fixes this by making gst_camerabin_setup_default_element return a ref to the passed in user_element, rather then returning the user_element as is, so that that ref can be passed on to the bin, and the app_vid_src ref stays valid. https://bugzilla.gnome.org/show_bug.cgi?id=701915
Anything left to do, here?
(In reply to comment #6) > Anything left to do, here? Not from my (the reporter) pov.