Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support H.235 for H.239 #8

Closed
nguyen-viet-hung opened this issue Aug 29, 2018 · 26 comments
Closed

Add support H.235 for H.239 #8

nguyen-viet-hung opened this issue Aug 29, 2018 · 26 comments
Assignees

Comments

@nguyen-viet-hung
Copy link

I don't have permission to commit so I write it here. I have patched for supporting H.235 for H.239. Please review it. I renamed to mypatch.txt cause this not support "patch" extension
mypatch.txt

@willamowius
Copy link
Owner

willamowius commented Aug 29, 2018

When I apply your patch H.264 and H.261 stop working and thus H.239, too. Did you test with the current master branch ?

@nguyen-viet-hung
Copy link
Author

I tested with master branch with Polycom PVX 8.4.0

@willamowius
Copy link
Owner

And you tested with the H323Plus provided video plugins or did you use your own video codec ?

@nguyen-viet-hung
Copy link
Author

I tested with my own video codec

@willamowius
Copy link
Owner

Could you please try with the H.261 and H.264 plugins from H323Plus, too ? If your patch breaks those, I can't apply it and it seems like it does.

@nguyen-viet-hung
Copy link
Author

I will test with video plugins and fix if it has issue.

@willamowius
Copy link
Owner

willamowius commented Aug 29, 2018

Maybe its not the plugings, but with master branch when I call a PVX with simph323 and press S during the call the PVX receives a H.239 stream. But with your patch the H.239 channel doesn't start. Same with callgen323.

@nguyen-viet-hung
Copy link
Author

Currently I can test with H.261 plugin and it doesn't break at all. I attached here with the log trace. For H.264 plugin, I failed to configure although it can detect LIBAVCODEC and X264. I checked the configure and saw it had failed to test h264_decoder function. How can you configure it for H.264? I tested with master branch and also attached the configuration log file.
result.zip

@nguyen-viet-hung
Copy link
Author

It seems I found issue. I will fix it and send the patch again

@nguyen-viet-hung
Copy link
Author

After inspecting the TCS from PVX, it doesn't have H.239 (H.264) capability, so we cannot start H.239 transmitting channel. I do not know why PVX can open H.239 (H.264) with my program. I will check with another version of PVX. So at this time, I find no issue.

@nguyen-viet-hung
Copy link
Author

nguyen-viet-hung commented Aug 30, 2018

I found the issue that cause break when open H.239 channel for transmitting. It cause wrong type cast of Capability. I attach again the patch. Please review it. You can check with RealPresence Desktop.
mypatch.txt

@willamowius
Copy link
Owner

Thanks, but for me sending H.239 is still broken.

I'm testing with RealPresence Desktop and simph323 -v FakeVideo -n -P H.264 but when i press S to start the presentation I see a PresentationTokenResponse (funtionNotUnderstood) from simph323 in the Wireshark trace and no content in RealPresence.

@nguyen-viet-hung
Copy link
Author

I faced this problem before. It cause the video capability for H.239 is not same as RealPresence's. if I change my H.264 cap. it will lead to fail to open H.239 channel

@willamowius
Copy link
Owner

willamowius commented Aug 30, 2018

Not sure what exactly you mean with your last comment, but (unencrypted) H.239 from H323Plus to RealPresence used to work fine before your patch and doesn't now. So I guess we have keep debugging.

@nguyen-viet-hung
Copy link
Author

I think the error functionNotUnderstood is because of not open ExtVideoChannel before. I check in H323Connection::OpenH239Channel(), the ctrl just send GenericRequest. If you don't open ExtVideoChannel, how can far-end understand because, by default, we do not start ExtVideoChannel? In my test. If I disable encryption, I still got this error. Please check it

@willamowius
Copy link
Owner

Yes, with your patch it also occurs without encryption, but it was working fine before your patch.

@nguyen-viet-hung
Copy link
Author

nguyen-viet-hung commented Aug 31, 2018

with my test, after change autoStartTransmitExtVideo = true. I can share the desktop. Please take a look to my log from simple. Below is the output from simple.

