Created attachment 29648 [details] [review]
patch to backup/set error handler and GST_ELEMENT_ERROR call from _chain

by setting an X error handler, you break gdk.

I think we can still chain up to the old handler by just calling it, no ?

That is very ugly, because it causes side effects in unrelated parts of the
program (other ximagesinks). It also doesn't recover from errors in other parts
of the code - like state changes or buffer allocations, that might not even be
critical (-ENOMEM on buffer alloc for example).

I'd suggest that if you want to be able to debug X errors, use a global mapping
of display <=> element and properly lock it when adding/removing/querying
elements from this mapping.
And check the error stuff after every XSync, if you have avoidable errors (like
buffer_alloc).

Whoops, XSetErrorHandler is not per display but global?

In that case I'd not use it at all, because you cannot guarantee it works
correctly with other error handlers set by other libs/applications.

XSetErrorHandler is fine, it's setting a global boolean from the handler that is
the problem.

for making it work with more than one sink, yes, using a global hash of displays
is what I would use.  Only atm it's not that critical since we don't actually
use two sinks at the same time, and even if you do all that will happen is that
concurrent errors will get discarded.

What will happen is that an error for ximagesink1 is triggered and ximagesink2
throws an error.

What will also happen is that libfoo installs a custom error handler, GStreamer
installs a custom error handler, libfoo uninstalls the custom error handler and
then GStreamer uninstalls the custom handler, resetting it to libfoo's handler
(that should already be uninstalled).

Those are 2 bugs I don't want in my code for no real benefit at all.

heh, we need the error handler since that's the only way we can check if some
functions (like, XvPutImage) that can fail for whatever reason have failed. 
Let's not conjecture what XSetErrorHandler does, but look into it and verify it.
 Let's not worry about other libs that do the wrong thing when
installing/removing handlers, but make sure GStreamer does the right thing.

I already know how XSetErrorHandler() works.  If you set an error handler, you
will steal errors from gdk.

You can attempt to work around it; XSetErrorHandler() will return the old error
handler, and you can call that if a particular error is not for your display,
but this is a problem a) if ximagesink is initialized before gdk (because gdk
will simply complain and abort instead of calling the ximagesink error handler)
or b) if you have multiple ximagesinks, and attempt to free them in a different
order than allocated.

It's not too bad if one sets an error handler, runs some operations, calls
XSync(), and then sets the old error handler.  But having an error handler set
all the time should be reserved for gdk, since gdk really needs it.
It's a problem in Xlib.  It sucks, I know.

I don't think setting an error handler in x(v)imagesink will interfere with gdk
because x(v)imagesink opens its own connection (Display) to the X server.

I thought of one reason why one would use XSync() -- between creating an X
window and handing off the XID of that window via an xoverlay call.  The XSync
call guarantees the X server knows about the window before the other caller
attempts to use the window from a different connection.

confirming bug, adding PATCH keyword

Comment on attachment 29648 [details] [review]
patch to backup/set error handler and GST_ELEMENT_ERROR call from _chain

It can be useful for debugging. Therefore, add a property (like the sync
property) to only set the error handler if the specific property is set. That
can be useful for gs-tlaunch pipelines. You also don't need to store the old
handler then, because it'll break anyway. Setting to needs-work to reflect
this.

Updating for plugins-base and the version. But it's been a year, what's up with this? Julien?

I think we should leave the error handling to gdk.

I don't think that patch is usefull.

closing, the x(v)imagesink jedi has spoken.