GNOME Bugzilla – Bug 687775
Display extended remote video stream (H.323/H.239) simultaneously
Last modified: 2012-12-26 20:38:20 UTC
In the Bug 683456 I added support of displaying an extended remote video stream (H.239). I it's shown through a menu selector in the call window. This means that only one video stream can be shown at once. But our purpose is to display two video streams simultaneously: the main stream (normally a person speaking) and the slides stream (presentation slides) in two different windows, so we could setup two monitors, each one with a window displaying each stream. This patch series must be applied above the patches provided in the bug 683456.
Created attachment 228260 [details] [review] videoutput: new parameter size_changed() signal This patch is traversal to all the videoouput subsystem. It add a new parameter to the size_changed() signal. This new parameter indicates the video output mode of the current frame that changed its size. With this new parameter the UI would know which window resize in case the output is other window.
Created attachment 228261 [details] [review] videooutput: other instance of DisplayInfo Add another instance of DisplayInfo for the new window which will show the extended video stream. And its getters and setters in the class hierarchy.
Created attachment 228262 [details] [review] ui: add remote video window This remote video widget will display the extended remote video stream. For this iteration, the window doesn't add any other facility.
Created attachment 228263 [details] [review] ui: instantiate the extended video window When an extended video stream is opened, a extended video widget is created and displayed.
Created attachment 228264 [details] [review] x-videoutput: select display info with frame mode In the method setup_frame_display() the local display info is choose with the current frame mode, hence the corresponded display window is configured.
Created attachment 228265 [details] [review] x-videooutput: remove zoom in extended video stream
Created attachment 228266 [details] [review] common-videooutput: first new streams, size change later When GMVideoOutputManager::redraw (), if its more important to knew if there is new streams in the connection, so we can allocate the required resources to process it. Afterwards we could process the frame size changes.
Created attachment 228267 [details] [review] common-videoutput: fix code style
Created attachment 228268 [details] [review] x-videoutput: fix code style
Created attachment 228269 [details] [review] common-videoutput: get display with stream type Get the display info structure according to the stream type, since they are different widgets.
Created attachment 228270 [details] [review] x-videoutput: close only current display Added the close_display () method. It will only close the display associated with a videooupt mode (remote/local/pip vs remote extended). This is used when setup_frame_display() we must close only the current display, avoiding the flickering when displaying both streams simultaneously.
Created attachment 228271 [details] [review] videooutput: keep position of remote video display Add the x and y position of the remote stream display, keeping it separated of the main video display.
Created attachment 228272 [details] [review] gmwindow: avoid critical message As the remote video display doesn't store its information in gconf, we don't have a gconf key for it. So, instead a g_return_if_fail(), we put a simple if (), avoiding the critical message.
Created attachment 228273 [details] [review] common-videooutput: trivial fix code style
Created attachment 228274 [details] [review] common-videooutput: switching from/to extended video Differentiate the remote extended stream from the others when evaluating if the mode or the zoom changed. We need to ignore the mode change when switching from or to a remote extended stream, because they are displayed in different windows, hence we shall avoid the display reconstruction.
Created attachment 228275 [details] [review] common-videooutput: trivial code style
Created attachment 228276 [details] [review] videooutput: add a extended last frame Hence we could demux more easily
Created attachment 228277 [details] [review] ext-window: reset display info Reset to zero display info when finalizing
Created attachment 228278 [details] [review] ext-window: don't close the window
Created attachment 228279 [details] [review] ext-window: add clear_display_info()
Created attachment 228280 [details] [review] ext-window: add and use ekiga_ext_window_destroy () Now we hide the window instead of destroy it. It avoids the crash.
Created attachment 228281 [details] [review] x-videooutput: if mode local destroy extended win If the display mode changed to local (which happens at hang-up) the remote window is destroyed.
Created attachment 228282 [details] [review] cal-window: ext-window is created always And it is only show when the extended stream arrives.
Created attachment 228283 [details] [review] call-window: remove the extended video menu option
Created attachment 228284 [details] [review] gconf: add ext_zoom
Created attachment 228285 [details] [review] ext-window: signal for zooming
Created attachment 228286 [details] [review] Revert "x-videooutput: remove zoom in extended video stream" This reverts commit f68a536cbfdd9f2c79f3aa435b20e3b7eb88c116.
Created attachment 228287 [details] [review] ext-window: zooming buttons sensitivity
Created attachment 228288 [details] [review] ext-window: fix zoom_out() limit
Created attachment 228289 [details] [review] ext-window: use glib macros
Created attachment 228290 [details] [review] ext-window: silence the compiler
Created attachment 228291 [details] [review] ext-window: force window resize
Created attachment 228292 [details] [review] ext-window: fix copyright assignation.
(In reply to comment #3) > Created an attachment (id=228262) [details] [review] > ui: add remote video window > > This remote video widget will display the extended remote video stream. > > For this iteration, the window doesn't add any other facility. Some comments: +#include "../../src/common.h" We do not want to rely in lib on files in src. I can fix this myself. + info.gc = GDK_GC_XGC (ew->priv->gc); + info.xdisplay = GDK_GC_XDISPLAY (ew->priv->gc); etc. This code does not work on Windows, I will put the #ifdef myself (except if you prefer to do it, it should be easy). + "key", USER_INTERFACE_KEY "ext_window", Is it interesting to rely on an inexistent key? Maybe it is better not? I suppose this is the reason for attachment #228272 [details]. + /* No size requisition yet + * It's our first call so we silently set the new requisition and exit... What does this mean 'requisition"? Request maybe?
Hi Eugen, Many thanks for reviewing my patches. But I forgot to warn about this patch series in my bug description (I apologize for that): These patches are just an RFC. _I didn't mean to be them integrated_, because they are an ugly way to add a second window with a second video stream. They break the windows port (an other things, I may guess). The reason why I published them here is because I have to publish the changes done, but I'm not sure if they should be maintained in upstream :/ (In reply to comment #34) > (In reply to comment #3) > > Created an attachment (id=228262) [details] [review] [details] [review] > > ui: add remote video window > > > > This remote video widget will display the extended remote video stream. > > > > For this iteration, the window doesn't add any other facility. > > Some comments: > > +#include "../../src/common.h" > We do not want to rely in lib on files in src. I can fix this myself. Ok. > > + info.gc = GDK_GC_XGC (ew->priv->gc); > + info.xdisplay = GDK_GC_XDISPLAY (ew->priv->gc); > etc. > This code does not work on Windows, I will put the #ifdef myself (except if you > prefer to do it, it should be easy). Yep, that was part of what I said. > > + "key", USER_INTERFACE_KEY "ext_window", > Is it interesting to rely on an inexistent key? Maybe it is better not? I > suppose this is the reason for attachment #228272 [details]. > > + /* No size requisition yet > + * It's our first call so we silently set the new requisition and exit... > What does this mean 'requisition"? Request maybe? Yes, I overlooked that. It's better to remove that key.
Created attachment 230077 [details] Screenshot to illustrate what these patches intend
I thought that you show two call windows, whereas in the screenshot I see two ekiga-s (main window + call window in each of them). Also, have you had found a working Windows machine? I would really like to have this functionnality in both ports.
(In reply to comment #37) > I thought that you show two call windows, whereas in the screenshot I see two > ekiga-s (main window + call window in each of them). > > Also, have you had found a working Windows machine? I would really like to > have this functionnality in both ports. Hi Eugen, Sorry for my delay in this reply. I had to work on a couple patches for the Gatekeeper thing (and I'll post them here soon). I have asked for some time along the week 51 to work on these loose ends. I expect that it won't be any issue, and I'll finish these patches.
(In reply to comment #38) > Sorry for my delay in this reply. I had to work on a couple patches for the > Gatekeeper thing (and I'll post them here soon). > > I have asked for some time along the week 51 to work on these loose ends. I > expect that it won't be any issue, and I'll finish these patches. Excellent! Looking forward for your patches.
(In reply to comment #37) > I thought that you show two call windows, whereas in the screenshot I see two > ekiga-s (main window + call window in each of them). No, there's only one instance of ekiga running. When ekiga receives two video streams (H.239 thing), ekiga opens a new window with the second video stream. What do you see in the picture is the main call window and the extended video window, which, in this case, contains the real-time screencast of my own desktop, (running ekiga).
By the way, these patches don't work out-of-the-box anymore, they need to be rebased. If there's interest, I can post the rebased ones.
There is much interest for having these patches inside, so that H239 support be complete! But as you said they are not committable, so I would like to know: - if you plan to modify patch to take into account my comment #34 - if you plan to update these patches so that they can be pushed in, cf. comment #35 - if you have had gatekeeper patches, cf. comment #38 - if you plan to eventually add support for Windows too If you cannot do it, I will try to take care of myself, but it will be much more difficult for me.
(In reply to comment #42) > There is much interest for having these patches inside, so that H239 support be > complete! Really? wow! > > But as you said they are not committable, so I would like to know: > - if you plan to modify patch to take into account my comment #34 > - if you plan to update these patches so that they can be pushed in, cf. > comment #35 I have some work cleaning up the patches, including those comments. But still it is a WIP. > - if you have had gatekeeper patches, cf. comment #38 bug #690618 > - if you plan to eventually add support for Windows too I just did something: now it compiles in windows, but it is buggy: the extended windows is not used and the videos overlaps in the cal window :/ > If you cannot do it, I will try to take care of myself, but it will be much > more difficult for me. I'm running out of time for this task. If I couldn't clean up the patches, I'll just drop them here again and we'll see. Thanks
Let's start to commit these things, even if they work only for linux. We need to update Xlib/DirectX stuff to cairo anyway, so maybe there will be updates in that area. The first two patches are given in the other bug, is that right? We will update the third one, and so on.
I mean, let's look at each patch and if it is good, commit it, otherwise improve it and commit it.
Created attachment 232162 [details] [review] videooutput: new parameter size_changed() signal This patch is traversal to all the videooutput subsystem. It adds a new parameter to the size_changed() signal. This new parameter indicates the video output mode of the current frame that changed its size. With this new parameter the UI would know which window resize in case the output is other window.
Created attachment 232163 [details] [review] videooutput: add another instance of DisplayInfo Add another instance of DisplayInfo for the new window which will show the extended video stream, and its getters and setters in the class hierarchy.
Created attachment 232164 [details] [review] videooutput: select the display info to use In the method {x,dx} setup_frame_display() the local display info is choose with the current frame mode, hence the corresponded display window is configured. In the common-videoouput, we will get the display info structure according to the stream type, since they are different widgets.
Created attachment 232165 [details] [review] common-videooutput: first new streams, size change later When GMVideoOutputManager::redraw (), if its more important to knew if there is new streams in the connection, so we can allocate the required resources to process it. Afterwards we could process the frame size changes.
Created attachment 232166 [details] [review] common-videooutput: fix code style No functional changes.
Created attachment 232167 [details] [review] x-videooutput: fix code style No functional changes.
Created attachment 232168 [details] [review] x-videooutput: close only current display Added the close_display () method. It will only close the display associated with a videooutput mode (remote/local/pip vs remote extended). This is used when setup_frame_display() we must close only the current display, avoiding the flickering when displaying both streams simultaneously.
Created attachment 232169 [details] [review] videooutput: add a extended last frame Hence we could demux more easily, keeping separated the extended video display from the main video display.
Created attachment 232170 [details] [review] gconf: add ext_zoom
Created attachment 232171 [details] [review] ext-window: add remote video window This remote video widget will display the extended remote video stream.
Created attachment 232172 [details] [review] call-window: instantiate the extended video window When an extended video stream is opened, a extended video widget is created and displayed. It is only shown until the extended stream arrives.
(In reply to comment #42) > There is much interest for having these patches inside, so that H239 support be > complete! I'm uploading a rebase of the patches in a more cleaner way. > > But as you said they are not committable, so I would like to know: > - if you plan to modify patch to take into account my comment #34 > +#include "../../src/common.h" > We do not want to rely in lib on files in src. I can fix this myself. Fixed. Now I define the macros in the new ext-window.cpp file > + info.gc = GDK_GC_XGC (ew->priv->gc); > + info.xdisplay = GDK_GC_XDISPLAY (ew->priv->gc); > etc. > This code does not work on Windows, I will put the #ifdef myself (except if > you prefer to do it, it should be easy). Fixed. Now it works in Windows, but it is unstable (the application freeze after hanging up) > + "key", USER_INTERFACE_KEY "ext_window", > Is it interesting to rely on an inexistent key? Maybe it is better not? I > suppose this is the reason for attachment #228272 [details]. Fixed. No ext-window inherits from GtkWindow, not GMWindow, so the gconf keys are not needed. > + /* No size requisition yet > + * It's our first call so we silently set the new requisition and exit... > What does this mean 'requisition"? Request maybe? It is a copy of call-window, I'm a lazy bastard :D > - if you plan to update these patches so that they can be pushed in, cf. > comment #35 They are updated and ready to be pushed if you like. > - if you have had gatekeeper patches, cf. comment #38 Done. > - if you plan to eventually add support for Windows too It works in windows, but ekiga freezes after hanging up as I said. > > If you cannot do it, I will try to take care of myself, but it will be much > more difficult for me. These patches are ready to be pushed, but the UI needs to be polished and a bit of work in Windows.
Victor, a general question: can the extended stream appear during communication, or if it is, then it is from the beginning and it ends at the end of the call? In the former case, is the new window created when the extended stream is created, and disappear when it disappear, in your patches?
Review of attachment 232162 [details] [review]: Ok.
(In reply to comment #58) > Victor, a general question: can the extended stream appear during > communication, or if it is, then it is from the beginning and it ends at the > end of the call? In the former case, is the new window created when the > extended stream is created, and disappear when it disappear, in your patches? The extended stream window appears when a frame of the extended video is processed, and it is hide when the communication ends. Nevertheless, the window is always created, but it is only showed when the stream is processed. Happy holidays Eugen! :)
(In reply to comment #60) > (In reply to comment #58) > > Victor, a general question: can the extended stream appear during > > communication, or if it is, then it is from the beginning and it ends at the > > end of the call? In the former case, is the new window created when the > > extended stream is created, and disappear when it disappear, in your patches? > > The extended stream window appears when a frame of the extended video is > processed, and it is hide when the communication ends. Nevertheless, the window > is always created, but it is only showed when the stream is processed. Good! > Happy holidays Eugen! :) Thanks for finding this!!!
Review of attachment 232163 [details] [review]: Ok.
There were much more patches in the first series, is the second series the same as the first?
Review of attachment 232164 [details] [review]: Ok.
Review of attachment 232165 [details] [review]: Ok.
(In reply to comment #50) > Created an attachment (id=232166) [details] [review] > common-videooutput: fix code style > > No functional changes. GNU coding standards suggests to use || && etc. at the beginning of the next line, do you prefer to put them at the end of previous line?
(In reply to comment #63) > There were much more patches in the first series, is the second series the same > as the first? I rebased almost all patches, and cleaned up all of them a lot.
(In reply to comment #66) > (In reply to comment #50) > > Created an attachment (id=232166) [details] [review] [details] [review] > > common-videooutput: fix code style > > > > No functional changes. > > GNU coding standards suggests to use || && etc. at the beginning of the next > line, do you prefer to put them at the end of previous line? It makes sense, and yes, I did it because I'm used to place them in that way. Should I change it?
No need, ekiga code is by the way mixed.
Review of attachment 232166 [details] [review]: Ok.
Review of attachment 232167 [details] [review]: Ok.
Review of attachment 232169 [details] [review]: Ok.
Review of attachment 232170 [details] [review]: Ok.
Victor, with the full support of H.239 as you propose here, are H.239 fields in Preferences still needed or should they be removed?
(In reply to comment #56) > Created an attachment (id=232172) [details] [review] > call-window: instantiate the extended video window > > When an extended video stream is opened, a extended video widget is created > and displayed. It is only shown until the extended stream arrives. I suppose the last word should be "closed", not "arrives" ?! When an extended video stream is opened, an extended video widget is created and displayed. It is shown until the extended stream is closed.
(In reply to comment #74) > Victor, with the full support of H.239 as you propose here, are H.239 fields in > Preferences still needed or should they be removed? They are still needed. With them you can reject the extended stream video. Even, if I understand correctly, choose different roles in that extended video, but I haven't tried that feature. Also, there's also another piece (for which I am not interested at the moment): ekiga could make a stream from a desktop grabber and stream it.
(In reply to comment #75) > (In reply to comment #56) > > Created an attachment (id=232172) [details] [review] [details] [review] > > call-window: instantiate the extended video window > > > > When an extended video stream is opened, a extended video widget is created > > and displayed. It is only shown until the extended stream arrives. > > I suppose the last word should be "closed", not "arrives" ?! > > When an extended video stream is opened, an extended video widget is > created and displayed. It is shown until the extended stream is closed. Yes, I wrote it incorrectly. The stream will be shown until the extended stream is closed.
(In reply to comment #76) > Also, there's also another piece (for which I am not interested at the moment): > ekiga could make a stream from a desktop grabber and stream it. Ok, that would have been my next question..
Review of attachment 232171 [details] [review]: Ok.
Review of attachment 232172 [details] [review]: Ok.
All the patches are now in, thank you very much for your work!
Do you plan to work on other things, and if yes which one?
The gstreamer video input code in ekiga already does desktop video capture. When I tell you gstreamer is the way to go for media streams and devices in ekiga...
(In reply to comment #57) > These patches are ready to be pushed, but the UI needs to be polished and a bit > of work in Windows. And finally, do you plan to fix these issues, especially the Windows issue?