xrdp icon indicating copy to clipboard operation
xrdp copied to clipboard

Implement VideoToolbox-powered H.264 encoder for macOS

Open unstabler opened this issue 2 years ago • 8 comments

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

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

unstabler avatar May 14 '22 08:05 unstabler

@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.

Nexarian avatar May 14 '22 14:05 Nexarian

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.

matt335672 avatar May 16 '22 10:05 matt335672

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

unstabler avatar Mar 04 '23 11:03 unstabler

@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.

matt335672 avatar Mar 06 '23 10:03 matt335672

@matt335672

I was also working on this part: https://github.com/team-unstablers/xrdp-tumod/pull/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 https://github.com/team-unstablers/xrdp-tumod/pull/1 , I will post a PR on xrdp as well.

unstabler avatar Mar 14 '23 08:03 unstabler

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.

matt335672 avatar Mar 15 '23 08:03 matt335672

@matt335672

Then, if you don't mind, I would like to request a partial review of https://github.com/team-unstablers/xrdp-tumod/pull/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!)

unstabler avatar Mar 15 '23 21:03 unstabler

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.

matt335672 avatar Mar 16 '23 09:03 matt335672