[Spice-devel] [PATCH spice-streaming-agent 3/4] gst-plugin: Free input buffer and XImage as soon as possible

[Frediano Ziglio]

> 
> > 
> > ---
> >  src/gst-plugin.cpp | 31 ++++++++++++++++++++-----------
> >  1 file changed, 20 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> > index 0a1d041..c7412c5 100644
> > --- a/src/gst-plugin.cpp
> > +++ b/src/gst-plugin.cpp
> > @@ -67,6 +67,15 @@ struct GstSampleDeleter {
> >  
> >  using GstSampleUPtr = std::unique_ptr<GstSample, GstSampleDeleter>;
> >  
> > +struct GstBufferDeleter {
> > +    void operator()(GstBuffer* p)
> > +    {
> > +        gst_buffer_unref(p);
> > +    }
> > +};
> > +
> > +using GstBufferUPtr = std::unique_ptr<GstBuffer, GstBufferDeleter>;
> > +
> >  class GstreamerFrameCapture final : public FrameCapture
> >  {
> >  public:
> > @@ -86,7 +95,6 @@ private:
> >      Display *const dpy;
> >  #if XLIB_CAPTURE
> >      void xlib_capture();
> > -    XImage *image = nullptr;
> >  #endif
> >      GstObjectUPtr<GstElement> pipeline, capture, sink;
> >      GstSampleUPtr sample;
> > @@ -306,12 +314,6 @@ void GstreamerFrameCapture::free_sample()
> >          gst_buffer_unmap(gst_sample_get_buffer(sample.get()), &map);
> >          sample.reset();
> >      }
> > -#if XLIB_CAPTURE
> > -    if(image) {
> > -        image->f.destroy_image(image);
> > -        image = nullptr;
> > -    }
> > -#endif
> >  }
> >  
> >  GstreamerFrameCapture::~GstreamerFrameCapture()
> > @@ -327,9 +329,14 @@ void GstreamerFrameCapture::Reset()
> >  }
> >  
> >  #if XLIB_CAPTURE
> > +void free_ximage(gpointer data) {
> 
> bracket on next line
> 
> > +    ((XImage*)data)->f.destroy_image((XImage*)data);
> > +}
> > +
> >  void GstreamerFrameCapture::xlib_capture()
> >  {
> >      int screen = XDefaultScreen(dpy);
> > +    XImage *image = nullptr;
> >  
> 
> I would declare + initialize on the same line
> 
> >      Window win = RootWindow(dpy, screen);
> >      XWindowAttributes win_info;
> > @@ -355,9 +362,11 @@ void GstreamerFrameCapture::xlib_capture()
> >          throw std::runtime_error("Cannot capture from X");
> >      }
> >  
> > -    GstBuffer *buf;
> > -    buf = gst_buffer_new_wrapped(image->data, image->height *
> > image->bytes_per_line);
> > -    if (!buf) {
> > +    GstBufferUPtr buf(gst_buffer_new_wrapped_full((GstMemoryFlags)0,
> > image->data,
> > +                                                  image->height *
> > image->bytes_per_line, 0,
> > +                                                  image->height *
> > image->bytes_per_line, image,
> > +                                                  free_ximage));
> > +    if (!buf.get()) {

Minor: just "if (!buf) {" is enough.

> >          throw std::runtime_error("Failed to wrap image in gstreamer
> >          buffer");
> >      }
> >  
> > @@ -369,7 +378,7 @@ void GstreamerFrameCapture::xlib_capture()
> >                                           nullptr));
> >  
> >      // Push sample
> > -    GstSampleUPtr appsrc_sample(gst_sample_new(buf, caps.get(), nullptr,
> > nullptr));
> > +    GstSampleUPtr appsrc_sample(gst_sample_new(buf.get(), caps.get(),
> > nullptr, nullptr));
> 
> I'm a bit confused on reference counting here.
> The change seems to indicate that now "buf" is freed automatically
> (as the unique_ptr).
> So, is this patch also fixing a memory leak ?
> 
> >      if (gst_app_src_push_sample(GST_APP_SRC(capture.get()),
> >      appsrc_sample.get()) != GST_FLOW_OK) {
> >          throw std::runtime_error("gstramer appsrc element cannot push
> >          sample");
> >      }