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

[Snir Sheriber]

Hi,


On 8/1/19 6:39 PM, Frediano Ziglio wrote:
>> ---
>>   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


Sure


>>       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()) {
>>           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 ?


Indeed, forgot to mention, this was unnoticeable in common usage.
I noticed that leak only when i worked on this patch .

The issue is with the push_sample function which in oppose to push_buffer,
does not take ownership.

Snir.

>>       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");
>>       }
> Frediano