After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 683456 - Display extended remote video stream (H.323/H.239)
Display extended remote video stream (H.323/H.239)
Status: RESOLVED FIXED
Product: ekiga
Classification: Applications
Component: general
GIT master
Other Linux
: Normal normal
: ---
Assigned To: Ekiga maintainers
Ekiga maintainers
Depends on: 682489
Blocks: 681310
 
 
Reported: 2012-09-06 01:26 UTC by Víctor Manuel Jáquez Leal
Modified: 2013-03-04 08:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
common-videooutput: refactor frame_display_change_needed () (3.92 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
common-videooutput: store the extended stream buffer (4.49 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
common-videooutput: extended stream needs update (2.40 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
common-videooutput: add VO_MODE_REMOTE_EXT mode (4.52 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
none Details | Review
x-videooutput: refactor sync() (1.47 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
none Details | Review
x-videooutput: resources for the extended stream (7.20 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
none Details | Review
common-videooutput: extended stream in frame info (4.22 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
none Details | Review
videoutput: new parameter device_opened() signal (12.92 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
none Details | Review
common-videooutput: display extended stream (4.65 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
none Details | Review
ui: add the extended video option (6.02 KB, patch)
2012-09-06 01:30 UTC, Víctor Manuel Jáquez Leal
none Details | Review
ui: add the extended video option (6.02 KB, patch)
2012-09-06 17:47 UTC, Víctor Manuel Jáquez Leal
none Details | Review
common-videooutput: add VO_MODE_REMOTE_EXT mode (4.58 KB, patch)
2012-09-12 00:15 UTC, Víctor Manuel Jáquez Leal
none Details | Review
copyright assignation. (3.79 KB, patch)
2012-11-06 09:28 UTC, Víctor Manuel Jáquez Leal
none Details | Review
windows test logs (234.56 KB, text/plain)
2012-11-15 10:47 UTC, Víctor Manuel Jáquez Leal
  Details

Description Víctor Manuel Jáquez Leal 2012-09-06 01:26:34 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)
Comment 1 Víctor Manuel Jáquez Leal 2012-09-06 01:30:07 UTC
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.
Comment 2 Víctor Manuel Jáquez Leal 2012-09-06 01:30:09 UTC
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.
Comment 3 Víctor Manuel Jáquez Leal 2012-09-06 01:30:11 UTC
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.
Comment 4 Víctor Manuel Jáquez Leal 2012-09-06 01:30:14 UTC
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.
Comment 5 Víctor Manuel Jáquez Leal 2012-09-06 01:30:17 UTC
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.
Comment 6 Víctor Manuel Jáquez Leal 2012-09-06 01:30:19 UTC
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.
Comment 7 Víctor Manuel Jáquez Leal 2012-09-06 01:30:22 UTC
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.
Comment 8 Víctor Manuel Jáquez Leal 2012-09-06 01:30:24 UTC
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.
Comment 9 Víctor Manuel Jáquez Leal 2012-09-06 01:30:27 UTC
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.
Comment 10 Víctor Manuel Jáquez Leal 2012-09-06 01:30:30 UTC
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.
Comment 11 Víctor Manuel Jáquez Leal 2012-09-06 17:47:18 UTC
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.
Comment 12 Víctor Manuel Jáquez Leal 2012-09-12 00:15:11 UTC
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.
Comment 13 Eugen Dedu 2012-09-13 15:56:09 UTC
Patches from comments 1 to 3 are committed, thank you!  The others, a bit later.
Comment 14 Víctor Manuel Jáquez Leal 2012-09-14 12:59:55 UTC
(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 :)
Comment 15 Eugen Dedu 2012-09-16 19:08:50 UTC
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!
Comment 16 Damien Sandras 2012-09-30 15:29:41 UTC
Thanks for those patches!

Have you had the chance (or would you have the chance) to help porting that difficult part to GTK3?

Thanks
Comment 17 Víctor Manuel Jáquez Leal 2012-10-03 15:59:29 UTC
(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.
Comment 18 Víctor Manuel Jáquez Leal 2012-11-06 09:28:42 UTC
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.
Comment 19 Eugen Dedu 2012-11-07 14:00:19 UTC
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.
Comment 20 Eugen Dedu 2012-11-07 19:23:07 UTC
(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;
Comment 21 Víctor Manuel Jáquez Leal 2012-11-12 11:55:13 UTC
(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.
Comment 22 Víctor Manuel Jáquez Leal 2012-11-12 12:22:06 UTC
(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.
Comment 23 Eugen Dedu 2012-11-12 14:07:29 UTC
(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.
Comment 24 Víctor Manuel Jáquez Leal 2012-11-12 14:45:49 UTC
(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));
Comment 25 Eugen Dedu 2012-11-12 14:47:54 UTC
Have never tried it :o(  Only on Windows itself.
Comment 26 Eugen Dedu 2012-11-12 16:07:48 UTC
(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.
Comment 27 Eugen Dedu 2012-11-15 09:28:13 UTC
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.
Comment 28 Víctor Manuel Jáquez Leal 2012-11-15 10:46:38 UTC
(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)
Comment 29 Víctor Manuel Jáquez Leal 2012-11-15 10:47:34 UTC
Created attachment 229037 [details]
windows test logs
Comment 30 Eugen Dedu 2012-11-15 12:59:58 UTC
(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?
Comment 31 Víctor Manuel Jáquez Leal 2012-11-15 15:57:29 UTC
(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 :)
Comment 32 Eugen Dedu 2012-11-15 16:26:41 UTC
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.
Comment 33 Víctor Manuel Jáquez Leal 2012-11-15 17:03:45 UTC
(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...
Comment 34 Eugen Dedu 2012-11-15 18:19:18 UTC
I use too virtualbox and have a video problem...

Ok, then I will revert back the commits from the other bug.
Comment 35 Eugen Dedu 2013-03-03 17:12:53 UTC
Victor, what is the difference between:
- disable "Enable H.239 control"
- and enable it and choose "Disable H.239 Extended Video" in the combobox
?
Comment 36 Víctor Manuel Jáquez Leal 2013-03-04 08:55:49 UTC
(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.
Comment 37 Eugen Dedu 2013-03-04 08:57:29 UTC
Ok, thanks, it confirms what I thought.