-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix H264 hardware video decoding failure #2042
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ export default class Display { | |
|
|
||
| this._renderQ = []; // queue drawing actions for in-order rendering | ||
| this._flushPromise = null; | ||
| this._pendingFrames = []; // video frames awaiting decoder output | ||
|
|
||
| // the full frame buffer (logical canvas) size | ||
| this._fbWidth = 0; | ||
|
|
@@ -479,6 +480,22 @@ export default class Display { | |
| } | ||
| } | ||
|
|
||
| _drawVideoFrame(pendingFrame, rect) { | ||
| let frame = pendingFrame.frame; | ||
| if (!frame) { | ||
| return; | ||
| } | ||
| if (frame.codedWidth < rect.width || frame.codedHeight < rect.height) { | ||
| Log.Warn("Decoded video frame does not cover its full rectangle area. Expecting at least " + | ||
| rect.width + "x" + rect.height + " but got " + | ||
| frame.codedWidth + "x" + frame.codedHeight); | ||
| } | ||
| this.drawImage(frame, | ||
| 0, 0, rect.width, rect.height, | ||
| rect.x, rect.y, rect.width, rect.height); | ||
| frame.close(); | ||
| } | ||
|
|
||
| _renderQPush(action) { | ||
| this._renderQ.push(action); | ||
| if (this._renderQ.length === 1) { | ||
|
|
@@ -501,7 +518,21 @@ export default class Display { | |
| const a = this._renderQ[0]; | ||
| switch (a.type) { | ||
| case 'flip': | ||
| this.flip(true); | ||
| if (this._pendingFrames.length > 0) { | ||
| // Wait for all pending video frames to be | ||
| // decoded before flipping, so they are | ||
| // visible on screen. | ||
| let display = this; | ||
| let frames = this._pendingFrames; | ||
| this._pendingFrames = []; | ||
| Promise.all(frames.map(f => f.promise)).then(() => { | ||
| display.flip(true); | ||
| display._scanRenderQ(); | ||
| }); | ||
| ready = false; | ||
| } else { | ||
| this.flip(true); | ||
| } | ||
| break; | ||
| case 'copy': | ||
| this.copyImage(a.oldX, a.oldY, a.x, a.y, a.width, a.height, true); | ||
|
|
@@ -533,32 +564,20 @@ export default class Display { | |
| } | ||
| break; | ||
| case 'frame': | ||
| if (a.frame.ready) { | ||
| // The encoded frame may be larger than the rect due to | ||
| // limitations of the encoder, so we need to crop the | ||
| // frame. | ||
| let frame = a.frame.frame; | ||
| if (frame.codedWidth < a.width || frame.codedHeight < a.height) { | ||
| Log.Warn("Decoded video frame does not cover its full rectangle area. Expecting at least " + | ||
| a.width + "x" + a.height + " but got " + | ||
| frame.codedWidth + "x" + frame.codedHeight); | ||
| } | ||
| const sx = 0; | ||
| const sy = 0; | ||
| const sw = a.width; | ||
| const sh = a.height; | ||
| const dx = a.x; | ||
| const dy = a.y; | ||
| const dw = sw; | ||
| const dh = sh; | ||
| this.drawImage(frame, sx, sy, sw, sh, dx, dy, dw, dh); | ||
| frame.close(); | ||
| } else { | ||
| if (!a.frame.ready) { | ||
| // Don't block the queue — the video decoder | ||
| // pipeline needs continued input to produce | ||
| // output. Register a callback to draw later, | ||
| // and let the queue keep feeding the decoder. | ||
|
Comment on lines
+567
to
+571
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This gives us tearing, so I'm afraid it's not an approach we want to use. B-frames should ideally be rare when used with VNC, as they will never be rendered and hence pointless to waste resources on. But I can accept that you might not always have that control and we need to be prepared to deal with them. Ideally, we queue them in the decoder rather than the display. Hopefully, we can detect these there?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify, this issue is not caused by B-frames. The stream I tested uses H264 High profile with only IDR and P-frames, yet Intel's hardware decoder still doesn't output frames immediately. It buffers a few frames of input before producing any output. It seems is Intel-specific; NVIDIA and AMD GPUs output frames without delay. Since Intel iGPUs are very common (laptops, office machines), I think it's worth handling. I agree that handling this in the display layer is not ideal. I'll look into addressing it on the decoder side (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be a violation of the protocol, and could cause all kinds of weird rendering effects. Frames need to be displayed immediately after they are received. It's incorrect to buffer them and display them later. So we need to find some way to get the Intel decoder to spit out frames right away. Perhaps there is some "low latency mode"? Failing that, we'd need to blacklist it. Can we detect it and fall back to software decoding?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've looked into what's available from the WebCodecs API:
As far as I can tell, there's no other way from JavaScript to change Intel's buffering behavior. The only practical option is detection and fallback to software decoding via For detection, we can query the GPU vendor via WebGL's
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to spec, constrained baseline profile is prescribed, so B frames are explicitly not allowed. |
||
| let display = this; | ||
| a.frame.promise.then(() => { | ||
| display._scanRenderQ(); | ||
| let pendingFrame = a.frame; | ||
| let rect = { x: a.x, y: a.y, width: a.width, height: a.height }; | ||
| pendingFrame.promise.then(() => { | ||
| display._drawVideoFrame(pendingFrame, rect); | ||
| }); | ||
| ready = false; | ||
| this._pendingFrames.push(pendingFrame); | ||
| } else { | ||
| this._drawVideoFrame(a.frame, a); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit embarrassing that this was overlooked.
It suggests that we are lacking one or more unit tests for this code. Is it something you could have a look at?