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

WebCodecs H.264 #1876

Merged
merged 2 commits into from
Aug 29, 2024
Merged

WebCodecs H.264 #1876

merged 2 commits into from
Aug 29, 2024

Conversation

any1
Copy link
Contributor

@any1 any1 commented Jul 8, 2024

This adds the Open H.264 encoding to noVNC, as defined here: https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding

This was tested on the following browsers:

  • Firefox 127.0.2 with dom.media.webcodecs.enabled=true
  • Firefox Nightly 129.0a1 (2024-07-08)
  • Chromium 126.0.6478.126

I haven't run into any issues with Chromium, but Firefox keeps killing the decoder after a few frames because it thinks that the hardware decoder is too slow, so it wants to switch to software. However, after the switch, a new key frame is needed, so the decoder goes into a bad state and a reconfiguration is required, so this sequence of events is repeated indefinitely.

We can work around Firefox's issue by setting a frame duration for each frame, but that would be a hack. We don't know the frame duration for sure, and it is an optional parameter so Firefox should not rely on it.

I created an issue in bugzilla here: https://bugzilla.mozilla.org/show_bug.cgi?id=1906761

On the server side, I tested this against wayvnc.

WLR_BACKENDS=headless sway &
WAYLAND_DISPLAY=wayland-2 wayvnc -g

WayVNC can also run as a WebSocket server using the -w flag.

@any1 any1 marked this pull request as ready for review July 8, 2024 23:14
Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

We try to make sure all features in noVNC work on all current browsers, so it's annoying that Firefox isn't ready yet. :/
Is the new API the only method? Have you looked at using the older WebRTC API?

Also, we would appreciate it if you could have a look at unit tests for the new code. It would help prevent bit rot in this new decoder.

core/display.js Show resolved Hide resolved
core/display.js Outdated Show resolved Hide resolved
core/rfb.js Show resolved Hide resolved
core/rfb.js Show resolved Hide resolved
core/decoders/h264.js Outdated Show resolved Hide resolved
core/rfb.js Outdated Show resolved Hide resolved
@any1
Copy link
Contributor Author

any1 commented Jul 24, 2024

We try to make sure all features in noVNC work on all current browsers, so it's annoying that Firefox isn't ready yet. :/ Is the new API the only method? Have you looked at using the older WebRTC API?

WebRTC is not a good fit for this as it does not expose an API for decoding video at the required level of abstraction.

@CendioOssman
Copy link
Member

The WebCodecs definitely looks like a better fit. No arguments there. But if WebCodecs isn't fully supported yet, the question is if it's possible to build a more complex solution around WebRTC that works on more browsers.

@any1
Copy link
Contributor Author

any1 commented Jul 24, 2024

The WebCodecs definitely looks like a better fit. No arguments there. But if WebCodecs isn't fully supported yet, the question is if it's possible to build a more complex solution around WebRTC that works on more browsers.

I suppose that it might be possible to create two peers within the browser, connect them together and stream video from one to the other which would render into an offscreen video element that gets duplicated onto the canvas. I have not explored this option. It's just hypothetical. It would be very hacky.

In any case, I don't think that this would be worth the effort as WebCodec support is very close to being complete in Firefox.

@CendioOssman
Copy link
Member

That does sound very complex. I was hoping it was a smaller step, like having to frame the data better, or some extra steps to get the data out (like creating some <video> element). Let's hope Firefox is ready soon then.

@any1
Copy link
Contributor Author

any1 commented Aug 4, 2024

The requested changes have been made.

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks!

Did you check if it was possible to generate some data that could be used for unit tests? Like we have for JPEG and PNG rects.

@any1
Copy link
Contributor Author

any1 commented Aug 12, 2024

I added a couple of basic tests for H264Parser and H264Context, but I intend to add more. I hope it's OK to export those. I don't see any other way to test them.

I hardcoded the colour space because it seems that Firefox and Chrome have different defaults. Colour spaces were never specified for the Open H.264 extension as far as I can see. The protocol is missing a way to tell the client what colour space was used for encoding the video. Maybe we can decide a colour space and just say, in the protocol, that the video should always be encoded using that?

@CendioOssman
Copy link
Member

I added a couple of basic tests for H264Parser and H264Context, but I intend to add more. I hope it's OK to export those. I don't see any other way to test them.

They should not require explicit testing, as they are not part of the interface. Can't we feed the data to H264Decoder? That's the interface we want a stable behaviour out of.

I hardcoded the colour space because it seems that Firefox and Chrome have different defaults. Colour spaces were never specified for the Open H.264 extension as far as I can see. The protocol is missing a way to tell the client what colour space was used for encoding the video. Maybe we can decide a colour space and just say, in the protocol, that the video should always be encoded using that?

VNC doesn't explicitly define color space anywhere. But given its nature, sRGB would be the base assumption.

@CendioOssman
Copy link
Member

VNC doesn't explicitly define color space anywhere. But given its nature, sRGB would be the base assumption.

That is, assuming the data stream doesn't specify an explicit colour space. In which case, I'd say anything goes. It's up to the client at that point to match things up.

@any1
Copy link
Contributor Author

any1 commented Aug 12, 2024

VNC doesn't explicitly define color space anywhere. But given its nature, sRGB would be the base assumption.

That is, assuming the data stream doesn't specify an explicit colour space. In which case, I'd say anything goes. It's up to the client at that point to match things up.

Well, it's YCbCr encoded, so I don't think sRGB applies.

@any1
Copy link
Contributor Author

any1 commented Aug 12, 2024

I added a couple of basic tests for H264Parser and H264Context, but I intend to add more. I hope it's OK to export those. I don't see any other way to test them.

They should not require explicit testing, as they are not part of the interface. Can't we feed the data to H264Decoder? That's the interface we want a stable behaviour out of.

Sure, we can do that too, but we get more thorough test coverage by testing the individual components.

If you see no value in test coverage and just want the end-to-end test, I can throw them out. It's up to you.

@CendioOssman
Copy link
Member

I don't think we have any plans of reusing those components elsewhere, so it's quite sufficient to test only that which we might see from an actual VNC server.

@any1
Copy link
Contributor Author

any1 commented Aug 12, 2024

I don't think we have any plans of reusing those components elsewhere, so it's quite sufficient to test only that which we might see from an actual VNC server.

I think that we have very different ideas about what unit tests are supposed to achieve, but it's up to you.

This adds an H.264 decoder based on WebCodecs.
@any1 any1 force-pushed the webcodec-h264 branch 2 times, most recently from 1813055 to fe455a1 Compare August 18, 2024 14:10
@any1
Copy link
Contributor Author

any1 commented Aug 18, 2024

I've added some more tests and I've fixed some bugs that I found when writing them.

Generating good test data was actually a bit of a headache and I was unable to get ffmpeg to do colour conversions correctly so I ended up writing a C program to do it using libavcodec and libswscale. I looked deeper into colour spaces along the way and did my best to set them up correctly in other projects.

I wrote some lower level unit tests to help me figure out some things and I've included them, but I can throw them out if you don't want them. They did help me to find some bugs and to understand what was going wrong with the higher level tests.

The tests don't run on all the CIs, due to lack of WebCodecs support. Do you know if a test file can be skipped depending on whether a feature exists or not?

@CendioOssman
Copy link
Member

The tests don't run on all the CIs, due to lack of WebCodecs support. Do you know if a test file can be skipped depending on whether a feature exists or not?

Yes, you can do runtime checks and skip tests. An example here:

this.skip();

@any1 any1 force-pushed the webcodec-h264 branch 2 times, most recently from 5099bbb to 55344dd Compare August 19, 2024 21:26
@any1
Copy link
Contributor Author

any1 commented Aug 19, 2024

Thanks, the test skipping works.

Now it appears that I've exposed a bug in Safari. It doesn't scale from limited range up to full range like it should when rendering the frame to canvas. How should that be handled?

@any1
Copy link
Contributor Author

any1 commented Aug 19, 2024

Well, I managed to make the tests pass on all browsers by carefully crafting the test data. It's now BT.709 full range.

Ideally, the browsers should get this right, but I can't be chasing down bugs in all the browsers.

@any1
Copy link
Contributor Author

any1 commented Aug 29, 2024

Ping?

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for your hard work!

@CendioOssman CendioOssman merged commit 047531e into novnc:master Aug 29, 2024
11 checks passed
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.

2 participants