[Bug 796640] qmlglsink: synchronization issue

[GStreamer (GNOME Bugzilla)]

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

--- Comment #15 from Matthew Waters (ystreet00) <ystreet00 at gmail.com> ---
(In reply to jimmy from comment #13)
> (In reply to Matthew Waters (ystreet00) from comment #11)
> > Review of attachment 372742 [details] [review] [review]:
> > 
> > What you might be seeing is a use-before-release where qmlglsink releases
> > the buffer before Qt has drawn with it and could be written too by upstream.
> 
> I think that is exactly what is happening, it looks to me like the buffer is
> sometimes written to by another thread after the lock has been released.
> 
> BUT that doesn't really make sense either, aren't all writes to this buffer
> scheduled to the same stream / command queue? Wouldn't that cause them to be
> done in order?

In theory what might happen is
1. buffer come in and is pushed up to Qt for display
2. next buffer comes in and the previous one is freed and reused.
3. Problem here is that Qt might not have displayed the previous buffer or is
in the process of displaying it while the previous buffer is written to by
upstream (as it has been freed).

So, either we hold the buffer for an extra frame, or only free after 1. the
next frame has arrived and 2. the old frame is not going to be used by Qt. 2.
Might be signalled through fences after the next Qt frame update has completed.

> Another question I have is if I'm correct in assuming that the glupload
> element is writing to the same GstBuffer?

Depends on the pipeline as to which element exactly is writing to the buffer
that is used by qmlglsink.

e.g.

videotestsrc ! glupload ! qmlglsink -> videotestsrc uses downstream's buffer
pool and is the writing element

videotestsrc ! video/x-raw,format=BGRA ! glupload ! glcolorconvert ! qmlglsink
-> glcolorconvert converts between BGRA and RGBA so is the writing element for
qmlglsink.

A case where glupload is the writing element is if upstream of glupload does
not us the GLBufferPool and glupload copies/wraps into a GL texture.

> > 
> > ::: ext/qt/gstqsgtexture.cc
> > @@ +152,3 @@
> > +  *  since the memory in buffer_ is used by other threads
> > +  */
> > +  gst_gl_sync_meta_wait_cpu (sync_meta, this->qt_context_);
> > 
> > This is actually the wrong API to cause a wait and will cause a glFinish()
> > (or equivalent in GL fences) which is way to heavy handed for this.  The
> > fact that they're used by other threads/contexts does not matter in GPU
> > space.
> > 
> > You only need to wait for the GPU to complete (of which there is already
> > code to do that).
> 
> Yes causing a glFinish() is probably very disruptive to the GL command queue
> (will likely cause sync bubbles).
> 
> So how do we wait for the GPU to finish appropriately? There are
> synchronization fences that could be imposed (glFenceSync /
> glClientFenceSync), but in my view as long as they are done on the same GPU
> stream in order there should be no need to halt any CPU threads?

That's what gst_gl_sync_meta_wait() does behind the hood already :)

> By the way, making a copy of the incoming GstBuffer also resolves the issue,
> but I'm sure we would also like to avoid making unnecessary copies.
> 
> Anyways I'm gonna do some more digging, your input is very helpful.

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