Enabled Media Encryption AES128
Waiting for incoming calls for "devel"
Press X to exit.
H323> TLS NOT enabled for call.
Automatically accepting call.
TLS NOT enabled for call.
Media Encryption 1 IsTransmitter AES128
Started logical channel: sending G.711-ALaw-64k # <25>
Media Encryption 2 IsTransmitter AES128
Started logical channel: sending H.261-CIF{sw} # <37>
Started logical channel: sending H.239(H.264{sw} <1>) # <40>
In call with ip$172.20.73.113:3234
Media Encryption 1 IsReceiver AES128
Started logical channel: receiving G.722-64k{sw} # <3>
Media Encryption 2 IsReceiver AES128
Started logical channel: receiving H.264-CIF{sw} # <30>
Started logical channel: receiving H.239(H.264{sw} <1>) # <31>
s
H.239 session open..
H323> h
H323> Call with "ip$172.20.73.113:3234" completed, duration 0:44

trace.txt

@nguyen-viet-hung
Copy link
Author

nguyen-viet-hung commented Aug 31, 2018

I found an issue in CreateRealtimeLogicalChannel. So I send the patch again. Already tested with RealPresence Desktop with H.264 plugin by simple. All work fine. You can refer to trace.txt for log from simple and the screen shot.
centos-2018-08-31 23-08-15
realpresencedesktop
mypatch.txt
trace.txt

@willamowius
Copy link
Owner

With autoStartTransmitExtVideo=true your patch works great. I have commited it. Thanks!

But why do we need this set to true now where it worked fine without before ? I'm a little worried that many people have to change their applications now to keep unencrypted H.239 going like before and we really should avoid breaking old code.

Any chance you find a way to make wit work without autoStartTransmitExtVideo ? Until now I though this switch was only needed to offer FastSTart for H.239.

@nguyen-viet-hung
Copy link
Author

I think we should change H323Connection::OpenH239Channel, detect if channel for transmitting ext video is open first. I am sure if you remove my patch, set autoStartTransmitExtVideo to false, when you request sharing, it will fail because it doesn't have channel for sending data. By the way, I succeeded patch H.264 plugin using modern FFMPEG but still fail in configure script. When I find the solution to detect H.264 enabled in FFMPEG, I will request commit again

@willamowius
Copy link
Owner

Sorry, but without your patch H.239 in Simple and callgen323 worked fine with autoStartTransmitExtVideo = false and thats what we need to get back.

An updated H.264 plugin would be great. Please make it a separate pull request.

@nguyen-viet-hung
Copy link
Author

I will test without patch to see how it run and then take it back with the patch

@nguyen-viet-hung
Copy link
Author

I found the issue. It comes from H323Connectopn::OpenExtendedVideoSession. I fixed it for H.235 case and add prevent re-open extended video channel if if is automatically started. All ready tested with RealPresence Desktop. I attach new patch for master branch. Please check it.
mypatch.txt

@willamowius
Copy link
Owner

Yes, that fixed the issue. Patch applied. Thanks!

H.239 connections to a Polycom m100 still don't work. This was also the case before your patch, but do you have any idea why the m100 is different ? Other endpoints like PVX or RealPresence can send H.239 to the m100 fine and I don't see any difference to H323Plus in the trace.

m100 Trial: https://support.polycom.com/content/support/north-america/usa/en/support/video/personal-telepresence/m100.html

@nguyen-viet-hung
Copy link
Author

I will check and see what happen

@nguyen-viet-hung
Copy link
Author

nguyen-viet-hung commented Sep 4, 2018

I tested and found something as follow

  • If autoStartTransmitExtVideo = false; m100 cannot receive H.239 data
  • If autoStartTransmitExtVideo = true; m100 can receive H.239 data for the first times, from second times, m100 cannot receive H.239 data.

So I think, to m100, H.239 channel need be started before we send the generic request. I modify the source to ensure send the generic request after H.239 channel started (we recevied ACK PDU) and test, now m100 can receive H.239 data for all time. I attach here the patch for master branch for test purpose only. If you find it usefull, please think of the architure before make any patch.
m100patch.txt
trace.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants