Indeed, this is not implemented in GStreamer. Do you intend to provide support for that ?

Created attachment 337237 [details] [review]
proposed patch to implement gst_gl_window_dispmanx_egl_set_window_handle

This will not entirely fix the original issue. With this patch it uses the window which is set by app using gst_video_overlay_set_window_handle() But then player resizes the window to video frame resolution.

Review of attachment 337237 [details] [review]:

This needs checking the other native.width/height setting to make sure they don't override the user set variables.

e.g. create_window() sets width/height to 0 unconditionally.

gst_gl_window_dispmanx_egl_create_window() sets width/height to 0 unconditionally. But then it resizes to 16x16 .. 

What kind of checking are you suggesting ? I am totally new to gstreamer and learning it with Raspberry PI3. So don't have much idea with dispmanx and elements internals. Kindly guide me.

Additionally we may have to remove element added by player before setting new element, something like below ?

    if(window_egl->native.element != 0) {
      GST_DEBUG_OBJECT (window, "Remove existing native element");
      update = vc_dispmanx_update_start(0);
      vc_dispmanx_element_remove(update, window_egl->native.element);
      vc_dispmanx_update_submit_sync(update);
    } 

I noticed that even before set_window_handle is called, player adds a window by calling resize_window(16x16)..

Ahhh while going through code, I noticed another bug...in gst_gl_window_dispmanx_egl_set_render_rectangle() 

  window_egl->render_rect.y = x;  ---> this should have been y not x

(In reply to Munez from comment #4)
> gst_gl_window_dispmanx_egl_create_window() sets width/height to 0
> unconditionally. But then it resizes to 16x16 .. 
> 
> What kind of checking are you suggesting ? I am totally new to gstreamer and
> learning it with Raspberry PI3. So don't have much idea with dispmanx and
> elements internals. Kindly guide me.

checking as in making sure that the user set variables are not overriden.  What the other backends do is have a seperate variable for the user set window.  You probably also want to do the same thing here.

(In reply to Munez from comment #5)
> Additionally we may have to remove element added by player before setting
> new element, something like below ?
> 
>     if(window_egl->native.element != 0) {
>       GST_DEBUG_OBJECT (window, "Remove existing native element");
>       update = vc_dispmanx_update_start(0);
>       vc_dispmanx_element_remove(update, window_egl->native.element);
>       vc_dispmanx_update_submit_sync(update);
>     } 
> 
> I noticed that even before set_window_handle is called, player adds a window
> by calling resize_window(16x16)..

Correct.

(In reply to Munez from comment #6)
> Ahhh while going through code, I noticed another bug...in
> gst_gl_window_dispmanx_egl_set_render_rectangle() 
> 
>   window_egl->render_rect.y = x;  ---> this should have been y not x

Correct.  Another patch please for that ;)

(In reply to Matthew Waters (ystreet00) from comment #7)
> (In reply to Munez from comment #4)
> > gst_gl_window_dispmanx_egl_create_window() sets width/height to 0
> > unconditionally. But then it resizes to 16x16 .. 
> > 
> > What kind of checking are you suggesting ? I am totally new to gstreamer and
> > learning it with Raspberry PI3. So don't have much idea with dispmanx and
> > elements internals. Kindly guide me.
> 
> checking as in making sure that the user set variables are not overriden. 
> What the other backends do is have a seperate variable for the user set
> window.  You probably also want to do the same thing here.
> 
Sorry, If I have understood correctly you are suggesting to 

1] add a new member in _GstGLWindowDispmanxEGL structure
 EGL_DISPMANX_WINDOW_T foreign;

2] When set_window_handle is called, assign it to foreign instead of native
3] Everywhere in file gstglwindow_dispmanx_egl.c, make check like, if foreign != 0 then use foreign else use native ?



> (In reply to Munez from comment #5)
> > Additionally we may have to remove element added by player before setting
> > new element, something like below ?
> > 
> >     if(window_egl->native.element != 0) {
> >       GST_DEBUG_OBJECT (window, "Remove existing native element");
> >       update = vc_dispmanx_update_start(0);
> >       vc_dispmanx_element_remove(update, window_egl->native.element);
> >       vc_dispmanx_update_submit_sync(update);
> >     } 
> > 
> > I noticed that even before set_window_handle is called, player adds a window
> > by calling resize_window(16x16)..
> 
> Correct.

If i use above approach then i dont have to do this but have to make sure foreign element is destroyed in cleanup ?
> 
> (In reply to Munez from comment #6)
> > Ahhh while going through code, I noticed another bug...in
> > gst_gl_window_dispmanx_egl_set_render_rectangle() 
> > 
> >   window_egl->render_rect.y = x;  ---> this should have been y not x
> 
> Correct.  Another patch please for that ;)

I am planing to provide a patch all together once it is in good shape :)

(In reply to Munez from comment #8)
> (In reply to Matthew Waters (ystreet00) from comment #7)
> > checking as in making sure that the user set variables are not overriden. 
> > What the other backends do is have a seperate variable for the user set
> > window.  You probably also want to do the same thing here.
> > 
> Sorry, If I have understood correctly you are suggesting to 
> 
> 1] add a new member in _GstGLWindowDispmanxEGL structure
>  EGL_DISPMANX_WINDOW_T foreign;
> 
> 2] When set_window_handle is called, assign it to foreign instead of native
> 3] Everywhere in file gstglwindow_dispmanx_egl.c, make check like, if
> foreign != 0 then use foreign else use native ?

More or less.

When we actually need the native window, foreign should be copied to native and native should be reset when closing.

> > (In reply to Munez from comment #5)
> > > Additionally we may have to remove element added by player before setting
> > > new element, something like below ?
> > > 
> > >     if(window_egl->native.element != 0) {
> > >       GST_DEBUG_OBJECT (window, "Remove existing native element");
> > >       update = vc_dispmanx_update_start(0);
> > >       vc_dispmanx_element_remove(update, window_egl->native.element);
> > >       vc_dispmanx_update_submit_sync(update);
> > >     } 
> > > 
> > > I noticed that even before set_window_handle is called, player adds a window
> > > by calling resize_window(16x16)..
> > 
> > Correct.
> 
> If i use above approach then i dont have to do this but have to make sure
> foreign element is destroyed in cleanup ?

No, that's the application's responsibility.  You don't touch the foreign window just use it a place to render.

> I am planing to provide a patch all together once it is in good shape :)

The smaller the patches, the better they are ;)

(In reply to Matthew Waters (ystreet00) from comment #9)
> (In reply to Munez from comment #8)
> > (In reply to Matthew Waters (ystreet00) from comment #7)
> > > checking as in making sure that the user set variables are not overriden. 
> > > What the other backends do is have a seperate variable for the user set
> > > window.  You probably also want to do the same thing here.
> > > 
> > Sorry, If I have understood correctly you are suggesting to 
> > 
> > 1] add a new member in _GstGLWindowDispmanxEGL structure
> >  EGL_DISPMANX_WINDOW_T foreign;
> > 
> > 2] When set_window_handle is called, assign it to foreign instead of native
> > 3] Everywhere in file gstglwindow_dispmanx_egl.c, make check like, if
> > foreign != 0 then use foreign else use native ?
> 
> More or less.
> 
> When we actually need the native window,
Sorry again. Why would we need the native window if we have set a custom (foreign) window ?
> foreign should be copied to native
This will be like overriding native window values including element_id? Why cant we use foreign directly instead of native 
> and native should be reset when closing.
> 
 If i am not wrong native is used only inside gstglwindow_dispmanx_egl.c right ?


> > > (In reply to Munez from comment #5)
> > > > Additionally we may have to remove element added by player before setting
> > > > new element, something like below ?
> > > > 
> > > >     if(window_egl->native.element != 0) {
> > > >       GST_DEBUG_OBJECT (window, "Remove existing native element");
> > > >       update = vc_dispmanx_update_start(0);
> > > >       vc_dispmanx_element_remove(update, window_egl->native.element);
> > > >       vc_dispmanx_update_submit_sync(update);
> > > >     } 
> > > > 
> > > > I noticed that even before set_window_handle is called, player adds a window
> > > > by calling resize_window(16x16)..
> > > 
> > > Correct.
> > 
> > If i use above approach then i dont have to do this but have to make sure
> > foreign element is destroyed in cleanup ?
> 
> No, that's the application's responsibility.  You don't touch the foreign
> window just use it a place to render.
> 

make sense.

> > I am planing to provide a patch all together once it is in good shape :)
> 
> The smaller the patches, the better they are ;)

Sure. I will create separate patch for this

