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

Implement VideoToolbox-powered H.264 encoder for macOS #2263

Open
wants to merge 13 commits into
base: devel
Choose a base branch
from

Conversation

unstabler
Copy link

Changes / Notes

  • This PR is based / depends on Nexarian/xrdp/mainline_merge

  • This PR adds H.264/yuv420 support for macOS.

  • This PR contains Obj-C codes; these codes may not meet coding convention of this repository. (I thought it had to be written in Objective-C, but it wasn't.)

  • Initialization of VTCompressionSession will fail if fork=true in xrdp.ini

TODO

  • add support for dynamic-sized drects
  • set kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder to true
  • (if possible) rewrite codes in C

Nexarian and others added 13 commits May 13, 2022 22:50
- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor
- Tested with xorgxrdp
- Tested with vnc
- Only works with single monitor.
- Update documentation to clarify the difference between MSTSC and
Microsoft Remote Desktop.
- Compatible with NVENC and xorgxrdp_helper
- Compatible with Nvidia.
- Updates to include ms-rdpedisp.h header for the 2.2.2 specification of
the protocol.
- State machine for resizing.
- Mechanisms to make sure the key frame is always sent on resize.
- Adds new struct that shares the number of monitors with xrdp_client_info.h
- Modification to the original resize setup that works with /gfx.
- YAMI compat.
- Updating to librfxcodec branch that is also a merge from mainline.
- Made sure RFX progressive works.

Depends on neutrinolabs/xorgxrdp#183
@Nexarian
Copy link
Contributor

@jsorg71 and I are working on how to merge in everything into devel, but I'm afraid it might be a while. There's a lot to test and make sure is right. I suggest taking the pieces from mainline merge and working with us to build a plan on what should go first, and what can wait.

@matt335672
Copy link
Member

I agree with @Nexarian's comment above. I'm happy to contribute to the review when you think the time is right. In the meantime, the CI pipeline should at least now be working on this PR.

@unstabler
Copy link
Author

Hi Gyuhwan,

You wrote:

DESCRIPTION OF PROBLEM
Hello, we are implementing macOS support for an open source remote desktop project called xrdp.

We are implementing a feature to compress image data with h.264 codec to save bandwidth, but when the daemon process fork(), VTCompressionSessionCreate() fails to initialize and returns -12903( kVTInvalidSessionErr).

If VTCompressionSessionCreate() is called without fork(), initialization succeeds normally.

Is there something we are doing wrong?

(+ I heard before that there is a problem that fork() breaks the Objective-C runtime library (Foundation?), is the VideoToolbox code written in plain C also affected?)

STEPS TO REPRODUCE

  • call fork()
  • call VTCompressionSessionCreate() from child process

It seems that you’re calling fork without calling exec*. Is that right?

If so, that’s not supported on macOS. Many Apple frameworks use Mach messaging to communicate with helper processes, for example, launchd daemons and agents. Mach messaging and fork don’t play well together. The child inherits the parent’s memory but it does not inherit the parent’s Mach port namespace. So the child thinks it has access to a bunch of Mach services but it doesn’t.

Many of the most-commonly used frameworks, like Foundation, have asserts that detect this situation. It seems that VideoToolbox is reporting an error in this case.

In short, if you call fork without calling exec* you will run into problems. If you’re porting Unix-y code that relies on this then you may get away with this, but not if you use any of Apple’s frameworks.

If you're spinning up a new process for some VideoToolbox work, most folks use threads for this sort thing.

Apple Developer Technical Support

@matt335672
Copy link
Member

@unstabler - one way to solve this might be to re-exec xrdp after the xrdp fork() call, but ensure the child gets the file descriptor from the accept().

I'm currently working on a branch where I've implemented some of what we'd need for this. If you're interested, let me know and I'll open a feature request where I can add more detail.

@unstabler
Copy link
Author

unstabler commented Mar 14, 2023

@matt335672

I was also working on this part: team-unstablers#1 .

However, I was afraid that my work did not fit the overall direction of this project, so I forked it under the name xrdp-tumod.

If I complete team-unstablers#1 , I will post a PR on xrdp as well.

@matt335672
Copy link
Member

Thanks for the update.

I'm currently looking at splitting sesman into two executables to fix some long-standing problems. I've got to solve some of the same problems you have to get it working securely.

If you want me to review anything, let me know. We probably don't want these to drift too far apart.

@unstabler
Copy link
Author

@matt335672

Then, if you don't mind, I would like to request a partial review of team-unstablers#1, although the work is not that advanced yet. (+ can i post / move this PR to here?)

Also, I need to reconstruct the struct trans in the child process, should I pass the contents of the fields needed for reconstruction to the child process through args[] (or piping) and assign them directly in the child process?

(sorry for repost!)

@matt335672
Copy link
Member

To reconstruct a struct trans, I suggest you pass just the file descriptor over, and then just set up a new one in the child.

You can either leave the file descriptor open and pass the number over, or use libipm to pass the file descriptor directly. The first is easier to set up, but the second lets you close the file descriptor safely in the parent if that's what you want to do, without a race condition.

I've got examples of both in my split_session_driver branch, but be aware this is changing quite a lot at the moment.

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

Successfully merging this pull request may close these issues.

3 participants