[Bug 777146] vaapisink: segfault caused by race condition with OverlayInterface expose method

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=777146

--- Comment #11 from Víctor Manuel Jáquez Leal <vjaquez at igalia.com> ---
(In reply to Matt Staples from comment #10)
> Review of attachment 343453 [details] [review]:
> 
> ::: gst/vaapi/gstvaapisink.c
> @@ +1461,2 @@
>    gst_vaapi_display_unlock (GST_VAAPI_PLUGIN_BASE_DISPLAY (sink));
> +  gst_buffer_replace (&old_buf, NULL);
> 
> I think this changes the behavior.  Previously, sink->video_buffer was set
> to NULL at the end of this function, but now it's being set to a new
> reference to buffer.  Was that intentional?
> If not, I think sink->video_buffer could just be assigned NULL on line 1458.

sink->video_buffer wasn't never set to NULL inside
gst_vaapisink_show_frame_unlocked(), since buffer always has a value, it always
has have a reference to this buffer.

(In reply to Matt Staples from comment #7)
> I like the general approach.  It certainly simplifies the overlay_expose
> method.  
> However, the race condition is still there as long as the call,
> "gst_buffer_replace(&sink->videoBuffer, buffer);" is made outside the lock.

Giving a second thought, the race condition is not there anymore, IIUC, since 
sink->videoBuffer now is not read/write outside of
gst_vaapisink_show_frame_unlocked(), so we can call gst_buffer_replace
(sink->video_buffer, buffer) with the display unlocked.

Nonetheless, I think we'll do it as you propose since it is clearer, IMO.

Do you agree?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.