destroy_context is not the correct place to call eglTerminate.  What if you have multiple GL contexts? They will all fail in some way from the eglTerminate call.

This should be done in a GstGLDisplayEGL class that the egl using window systems' GstGLDisplay (wayland/x11/rpi/etc) would hold a reference to. Then it would be possible to differentiate between an EGLDisplay and a winsys display as well as get the EGLDisplay from a winsys GstGLDisplay.

Please note this eglTerminate only be called when the display type is not GST_GL_DISPLAY_TYPE_EGL, say the egl_display is created in this context, so this case means each egl context should have it's own egl display. 
I noticed both x11 and wayland backend go into this case.

That may be the case however calling eglTerminate while there's a possibility of there being another user of that display will cause problems.  As we don't know exactly how the native display -> EGLDisplay eglGetDisplay() is implemented (maybe a simple mapping or it might return new EGLDisplay's for each call), we cannot terminate access to an EGLDisplay without knowing nobody is currently using it.

See my comment above on how this should be solved.

This is not the display sharing case, it will create a egl_display in gst_gl_context_egl_create_context() if it's not GST_GL_DISPLAY_TYPE_EGL type, see below code, this means one context will have it's own egl_display, shall it release the resources when this context is destroyed?

  if (display->type == GST_GL_DISPLAY_TYPE_EGL) {
    egl->egl_display = (EGLDisplay) gst_gl_display_get_handle (display);
  } else {
    guintptr native_display = gst_gl_display_get_handle (display);

    if (!native_display) {
      GstGLWindow *window = NULL;
      GST_WARNING ("Failed to get a global display handle, falling back to "
          "per-window display handles.  Context sharing may not work");

      if (other_context)
        window = gst_gl_context_get_window (other_context);
      if (!window)
        window = gst_gl_context_get_window (context);
      if (window) {
        native_display = gst_gl_window_get_display (window);
        gst_object_unref (window);
      }
    }

    egl->egl_display = eglGetDisplay ((EGLNativeDisplayType) native_display);
  }

(In reply to Jian Li from comment #4)
> This is not the display sharing case

That may be true/false according to the EGL implementation.

As I said above, we don't know how eglGetDisplay is implemented and the spec being unclear about this issue, calling eglTerminate on an EGLDisplay that has been eglGetDisplay-ed multiple times with the same native display could cause problems for the other GL contexts.  

I'd be happy to be proved wrong across all the EGL implementation out there (Android, Mesa, NVIDIA, AMD, X11, Wayland, WinCE?, rpi, Mali, etc) however see above for how this reference stuff should be solved.

I got your point.

You mean eglTerminate will impact other connections with the eglDisplay? I believe it only releases resources of current eglDisplay connection.

I think eglTerminate is used to free the resources allocated in eglInitialize, not for eglGetDisplay, see below from spec:
https://www.khronos.org/registry/egl/sdk/docs/man/

eglGetDisplay obtains the EGL display connection for the native display native_display.
Use eglInitialize to initialize the display connection.

eglInitialize initialized the EGL display connection obtained with eglGetDisplay. 
Use eglTerminate to release resources associated with an EGL display connection.

Not sure if my understanding is correct.

eglTerminate attempts to release *any* resource associated with the EGLDisplay connection (or at least marks for deletion and freed when they are not used).  That being *any* EGLSurface/pbuffer/EGLImage?/EGLConfig or eglInitialize()ed, etc objects.

So would this be solved if create a egl display with gst_gl_display_egl_new() rather than create a dummy display for wayland? This will make all the egl context share the same display. Why wayland used a dummy display in gst_gl_display_new()?

And I personal don't believe terminate one eglDisplay will impact other eglDisplay connection. Think about if I run two opengl applications at the same time, they all will get a eglDisplay each when start. If eglTerminate will impact other display connections, one application quit will cause another application has problem.

(In reply to Jian Li from comment #8)
> So would this be solved if create a egl display with
> gst_gl_display_egl_new() rather than create a dummy display for wayland?
> This will make all the egl context share the same display. Why wayland used
> a dummy display in gst_gl_display_new()?

Wayland needs its own GstGLDisplay subclass for other reasons.

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

> And I personal don't believe terminate one eglDisplay will impact other
> eglDisplay connection.

They don't.

> Think about if I run two opengl applications at the
> same time, they all will get a eglDisplay each when start. If eglTerminate
> will impact other display connections, one application quit will cause
> another application has problem.

Different OpenGL applications are in different process spaces entirely.

Think about it like this.

The question is whether (eglGetDisplay (native_display) == eglGetDisplay (native_display)). OR in words, across two invocations of eglGetDisplay are the returned EGLDisplay's the same for the same input native display handle.  The spec is unclear therefore we cannot assume that they are or aren't.

So the solution is to create a gldisplay_wayland class, move wayland display from window_wayland_egl to this class, and do eglGetDisplay and eglTerminate based on the reference count? When create egl context, it will get the egl display from this class directly.

I see there is GST_GL_DISPLAY_TYPE_WAYLAND defined, but no implementation.

commit 2704fdaa590b510316a0be1a4a41a7d77cada1ef
Author: Matthew Waters <matthew@centricular.com>
Date:   Thu May 21 17:48:31 2015 +1000

    gl/wayland: add GstGLDisplayWayland
    
    Simple implementation split from GstGLWindowWayland
    
    Can now have multiple glimagesink elements all displaying output
    linked via GL or otherwise (barring GL platform limitations).
    
    The intel driver is racy and can crash setting up the two glimagesink contexts.
    
    e.g.
    videotestsrc ! tee name=t ! queue ! glupload ! glimagesinkelement
      t. ! queue ! gleffects_blur ! glimagesinkelement
    
    videotestsrc ! glupload ! glfiltercube ! tee name=t ! queue ! glimagesinkelement
      t. ! queue ! gleffects_blur ! glimagesinkelement

(In reply to Jian Li from comment #10)
> So the solution is to create a gldisplay_wayland class, move wayland display
> from window_wayland_egl to this class, and do eglGetDisplay and eglTerminate
> based on the reference count? When create egl context, it will get the egl
> display from this class directly.

Something like that.  It might make more sense to have some kind of interface for that or maybe just a place to store related data on the GstGLDisplay (a GstStructure?).

With this patch, I think we don't need to call eglTerminate. The whole process will share the same display, so there is only one display connection. When process quit, it will free the resources automatically, correct?

Actually I don't know where to call this eglTerminate with current code structure. If move eglGetDisplay into GstGLDisplayWayland, this will change the returned display to egl display, not wayland display.

(In reply to Jian Li from comment #13)
> With this patch, I think we don't need to call eglTerminate. The whole
> process will share the same display, so there is only one display
> connection. When process quit, it will free the resources automatically,
> correct?

The whole process was sharing the same display before, just through GstGLWindow instead of GstGLDisplay.  It's still advisable to call eglTerminate when you're done with EGL resources so you don't leak.

> Actually I don't know where to call this eglTerminate with current code
> structure. If move eglGetDisplay into GstGLDisplayWayland, this will change
> the returned display to egl display, not wayland display.

Yes, we need both native display and eglDisplay which is where the interface/GstStructure comes in.

(In reply to Matthew Waters from comment #14)
> (In reply to Jian Li from comment #13)
> > With this patch, I think we don't need to call eglTerminate. The whole
> > process will share the same display, so there is only one display
> > connection. When process quit, it will free the resources automatically,
> > correct?
> 
> The whole process was sharing the same display before, just through
> GstGLWindow instead of GstGLDisplay.  It's still advisable to call
> eglTerminate when you're done with EGL resources so you don't leak.

Yes, whole process share the same GstGLWindow, but in loop playback, when destroy glimagesink, the GstGLWindow will be destroyed too, so that the wayland display destroyed. If you didn't call eglTerminate, leak occurs.

If split display from window, the display will be stored in the GstContext,in the whole process lifetime, only one display instance, this avoid resource leak.

> 
> > Actually I don't know where to call this eglTerminate with current code
> > structure. If move eglGetDisplay into GstGLDisplayWayland, this will change
> > the returned display to egl display, not wayland display.
> 
> Yes, we need both native display and eglDisplay which is where the
> interface/GstStructure comes in.