Re: [PATCH v3 2/9] add vnc h264 encoder

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Apr 24, 2025 at 10:43:21AM +0200, Dietmar Maurer wrote:
> 
> > On 24.4.2025 09:32 CEST Dietmar Maurer <dietmar@proxmox.com> wrote:
> > 
> >  
> > > > +    gst_object_ref(vs->h264->source);
> > > > +    if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
> > > > +        gst_object_unref(vs->h264->source);
> > > > +        VNC_DEBUG("Could not add source to gst pipeline\n");
> > > > +        goto error;
> > > > +    }
> > > 
> > > If you put the gst_object_ref call after sucessfully calling
> > > gst_bin_add, then it wouldn't need the gst_object_unref call
> > > on failure. Repeated many times below.
> > 
> > Gstreamer docs claims that gst_bin_add() takes ownership of the element. So I assumed that it unref the element in case of error.
> > If I do not ref the object before, this would free the object too early.
> > 
> > But a look at the source code of gstbin.c reveals that it does
> > not unref the element in case of errors, so your suggestion works.
> > I will change that in the next version...
> 
> From the gstreamer docs about refcounting and gst_bin_add:
> 
> https://gstreamer.freedesktop.org/documentation/additional/design/MT-refcounting.html?gi-language=c#refcounting1
> 
> > As soon as this function is called in a Bin, the element passed as an argument is owned by the bin and you are not allowed to access it anymore without taking a _ref() before adding it to the bin. 
> 
> This clearly states we should taking a _ref() before adding it to the bin?

Yes, you are correct, ignore my suggestion.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|