Created attachment 251340 [details] [review]
eglglesink: support window handle changes in PAUSED

Review of attachment 251340 [details] [review]:

Why would you like to support changing the window in PAUSED? It's not supported by any of the other video sinks right now (except for glimagesink).

Also eglglessink is going to go away sooner or later, to be replaced by gst-plugins-gl.

::: ext/eglgles/gsteglglessink.c
@@ +959,3 @@
+  }
+  /* We also need a new VBO */
+  eglglessink->render_region_changed = TRUE;

Note that all this happens neither from the streaming thread nor the GL thread. Stuff might crash in interesting ways if you clean up everything here at random times, while rendering still happens for example.

I think you should store the newly set window only here, and next time the streaming thread or GL thread is called you compare the current window to the set window, and if they differ reconfigurate whatever is necessary.

Even if no other sink supports this, it does not mean that the feature is not useful.

On android, whenever the screen is rotated, first you lose the surface handle and then you get a new one. That means that if we keep the sink as is, we need to send the pipeline to READY, change the window id and then go back to the previous position of the playback, which implies a seek and thus download again any source that you where downloading, fill the queues again, etc, etc, you get the idea :)

As you said, the render_region_changed = TRUE is not done from the streaming thread nor the GL thread, that's why on the android side, we don't set the new surface until the pipeline has reached the PAUSED state.

So, to be able to change the window while PLAYING, I suppose the streaming thread/gl thread must be taken. Comparing the window as you propose, is not possible, nothing guarantees that a new window will be sent to the sink with the same pointer but a real new window. And thus all the textures/vbo/whatever context related objects are no longer valid. That's why this patch is to be able to change the window on PAUSED :)

(In reply to comment #3)
> Even if no other sink supports this, it does not mean that the feature is not
> useful.
> 
> On android, whenever the screen is rotated, first you lose the surface handle
> and then you get a new one. That means that if we keep the sink as is, we need
> to send the pipeline to READY, change the window id and then go back to the
> previous position of the playback, which implies a seek and thus download again
> any source that you where downloading, fill the queues again, etc, etc, you get
> the idea :)

Yes, just saying :) It can be useful indeed, you just can't expect every sink to implement that as it's not guaranteed by the interface.

> As you said, the render_region_changed = TRUE is not done from the streaming
> thread nor the GL thread, that's why on the android side, we don't set the new
> surface until the pipeline has reached the PAUSED state.
> 
> So, to be able to change the window while PLAYING, I suppose the streaming
> thread/gl thread must be taken. Comparing the window as you propose, is not
> possible, nothing guarantees that a new window will be sent to the sink with
> the same pointer but a real new window. And thus all the textures/vbo/whatever
> context related objects are no longer valid. That's why this patch is to be
> able to change the window on PAUSED :)

At least all the cleanup must happen when the GL and streaming thread are not running, otherwise stuff will crash.

(In reply to comment #2)
> Review of attachment 251340 [details] [review]:
> 
> Why would you like to support changing the window in PAUSED? It's not supported
> by any of the other video sinks right now (except for glimagesink).
> 
> Also eglglessink is going to go away sooner or later, to be replaced by
> gst-plugins-gl.
> 
> ::: ext/eglgles/gsteglglessink.c
> @@ +959,3 @@
> +  }
> +  /* We also need a new VBO */
> +  eglglessink->render_region_changed = TRUE;
> 
> Note that all this happens neither from the streaming thread nor the GL thread.
> Stuff might crash in interesting ways if you clean up everything here at random
> times, while rendering still happens for example.
> 
> I think you should store the newly set window only here, and next time the
> streaming thread or GL thread is called you compare the current window to the
> set window, and if they differ reconfigurate whatever is necessary.

That should be easily integrated with #705831 to track window changes and delay full reconfigurations in the correct thread.

Created attachment 252193 [details] [review]
0001-eglglessink-fix-expose-after-changing-the-window_han.patch

Use a flag to force a reconfiguration, which is done in the rendering thread.

Review of attachment 252193 [details] [review]:

::: ext/eglgles/gsteglglessink.c
@@ +505,3 @@
   eglBindAPI (EGL_OPENGL_ES_API);
 
+  g_mutex_lock (&eglglessink->render_lock);

Did you test this? ;) The loop of this thread is a few lines below, so this code would only ever be executed right after the thread was started

@@ +965,3 @@
 
+  g_mutex_lock (&eglglessink->render_lock);
+  eglglessink->force_full_reconf = TRUE;

You should probably add some object to the queue to wake up the rendering thread and trigger reconfiguration

Should we close this bug since eglglessink has been replaced by glimagesink ?

Yes, let's close it.

eglglessink has been removed, with glimagesink being the replacement.

Please file a new bug against glimagesink if you still have problems with glimagesink and recent versions of GStreamer (git master or 1.4.x), thanks!