(In reply to Munez from comment #10)
> (In reply to Matthew Waters (ystreet00) from comment #9)
> > When we actually need the native window,
>
> Sorry again. Why would we need the native window if we have set a custom
> (foreign) window ?

If there's a foreign window, then native == foreign.

The extra variable is so that when we close the window, we don't lose the user set variables.

> > foreign should be copied to native
> This will be like overriding native window values including element_id? Why
> cant we use foreign directly instead of native 

We will. See above.

> > and native should be reset when closing.
> > 
>  If i am not wrong native is used only inside gstglwindow_dispmanx_egl.c
> right ?

Yes.

Created attachment 337265 [details]
playerTest which can be used to test the patch

I am working on patch. Meanwhile i am attaching a player test code which sets dispmanx window handle

In function window_resize()
 we need to check if foreign window is set and use that element, else use the native window element. But at the end of the function it has below lines.

  window_egl->native.width = width;
  window_egl->native.height = height;

Do i need to keep it as it is ? because I cannot assign it to foreign window as it will override user values.

(In reply to Munez from comment #13)
> In function window_resize()
>  we need to check if foreign window is set and use that element, else use
> the native window element. But at the end of the function it has below lines.
> 
>   window_egl->native.width = width;
>   window_egl->native.height = height;
> 
> Do i need to keep it as it is ? because I cannot assign it to foreign window
> as it will override user values.

You don't want to override the values at all when a foreign window is set (as you (and I) said above).

Created attachment 337276 [details] [review]
proposed patch to implement gst_gl_window_dispmanx_egl_set_window_handle

Review of attachment 337276 [details] [review]:

This is not what we discussed.

1. in set_window_handle(), set the foreign handle.
2. when we need the window created to render into, copy the foreign handle to the native handle.

::: gst-libs/gst/gl/dispmanx/gstglwindow_dispmanx_egl.c
@@ +172,3 @@
+  if (window_egl->forgein.element)
+    return (guintptr) & window_egl->forgein;
+  else if (window_egl->native.element)

Is this hunk specifically needed?

(In reply to Matthew Waters (ystreet00) from comment #16)
> Review of attachment 337276 [details] [review] [review]:
> 
> This is not what we discussed.
> 
I am little confused because I am new to gstreamer and not sure about the sequence. for e.g what is the call sequence when there is a custom(foreign) window set and when there is no custom window set.. Please bear with me for my bad patches :)

> 1. in set_window_handle(), set the foreign handle.

So this part of patch is fine right ? I have added a new foreign handle and setting it in set_window handle

> 2. when we need the window created to render into, copy the foreign handle
> to the native handle.
> 
a] Now I am not sure when exactly this happens. From app side we set the window handle when we receive prepare_window_handle message. So which function is the right place ?
b] When you say copy foreign handle did you mean copying only dispmanx element ? i.e  window_egl->native.element = window_egl->foreign.element.
c] if case b] right then we need to remove the native.element(if it is valid) from dispmanx layer before assigning the foreign element right? 

> ::: gst-libs/gst/gl/dispmanx/gstglwindow_dispmanx_egl.c
> @@ +172,3 @@
> +  if (window_egl->forgein.element)
> +    return (guintptr) & window_egl->forgein;
> +  else if (window_egl->native.element)
> 
> Is this hunk specifically needed?

Not required if we are copying foreign element to native element.

(In reply to Munez from comment #17)
> > 1. in set_window_handle(), set the foreign handle.
> 
> So this part of patch is fine right ? I have added a new foreign handle and
> setting it in set_window handle

Yes.

> > 2. when we need the window created to render into, copy the foreign handle
> > to the native handle.
> > 
> a] Now I am not sure when exactly this happens. From app side we set the
> window handle when we receive prepare_window_handle message. So which
> function is the right place ?

Whenever it's being created in the original code.  Instead of creating the dispmanx element, we use the foreign window.

> b] When you say copy foreign handle did you mean copying only dispmanx
> element ? i.e  window_egl->native.element = window_egl->foreign.element.

We copy all of it, the element, width and height.

> c] if case b] right then we need to remove the native.element(if it is
> valid) from dispmanx layer before assigning the foreign element right? 

Not sure what you mean by this.

(In reply to Matthew Waters (ystreet00) from comment #18)
> > c] if case b] right then we need to remove the native.element(if it is
> > valid) from dispmanx layer before assigning the foreign element right? 
> 
> Not sure what you mean by this.

In any case, removing the element from the dispmanx layer is the applications responsibility if it's foreign.  Same with the size and opacity and ...

I think first i need to be clear with the sequence. I added few prints gstglwindow_dispmanx_egl.c  and this is what is happening. I am pasting in sequence

From gstreamer
----------------
a] gst_gl_window_dispmanx_egl_create_window(+157): Create a window
b] window_resize(+253): preferred is 0 0 
c] window_resize(+263): Src (0 0 16 16) Dst (952 532 16 16)
d] window_resize(+277): ==== Create and add a new element ====
e] gst_gl_window_dispmanx_egl_get_window_handle(+170): Get Window handle
f] gst_gl_window_dispmanx_egl_get_window_handle(+170): Get Window handle

From App
---------
g] Received bus_sync_handler................
h] Setting Video Window handle 240200 as overlay

