[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 #4 from Matt Staples <staples255 at gmail.com> ---
Review of attachment 343315:
 --> (https://bugzilla.gnome.org/review?bug=777146&attachment=343315)

::: gst/vaapi/gstvaapisink.c
@@ +578,3 @@
   GstVaapiSink *const sink = GST_VAAPISINK (overlay);

+  gst_vaapisink_reconfigure_window (sink);

I thought about that, but the reason I moved the call to reconfigure_window
outside of the check on sink->video_buffer, is that its implementation doesn't
seem to have anything to do with the buffer.  It only works on the caps, which
could get set/changed independent of the buffer (or even before the first
buffer arrives).

Also, by reffing sink->video_buffer inside the lock, then releasing the lock,
and then calling show_frame, there's the possibility of a new frame arriving in
the interim.  I think that would cause this method to re-show the previous
frame out of sequence.

@@ +1463,3 @@
+  if (oldBuffer != NULL) {
+    gst_buffer_unref (oldBuffer);
+  }

Do you mean the check for whether oldBuffer is null?  I didn't know whether
gst_buffer_unref was robust to null pointers, so maybe that's not necessary.

Otherwise, yes, it's necessary to keep the assignment of the sink->video_buffer
variable inside the mutex lock in order to avoid a race with calls to the
expose method on separate threads.  The actual deadlock possibility, for which
we need to leave the lock, appears to only be related to the gst_buffer_unref
operation.

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