xpra icon
Bug tracker and wiki

Opened 3 years ago

Closed 12 months ago

#800 closed enhancement (fixed)

allow B frames with video encoders

Reported by: Antoine Martin Owned by: Antoine Martin
Priority: minor Milestone: 1.0
Component: encodings Version: trunk
Keywords: Cc:

Description

  • add client encoding capability flag (and make it a number specifying the max ref frames? will only support 1 for now?)
  • video encoders take this into account (ie: NVENC4 #796)
  • window video source needs to know about this and schedule a new frame refresh after a timeout to ensure we push the last frame out one way or another
  • client decoding code needs to ignore empty frames (B frames need the next frame to arrive first), and be able to paint more than one frame from a single buffer push..

Attachments (6)

delayed-frames.patch (11.9 KB) - added by Antoine Martin 2 years ago.
work in progress to support b frames
2016-06-24 11-41-13.mp4 (3.6 MB) - added by J. Max Mena 15 months ago.
Recorded with OBS - scrolling and switching tabs in Firefox
2016-06-28 09-15-18.mp4 (3.8 MB) - added by J. Max Mena 15 months ago.
Scrolling and collapsing images on Reddit - notice the flicker at the end.
800-bframes.log (26.4 KB) - added by Antoine Martin 13 months ago.
with b-frames
800-nobframes.log (28.7 KB) - added by Antoine Martin 13 months ago.
without b-frames
800-regionrefresh-mouse.log (7.4 KB) - added by Antoine Martin 13 months ago.
regionrefresh and mouse debug

Change History (33)

Changed 2 years ago by Antoine Martin

Attachment: delayed-frames.patch added

work in progress to support b frames

comment:1 Changed 2 years ago by Antoine Martin

Owner: changed from Antoine Martin to Antoine Martin
Status: newassigned

Code to flush the encoder:

while( x264_encoder_delayed_frames( h ) )
{
    i_frame_size = Encode_frame( h, opt->hout, NULL );
    if( i_frame_size < 0 )
        return -1;
    i_file += i_frame_size;
}

What we want is to keep at most N frames in the encoder at any point in time.
Where N is very low without av-sync (maybe just 1), or derived from the av-sync value.
We will need a timer to flush things if no new frames come in, and this belongs in the window source logic.

comment:2 Changed 23 months ago by Antoine Martin

Milestone: 0.160.17

re-scheduling

comment:3 Changed 18 months ago by Antoine Martin

Milestone: 0.170.18

comment:4 Changed 15 months ago by Antoine Martin

For NVENC, we will have to ensure we don't exceed NV_ENC_CAPS_NUM_MAX_BFRAMES.

From the headers for NV_ENC_CONFIG.frameIntervalP: Specifies the GOP pattern as follows: frameIntervalP = 0: I, 1: IPP, 2: IBP, 3: IBBP If goplength is set to NVENC_INFINITE_GOPLENGTH frameIntervalP should be set to 1.

Last edited 15 months ago by Antoine Martin (previous) (diff)

comment:5 Changed 15 months ago by Antoine Martin

Done for x264 in r12858.

Still todo:

  • verify the bandwidth savings / performance
  • take delayed frames into account for av-sync
  • we don't record encoding statistics when flushing (minor)
  • we call "fire_paint_callbacks" for the packet we just received, which may not be different from the frame that comes out of the decoder: the data we collect is no longer directly related to this frame, and when dealing with delayed frames with no data we don't take into account the time it takes to paint.. since there isn't anything to paint
  • implement for other encoders: vpx, nvenc..

Somewhat related: Explanation of x264 tune, maybe we could tune to "fastdecode" when we find the client is struggling with the decode speed. At the moment we just increase the batch delay which may lower the speed and increase the complexity for the decoding.. making things worse.

comment:6 Changed 15 months ago by Antoine Martin

Owner: changed from Antoine Martin to alas
Status: assignednew
  • not going to bother with NVENC for now (too hard), moved to #1235
  • vp8 does not have b-frames: Improving Prediction without B Frames: The lack of B frames in VP8 has sparked some discussion.. and we don't care about vp9 (too slow), and xvid is only useful for testing
  • not going to fix the missing statistics either: we record when things get sent out... and in the case of delayed frames, that complicates things too much - so we just record when things do get sent, without caring if the actual frame being sent corresponds to the timestamp of the request, for all intents and purposes this is good enough
  • not going to change the "fire_paint_callbacks" code for now either, for similar reasons: it's close enough and changing it might mess up the heuristics
  • r12868 takes delayed frames into account when calculating the av-sync delay, which we now also re-calculate when the batch delay changes, looks like this with "-d av-sync":
    update_av_sync_frame_delay() video encoder=x264_encoder(BGRX - 300x300), delayed frames=1, frame delay=49
    
  • r12869 makes it possible to disable b-frames with XPRA_B_FRAMES=0 on the server, r12890 adds the env var to the client

@afarr: ready for testing.
b-frames should compress better, and make better use of the delay used for av-sync.
Verifying that we compress better is hard because of the usual problems: "define better". So many heuristics are intertwined that we could get higher fps, lower bandwidth, higher quality, etc...

What you can check though:

  • xpra info now exposes what types of frames were encoded with x264:
    $ xpra info  | grep frame-types
    window.2.encoder.frame-types.B=73
    window.2.encoder.frame-types.IDR=1
    window.2.encoder.frame-types.P=74
    
  • "-d x264" on the server shows what types or frames are being used:
    x264 encode frame 2733 as    B slice with 4 nals, total    6148 bytes, keyframe=0, delayed=1
    x264 encode frame 2734 as    P slice with 4 nals, total    9670 bytes, keyframe=0, delayed=1
    
  • "-d paint" on the client shows (amongst many other things...):
    draw_region(0, 0, 300, 300, h264, 7057 bytes, 0, \
       {'speed': 56, 'type': 'B', 'pts': 3579, 'frame': 350, 'quality': 98, 'delayed': 1, 'csc': 'YUV444P'}, \
        [<function record_decode_time at 0x7f4df4073e60>, <function after_draw_refresh at 0x7f4df4073ed8>])
    
Last edited 15 months ago by Antoine Martin (previous) (diff)

comment:7 Changed 15 months ago by J. Max Mena

Owner: changed from alas to Antoine Martin

With a trunk r12899 server and client (both Fedora 23, connected over TCP), I'm getting some really really weird painting when I enable B-Frames. I'll attach a quick screenrecord.

That being said, with B-Frames enabled video appears to be better than with B-Frames disabled. That's just my subjective opinion, we'll need some kind of performance tests edit: as mentioned in comment:5.

Last edited 15 months ago by J. Max Mena (previous) (diff)

Changed 15 months ago by J. Max Mena

Attachment: 2016-06-24 11-41-13.mp4 added

Recorded with OBS - scrolling and switching tabs in Firefox

comment:8 Changed 15 months ago by Antoine Martin

Owner: changed from Antoine Martin to J. Max Mena

I believe that what you are recording is caused by the delayed video frames used for the whole browser window: those arrive out of order, after the other updates.

  • r12931 should fix this by only enabling b-frames for video regions.
  • r12941 also sends a frame in place of the initial buffered video frame (using whatever non-video encoding is available) and r12942 makes it prefer jpeg

Does that improve things?

comment:9 Changed 15 months ago by J. Max Mena

Owner: changed from J. Max Mena to Antoine Martin

Upped client and server to r12942:

Somewhat. I don't get the full repaints on switching tabs, but I do still see some painting issues that I only get with B-Frames enabled. The easiest to show is collapsing and uncollapsing images on Reddit with RES enabled in Firefox. I'll attach a screenrecord to demonstrate.

Changed 15 months ago by J. Max Mena

Attachment: 2016-06-28 09-15-18.mp4 added

Scrolling and collapsing images on Reddit - notice the flicker at the end.

comment:10 Changed 15 months ago by Antoine Martin

Support for b-frames was included in the enc_ffmpeg codec, see ticket:1107#comment:10.

The issue shown on the video above is likely due to the fact that we mix video and non-video encodings on the page. The video gets delayed, but the non-video is not. This is a similar issue to #1218.
The best solution is probably to only use b-frames for video regions, which is not easy... will try.

comment:11 Changed 14 months ago by Antoine Martin

Milestone: 0.181.0

Milestone renamed

comment:12 Changed 14 months ago by Antoine Martin

Status: newassigned

See r13022 and #1257.

We also need to skip some frames if we've sent them as jpeg already.

Last edited 14 months ago by Antoine Martin (previous) (diff)

comment:13 Changed 14 months ago by Antoine Martin

  • r13045 fixes a bug in the x264 encoder, made apparent by the use of b-frames and the work in #1257
  • r13046 tells the client to skip the delayed video frames if we have sent a non-video frame for it already - this should fix some of the out-of-order display issues reported

comment:14 Changed 14 months ago by Antoine Martin

Owner: changed from Antoine Martin to J. Max Mena
Status: assignednew

@maxmylyn: can you reproduce with the latest code?
(see also: ticket:1257#comment:3)

comment:15 Changed 14 months ago by J. Max Mena

Owner: changed from J. Max Mena to Antoine Martin

Yes, it's still problematic as of r13052.

Most notably, you can see it by scrolling text-sites in Firefox, and entering text in text-boxes (Trac comments, yelling at people through the internet, etc etc), and by changing tabs.

It's better, but still nowhere as good as before the B-Frames started.

comment:16 Changed 14 months ago by Antoine Martin

Owner: changed from Antoine Martin to J. Max Mena

It's better, but still nowhere as good as before the B-Frames started.


Does it go away if you run the with b-frames disabled? (XPRA_B_FRAMES=0)

If so, then we're using b-frames for the main window area, and as per ticket:1257#comment:3, this should only occur if the whole window is detected as a video area.
Can you confirm? Is the video region detection getting it wrong? (#410)

Using XPRA_VIDEO_SUBREGION=0 should have a similar effect: disabling video regions should now also disable b-frames.

comment:17 Changed 14 months ago by J. Max Mena

Owner: changed from J. Max Mena to Antoine Martin

Yes, you're correct - it only shows up when the whole page paints as h264. I don't think the region detect is failing, but rather a corner case we're not handling properly.

And, yes, it goes away with XPRA_B_FRAMES=0


Sorry, I had this ticket all typed out and never clicked submit.

comment:18 Changed 14 months ago by Antoine Martin

Resolution: fixed
Status: newclosed

OK, so let's keep this ticket only about b-frames, we can handle the blurriness caused by the video detection firing for the browser in #967 (re-opened).
#1232 will help a lot here as we may be able to avoid using video encodings when scrolling.

Unless proven otherwise, b-frames are being used when we detect video. So I am closing this, we can follow up and test some more in #1257. See also #1261 for multiple b-frames support.

Last edited 14 months ago by Antoine Martin (previous) (diff)

comment:19 Changed 13 months ago by alas

Resolution: fixed
Status: closedreopened

Re-opening... as mentioned originally in #1290, when mousing "vigorously" over a video region for several seconds, frames occasionally seem to display out of order. This behavior isn't seen when running with XPRA_B_FRAMES=0.

Testing with 1.0 r13101 win32 client against 1.0 r13410 fedora 23 server, with -d regionrefresh,mouse I got the following logs (migrated from #1290):
attachment/ticket/800/800-regionrefresh-mouse.log

The frame rate drops I left details about in #1290, but the out-of-order frames don't start to be noticed until the frame rate starts to drop.

Using watch -n 1 "xpra info | grep fps=" to watch the frame rate, I am seeing a video_subregion.fps value, which suggests that the region is still painting as video (from the reduced framerate):

window.2.encoder.fps=8
window.2.video_subregion.fps=7.56691646576

... but the -d paint logs from the client seem to be indicating a lot of RGB paint state -though, looking through some more, I do see that it is also periodically switching back to a YUV paint state (video?).

Looking at the -d paint logs with b-frames enabled vs. disabled I'm not seeing much difference, but I'll paste a sample of each for you to look at.

Mousing over video region with XPRA_B_FRAMES=0:
attachment/ticket/800/800-nobframes.log

Doing the same without disabling the b-frames (seems to take a lot longer to switch back to YUV):
attachment/ticket/800/800-bframes.log

Last edited 13 months ago by Antoine Martin (previous) (diff)

Changed 13 months ago by Antoine Martin

Attachment: 800-bframes.log added

with b-frames

Changed 13 months ago by Antoine Martin

Attachment: 800-nobframes.log added

without b-frames

Changed 13 months ago by Antoine Martin

Attachment: 800-regionrefresh-mouse.log added

regionrefresh and mouse debug

comment:20 Changed 13 months ago by Antoine Martin

  • converted to attachments so I can read comment:19
  • r13474 rounds up the fps value which would cause the bencoder to choke otherwise
  • the fact that the region fps is so low makes me think that there is a bottleneck before we get the damage events: either the application submitting events to X11, X11 itself, or us processing X11 events.. will have to try to figure out which - now tracked in #1299.
Last edited 13 months ago by Antoine Martin (previous) (diff)

comment:21 Changed 13 months ago by Antoine Martin

Owner: changed from Antoine Martin to alas
Status: reopenednew

It is possible that when the fps drops lower than 10, we switch back to a non-video encoding whilst a b-frame is still due. This would cause the non-video frame to arrive before both the delayed b-frame and its auto-refresh... r13598 should fix this by excluding the video region whenever a b-frame is still pending.
There may still be a corner case: if the video region is moving on screen (ie: the page is scrolling), then we may end up excluding the region where it was rather than where it actually is... r13599 tries to prevent that.


This may be easier to test before #1299 is fixed (assuming it is our problem to fix).
This issue could also be triggered by playing a youtube video (high fps) then stopping it (the last few updates will be low fps), but it is going to be hard to spot: b-frames only happen 50% of the time and those last few frames are likely identical... a better option may be to use opengl paint box, or "-d compress" (server side) or "-d paint" (client side).

comment:22 Changed 13 months ago by Antoine Martin

comment:23 Changed 12 months ago by J. Max Mena

With an r13767 Fedora 23 Server and Client:

(As mentioned in the scrum Tuesday - wasn't in office to make a screenrecord)

  • Continued scrolling well after the quality gets lowered causes an odd corner case:
  • If you then scroll with the mouse and stop scrolling, the heuristics will continue painting, but the last frame or so gets skipped, so the window keeps scrolling, and then when the refresh triggers, the window appears to "jump". It's reliably triggerable in Firefox and Chrome. I'll attach a screenrecord.

comment:24 Changed 12 months ago by J. Max Mena

Okay, so the video I made is too large to attach here, so I uploaded it to Google drive - I'll paste the link into the bottom of this ticket.

Also of note:

  • This behavior is not triggerable (despite much trying) with the flag XPRA_B_FRAMES=0 set client-side before connecting. (Disabling b-frames)

EDIT:

The actual link:

https://drive.google.com/file/d/0B54bNUvCEvvrNXdDS3J3U2ZzNEE/view?usp=sharing

Last edited 12 months ago by J. Max Mena (previous) (diff)

comment:25 Changed 12 months ago by Antoine Martin

Owner: changed from alas to J. Max Mena

This behaviour may well be unavoidable to some extent: we don't know if and when a new frame is going to come through... so we schedule a flush of the video encoder just in case nothing comes (the delay varies, it will be longer when we're quite busy). r13773 improves this by also flushing before doing any kind of screen refresh. This should make things appear smoother: you'll get another intermediate b-frame before the lossless refresh.

Another fix: prior to r13775, the video encoder would only use b-frames if we detected "true video" before (re)creating the video encoder. The new code can toggle this on or off as the results from the heuristics change. This should result in b-frames being used in more cases, which may also make some issues more apparent..

PS: #1014 may make it more difficult to test b-frames at certain screen sizes, you may want to force "encoding=h264" if that's the case.

Last edited 12 months ago by Antoine Martin (previous) (diff)

comment:26 Changed 12 months ago by J. Max Mena

Owner: changed from J. Max Mena to Antoine Martin

r13773 seems to have fixed my corner case and I'm having a hard time finding another one, despite trying very vigorously.

As farr (heh) as I can tell, it's been fixed; even after several hours testing. Even Chrome which is very liberal with its painting is behaving well.

Passing back to you to decide what to do with this ticket.

comment:27 Changed 12 months ago by Antoine Martin

Resolution: fixed
Status: newclosed

Thanks!
Closing at last.

Note: See TracTickets for help on using tickets.