GNOME Bugzilla – Bug 683456
Display extended remote video stream (H.323/H.239)
Last modified: 2013-03-04 08:57:29 UTC
The following patch set is required to add the option in the UI to show the extended remote video stream (supported by H.323/H.239)
Created attachment 223588 [details] [review] common-videooutput: refactor frame_display_change_needed () Make more readable the method using local boolean variables representing the changes in the frames or in the UI window.
Created attachment 223589 [details] [review] common-videooutput: store the extended stream buffer When a second remote video stream (extended stream) arrives, it is stored in a new byte array called eframeStore. The dimensions of the frame are also store, along with a new boolean property that confirms the existance of an extended stream in the connection. This patch doesn't modifies the current user experience, but it is a requirement for the extended video display.
Created attachment 223590 [details] [review] common-videooutput: extended stream needs update This patch adds the logic to mark when the extended stream is updated, meaning that a new frame has been stored int the byte array. The patch doesn't add visible changes in the UI. All remains behind the curtains.
Created attachment 223591 [details] [review] common-videooutput: add VO_MODE_REMOTE_EXT mode This patch adds the extended remote video stream enum in the video output mode. Also it adds the switch labels where the enum is handled in order to avoid compiler's complains, but no logic is added in those cases, in other words it is just the skeleton. In the following commits that logic will be added.
Created attachment 223592 [details] [review] x-videooutput: refactor sync() Make more readable the method using a local boolean variable that represents no synchronization demand by any video stream.
Created attachment 223593 [details] [review] x-videooutput: resources for the extended stream This patch adds the X resources required to display the extended stream in case the user selects that view. The resources are a XWindow and a Display instances. The patch contains the logic to handle these two new instances: construction, destruction, and also receives the frame from the byte array and the display synchronization. Since there is no a way in the user interface for enabling the extended stream view, this patch doesn't modify the user experience. TODO: add this bits for the dx-videooutput. Since I don't have any experience, nor access to DirectX, I prefer avoid its modification.
Created attachment 223594 [details] [review] common-videooutput: extended stream in frame info This patch adds a boolean variable (ext_stream_active). It tells if there is an extended remote video stream in the connection. It is stored in the frame info (FrameInfo) structure. FrameInfo is the structure that is going to be used for each frame that arrives. So if will tell to the user interface if connection contains an extended video. The patch specifies the value of this new attribute for current_frame and last_frame instances of FrameInfo.
Created attachment 223595 [details] [review] videoutput: new parameter device_opened() signal This patch is traversal to all the videoouput subsystem. It add a new parameter to the on_device_opened() signal. This new parameter indicates if there is an extended stream in the connection. With it, the UI can enable the option to display the extended stream in the video widget.
Created attachment 223596 [details] [review] common-videooutput: display extended stream This patch contains the logic when the extended remote video stream changes its size, hence the UI window should be adjusted. Also contains the case that if the stream adds or removes the extended stream, the UI setup should be readjusted too. Finally this patch handles the frame display of the extended remote stream if the UI requests it. No UI changes yet though.
Created attachment 223597 [details] [review] ui: add the extended video option With this patch the user can choose to display the "extended video" through the view menu. The menu option is only enabled when the on_videoutput_device_opened_cb() callback has the extended stream (ext_stream) as true. When the opened video output device doesn't contain an extended stream, and the view is set on that stream, the configuration is reset to the local view. Also, the validation of the values for the key /apps/ekiga/general/user_interface/video_display/video_view is more generic, using the enum symbols, rather than the numbers.
Created attachment 223678 [details] [review] ui: add the extended video option With this patch the user can choose to display the "extended video" through the view menu. The menu option is only enabled when the on_videoutput_device_opened_cb() callback has the extended stream (ext_stream) as true. When the opened video output device doesn't contain an extended stream, and the view is set on that stream, the configuration is reset to the local view. Also, the validation of the values for the key /apps/ekiga/general/user_interface/video_display/video_view is more generic, using the enum symbols, rather than the numbers.
Created attachment 224063 [details] [review] common-videooutput: add VO_MODE_REMOTE_EXT mode This patch adds the extended remote video stream enum in the video output mode. Also it adds the switch labels where the enum is handled in order to avoid compiler's complains, but no logic is added in those cases, in other words it is just the skeleton. In the following commits that logic will be added.
Patches from comments 1 to 3 are committed, thank you! The others, a bit later.
(In reply to comment #13) > Patches from comments 1 to 3 are committed, thank you! The others, a bit > later. Thanks! Any comments or rebasement requests are very welcome :)
Well, we are preparing the stable release 4.0.0 and after some discussion we will commit all the other patches right after the release, when we will create the stable branch. Hope it is fine for you. Thank you again!
Thanks for those patches! Have you had the chance (or would you have the chance) to help porting that difficult part to GTK3? Thanks
(In reply to comment #16) > Thanks for those patches! > > Have you had the chance (or would you have the chance) to help porting that > difficult part to GTK3? > > Thanks Hi Damien, I'm running out of my time for Ekiga, so won't have chance to work on porting to GTK3 in the near future. Anyway, I created the bug #685404 to keep track of this task. Also I dumped my observation about what's the problem porting the video display to gtk3.
Created attachment 228225 [details] [review] copyright assignation. Given this results: $ git blame --line-porcelain lib/engine/components/common-videooutput/videooutput-manager-common.cpp | sed -n 's/^author //p' | sort | uniq -c | sort -rn 281 Matthias Schneider 118 Víctor Manuel Jáquez Leal 21 Julien Puydt 5 Damien Sandras $ git blame --line-porcelain lib/engine/components/x-videooutput/videooutput-manager-x.cpp | sed -n 's/^author //p' | sort | uniq -c | sort -rn 270 Matthias Schneider 254 Víctor Manuel Jáquez Leal 39 Julien Puydt 1 Damien Sandras $ git blame --line-porcelain lib/engine/components/x-videooutput/videooutput-manager-x.h | sed -n 's/^author //p' | sort | uniq -c | sort -rn 93 Matthias Schneider 20 Víctor Manuel Jáquez Leal 15 Julien Puydt 1 Damien Sandras And by request of my client, I propose to add my client the copyright assignation, and myself in the authoring of these files.
After a bit of more thinking, we are pushing your useful patches now, thank you! (In reply to comment #6) > Created an attachment (id=223593) [details] [review] > x-videooutput: resources for the extended stream > > This patch adds the X resources required to display the extended stream in > case the user selects that view. [...] > TODO: add this bits for the dx-videooutput. Since I don't have any experience, > nor access to DirectX, I prefer avoid its modification. What does this mean for directx? Does this means only that on Windows (the platform using directx) the main video is always shown? Does it break anything on Windows? Testing directx can be done using http://wiki.ekiga.org/index.php/Building_Ekiga_for_Windows and executing on Windows.
(In reply to comment #11) > Created an attachment (id=223678) [details] [review] > ui: add the extended video option > > With this patch the user can choose to display the "extended video" through > the view menu. > > The menu option is only enabled when the on_videoutput_device_opened_cb() > callback has the extended stream (ext_stream) as true. > > When the opened video output device doesn't contain an extended stream, and > the view is set on that stream, the configuration is reset to the local view. > > Also, the validation of the values for the key > /apps/ekiga/general/user_interface/video_display/video_view is more generic, > using the enum symbols, rather than the numbers. Could you please tell why this code: + if (view > 2) // let's jump VO_MODE_PIP_WINDOW & VO_MODE_FULLSCREEN modes + view += 2;
(In reply to comment #19) > After a bit of more thinking, we are pushing your useful patches now, thank > you! > > (In reply to comment #6) > > Created an attachment (id=223593) [details] [review] [details] [review] > > x-videooutput: resources for the extended stream > > > > This patch adds the X resources required to display the extended stream in > > case the user selects that view. > [...] > > TODO: add this bits for the dx-videooutput. Since I don't have any experience, > > nor access to DirectX, I prefer avoid its modification. > > What does this mean for directx? Does this means only that on Windows (the > platform using directx) the main video is always shown? Does it break anything > on Windows? It means that the windows port won't show the extended video streams. All the request sent to videooutput-manager-dx to deal with it will be ignored. I don't thing it would break the windows port, but I need to verify it. > > Testing directx can be done using > http://wiki.ekiga.org/index.php/Building_Ekiga_for_Windows and executing on > Windows. Ok. I'll give it try.
(In reply to comment #20) > (In reply to comment #11) > > Created an attachment (id=223678) [details] [review] [details] [review] > > ui: add the extended video option > > > > With this patch the user can choose to display the "extended video" through > > the view menu. > > > > The menu option is only enabled when the on_videoutput_device_opened_cb() > > callback has the extended stream (ext_stream) as true. > > > > When the opened video output device doesn't contain an extended stream, and > > the view is set on that stream, the configuration is reset to the local view. > > > > Also, the validation of the values for the key > > /apps/ekiga/general/user_interface/video_display/video_view is more generic, > > using the enum symbols, rather than the numbers. > > Could you please tell why this code: > + if (view > 2) // let's jump VO_MODE_PIP_WINDOW & VO_MODE_FULLSCREEN > modes > + view += 2; In the attachment (id=224063) (the first patch to apply!) the symbol VO_MODE_REMOTE_EXT is added as part of the VideoOutputMode enum: typedef enum { VO_MODE_LOCAL, VO_MODE_REMOTE, VO_MODE_PIP, VO_MODE_PIP_WINDOW, VO_MODE_FULLSCREEN, VO_MODE_REMOTE_EXT, VO_MODE_UNSET } VideoOutputMode; As you can see, the VO_MODE_REMOTE_EXT is evaluated as 5. The purpose of that code is to store the current view (as enum value) in gconf for the next call. What we receive in the function is the menu item index, which comprehends local [0], remote [1], pip [2] and (now) remote_ext [3]. Previously, as the menu item index mapped transparently with the enum index, the simple value was used. But now, in the case of remote_ext, it doesn't anymore, (as 3 != 5). So, if the menu item index is bigger than two, we "jump" the pip_window and the fullscreen (as we don't want store that value in gconf) and store 5. I decided to do it in this way for "future" output modes.
(In reply to comment #21) > (In reply to comment #19) > > What does this mean for directx? Does this means only that on Windows (the > > platform using directx) the main video is always shown? Does it break anything > > on Windows? > > It means that the windows port won't show the extended video streams. All the > request sent to videooutput-manager-dx to deal with it will be ignored. > > I don't thing it would break the windows port, but I need to verify it. > > > Testing directx can be done using > > http://wiki.ekiga.org/index.php/Building_Ekiga_for_Windows and executing on > > Windows. > > Ok. I'll give it try. It would really be useful. Even if you cannot be sure that it works, better to have an implementation with bugs, than no implementation.
(In reply to comment #23) > (In reply to comment #21) > > (In reply to comment #19) [snip] > It would really be useful. Even if you cannot be sure that it works, better to > have an implementation with bugs, than no implementation. mmmhh... Does ekiga/libpt work with wine? I don't think so, cause I got a crash with DirectX: Backtrace: =>0 0x00a046b0 PVideoInputDevice_DirectShow::IsCapturing+0x12(this=0x7924508) [/home/vjaquez/checkout/gnome2/ekiga/win32/ptlib/src/ptlib/msos/directshow.cxx:790] in libpt.2.10-beta9 (0x07c7dcf4) 1 0x00a04745 in libpt.2.10-beta9 (+0x14744) (0x07c7dd14) 2 0x00a05bc4 in libpt.2.10-beta9 (+0x15bc3) (0x07c7dd34) 3 0x00a05c64 ~PVideoInputDevice_DirectShow+0x17(this=0x7924508) [/home/vjaquez/checkout/gnome2/ekiga/win32/ptlib/src/ptlib/msos/directshow.cxx:475] in libpt.2.10-beta9 (0x07c7dd54) 4 0x00a05d2e ~PVideoInputDevice_DirectShow+0x11(this=0x7924508) [/home/vjaquez/checkout/gnome2/ekiga/win32/ptlib/src/ptlib/msos/directshow.cxx:476] in libpt.2.10-beta9 (0x07c7dd74) 5 0x009f5365 PVideoInputDevice::CreateOpenedDevice+0x54(driverName=0x7c7df84, deviceName=0x7c7df98, startImmediate=false, pluginMgr=(nil)) [/home/vjaquez/checkout/gnome2/ekiga/win32/ptlib/src/ptlib/common/videoio.cxx:1413] in libpt.2.10-beta9 (0x07c7ddc4) 6 0x708443d3 in libekiga (+0x843d2) (0x002c64d8) 7 0x00000000 (0x0787c744) 0x00a046b0 PVideoInputDevice_DirectShow::IsCapturing+0x12 [/home/vjaquez/checkout/gnome2/ekiga/win32/ptlib/src/ptlib/msos/directshow.cxx:790] in libpt.2.10-beta9: movl 0x0(%eax),%edx 790 CHECK_ERROR_RETURN(m_pMediaControl->GetState(0, &state));
Have never tried it :o( Only on Windows itself.
(In reply to comment #22) > (In reply to comment #20) > > (In reply to comment #11) > > > Created an attachment (id=223678) [details] [review] [details] [review] [details] [review] Ok, thank you. I replaced jump with skip.
I would like to know if you are working on Windows port or not, for this bug and the other. I started to commit patches from bug #687775 and it breaks compilation on Windows. As we plan to release ekiga 4.0.0 on Monday, and currently do not have much time to fix them, I think I have to revert them back for the release.
(In reply to comment #27) > I would like to know if you are working on Windows port or not, for this bug > and the other. I started to commit patches from bug #687775 and it breaks > compilation on Windows. As we plan to release ekiga 4.0.0 on Monday, and > currently do not have much time to fix them, I think I have to revert them back > for the release. I've tested these patches on Windows yesterday using VirtualBox and WinXP (I don't have a windows machine at hand). It doesn't break Windows, but I couldn't get video (attaching opal logs)
Created attachment 229037 [details] windows test logs
(In reply to comment #28) > I've tested these patches on Windows yesterday using VirtualBox and WinXP (I > don't have a windows machine at hand). > > It doesn't break Windows, but I couldn't get video (attaching opal logs) What patches do you speak about?
(In reply to comment #30) > (In reply to comment #28) > > I've tested these patches on Windows yesterday using VirtualBox and WinXP (I > > don't have a windows machine at hand). > > > > It doesn't break Windows, but I couldn't get video (attaching opal logs) > > What patches do you speak about? I meant, the current HEAD :)
This comes from: 2012/11/13 17:06:40.889 0:34.474 GMVideoOut...nager:1296 DirectX no Overlay Capabilities See https://mail.gnome.org/archives/ekiga-list/2012-November/msg00002.html and https://mail.gnome.org/archives/ekiga-list/2012-November/msg00005.html for more information, hope you find a solution.
(In reply to comment #32) > This comes from: > 2012/11/13 17:06:40.889 0:34.474 GMVideoOut...nager:1296 DirectX > no Overlay Capabilities > > See https://mail.gnome.org/archives/ekiga-list/2012-November/msg00002.html and > https://mail.gnome.org/archives/ekiga-list/2012-November/msg00005.html for more > information, hope you find a solution. I guess this is because I'm using a virtualbox thing... grrr... I could get a laptop and install in it windows and so on, but right now I can't afford time for that. Perhaps the next week or so...
I use too virtualbox and have a video problem... Ok, then I will revert back the commits from the other bug.
Victor, what is the difference between: - disable "Enable H.239 control" - and enable it and choose "Disable H.239 Extended Video" in the combobox ?
(In reply to comment #35) > Victor, what is the difference between: > - disable "Enable H.239 control" > - and enable it and choose "Disable H.239 Extended Video" in the combobox > ? Hi Eugen, If you enable the H.239, the H.239 protocol is negotiated between the endpoints, independently if the a second stream of video is requested or not. And, if the H.239 protocol is negotiated, the endpoint can reject the transmission of the extended video.
Ok, thanks, it confirms what I thought.