From gstreamer
---------------
i] gst_gl_window_dispmanx_egl_set_render_rectangle(+302):  Set Rend Rect 0 0 1920 1080
j] window_resize(+253): preferred 0 0 
k] window_resize(+263): Src (0 0 1920 1080) Dst (0 0 1920 1080)
l] gst_gl_window_dispmanx_egl_set_window_handle(+184): set window handle with size 1920x1080
m] gst_gl_window_dispmanx_egl_get_window_handle(+170): Get Window handle
n] gst_gl_window_dispmanx_egl_set_preferred_size(+205): set preferred size to 1280x536
o] gst_gl_window_dispmanx_egl_show(+313): Show Window resize if rend rect w/h <=0 1920 1080
p] window_resize(+253): preferred 1280 536 
q] window_resize(+263): Src (0 0 1280 536) Dst (320 272 1280 536)
r] gst_gl_window_dispmanx_egl_close(+126): Remove native.element

Create window is called in the beginning (a]) and it creates and adds a new  dispmanx element at layer 0 with size 16x16 (c] & d]) and this element is stored in and as window_egl->native.element

After this app receives sync handler message in which app sets the a new dispmanx element (g] & h]) 

This new element will be stored in and as window_egl->foreign.element (l])
And finally when we quit native.element is removed from dispmanx layer (r])

So, when app sets the new dispmanx window element , there is already a native element created and stored in native.element  i.e when the create window is called i original code there is no foreign window available to use.

When I said, if we copy everything from foreign to native we need to make sure that native element is removed which was created by original code..

Then there is a call gst_gl_window_dispmanx_egl_close() when we stop the playback which removes native.element from dispmanx layer.

Based on my post above..

(In reply to Matthew Waters (ystreet00) from comment #18)
> (In reply to Munez from comment #17)
> > > 1. in set_window_handle(), set the foreign handle.
> > 
> > So this part of patch is fine right ? I have added a new foreign handle and
> > setting it in set_window handle
> 
> Yes.
> 
> > > 2. when we need the window created to render into, copy the foreign handle
> > > to the native handle.
> > > 
> > a] Now I am not sure when exactly this happens. From app side we set the
> > window handle when we receive prepare_window_handle message. So which
> > function is the right place ?
> 
> Whenever it's being created in the original code.  Instead of creating the
> dispmanx element, we use the foreign window.
> 
In the original code Window_create is called at very beginning even before a foreign window is set. 

> > b] When you say copy foreign handle did you mean copying only dispmanx
> > element ? i.e  window_egl->native.element = window_egl->foreign.element.
> 
> We copy all of it, the element, width and height.
When we copy window_egl->native.element = window_egl->foreign.element we loose the reference of native_window element which was created (16x16) before foreign window set so we need it remove it before assigning foreign element
> 
> > c] if case b] right then we need to remove the native.element(if it is
> > valid) from dispmanx layer before assigning the foreign element right? 
> 
> Not sure what you mean by this.
Like explained above, a native window element is always created before foreign window element is set.

I think this is what we should do.. we need to modify only 2 functions

1] Modify gst_gl_window_dispmanx_egl_set_window_handle()
  a] Store user window in a new member : "foreign"
  b] If "native" element is already set, then remove it from layer
  c] Copy "foreign" into "native" completely

2] Modify gst_gl_window_dispmanx_egl_close()
   a] If "native" element is set and it is same as "foreign" element then don't remove it from layer.

Kindly let me know if this is ok. I will update the patch.

(In reply to Munez from comment #21)
> > > b] When you say copy foreign handle did you mean copying only dispmanx
> > > element ? i.e  window_egl->native.element = window_egl->foreign.element.
> > 
> > We copy all of it, the element, width and height.
> When we copy window_egl->native.element = window_egl->foreign.element we
> loose the reference of native_window element which was created (16x16)
> before foreign window set so we need it remove it before assigning foreign
> element

ACK.

> Like explained above, a native window element is always created before
> foreign window element is set.
> 
> I think this is what we should do.. we need to modify only 2 functions
> 
> 1] Modify gst_gl_window_dispmanx_egl_set_window_handle()
>   a] Store user window in a new member : "foreign"
>   b] If "native" element is already set, then remove it from layer
>   c] Copy "foreign" into "native" completely
> 
> 2] Modify gst_gl_window_dispmanx_egl_close()
>    a] If "native" element is set and it is same as "foreign" element then
> don't remove it from layer.
> 
> Kindly let me know if this is ok. I will update the patch.

Looks good to me.

Created attachment 337399 [details] [review]
proposed patch to implement gst_gl_window_dispmanx_egl_set_window_handle

Created attachment 337409 [details]
proposed patch to implement gst_gl_window_dispmanx_egl_set_window_handle

New to git format-patch :) .. I have updated the patch now

Created attachment 337410 [details] [review]
re-attaching with patch format

commit cb8204f6c0d78e348a7715338326b3d1f4553af5
Author: Munez <munezbn.dev@gmail.com>
Date:   Tue Oct 11 17:06:23 2016 +0530

    Implemented gst_video_overlay_set_window_handle()
    
    https://bugzilla.gnome.org/show_bug.cgi?id=772608