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

better sub-window encoding: detect regions and use sub video encoder #410

Closed
totaam opened this issue Aug 9, 2013 · 98 comments
Closed

better sub-window encoding: detect regions and use sub video encoder #410

totaam opened this issue Aug 9, 2013 · 98 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Aug 9, 2013

Issue migrated from trac ticket # 410

component: core | priority: critical | resolution: fixed

2013-08-09 07:19:24: totaam created the issue


If you have a large window but only a fraction of that window changes we waste a lot resources.
We currently create a video encoder for the whole window and waste a fair bit of time capturing, csc-ing, encoding, (sending), decoding and displaying regions of the screen that have not changed at all (and we know they haven't)
[[BR]]
We have the "small region" code, which deals with small-ish regions by sending them using another encoding (usually rgb or png/jpeg) but it has its limits: it does not use a video encoder and is limited in size.
[[BR]]
We should keep track of the damage areas in more detail, including their location and size (easy). Then we can detect regions of the screen that update often (easy-ish) and create a sub video encoder just for those (harder).
This could even work when the region is moving, as long as its size stays the same.
This is negated somewhat by the fact that when we send the whole window (because of other updates), it will include this region - unless we purposedly blank it out before compression and ask the client to re-assemble the two before displaying the result (hard).

An even better solution to this would be #509 or #510, as this would give us the video area precisely everytime, even before it gets converted to RGB pixels.

@totaam
Copy link
Collaborator Author

totaam commented Feb 2, 2014

2014-02-02 06:35:56: antoine changed status from new to assigned

@totaam
Copy link
Collaborator Author

totaam commented Feb 2, 2014

2014-02-02 06:35:56: antoine edited the issue description

@totaam
Copy link
Collaborator Author

totaam commented Feb 10, 2014

2014-02-10 14:10:19: antoine uploaded file video-subregion-client.patch (4.9 KiB)

allows clients to process video sub-regions

@totaam
Copy link
Collaborator Author

totaam commented Feb 10, 2014

2014-02-10 14:10:39: antoine uploaded file video-subregion-server.patch (15.2 KiB)

server side patch: detect and use video sub-regions

@totaam
Copy link
Collaborator Author

totaam commented Feb 10, 2014

2014-02-10 14:22:22: antoine commented


Lots of preparatory work in:

With the 2 patches attached, it seems to work. Including with the new test app tests.xpra.test_apps.test_videoregions (as long as the server is started with XPRA_FORCE_BATCH=1)

Still TODO:

  • add some tests for the region substraction code (looks right - but too hard to tell for sure by manual inspection)
  • ensure we don't use the window dimensions anywhere in the encoder/csc pipeline creation or scoring methods (looks clean!)
  • test with scaling
  • test with proxy encoding
  • benchmark and profile it

Maybes:

  • take fps into account: discard old damage events?
  • pass subregion down to encoders so we can blank it out if we've sent it as video already?
  • somehow pass an event to the client telling it that we've discarded the video encoder (rather than wait for the window to close, or a new encoding stream to take its place)
  • support multiple regions (hard)

@totaam
Copy link
Collaborator Author

totaam commented Feb 11, 2014

2014-02-11 14:20:40: antoine commented


Merged the uncontroversial bits in:

  • r5429: client side
  • r5430: region handling code and tests
  • r5431: proxy encoder support

The tricky thing about the server side is that applications do stupid repaints... ie: firefox will repaint things on the right hand side of the flash player (even though they don't change..), so we have to be more clever than planned and introduce more heuristics, and region matching code. Work in progress.

@totaam
Copy link
Collaborator Author

totaam commented Feb 11, 2014

2014-02-11 14:52:02: antoine uploaded file video-subregion-v4.patch (14.2 KiB)

remaining server side bits, still needs a proper way to ensure non-video regions don't use the video encoding (which does a re-init and wastes everything)

@totaam
Copy link
Collaborator Author

totaam commented Feb 12, 2014

2014-02-12 04:31:40: antoine uploaded file video-subregion-v5.patch (18.3 KiB)

better patch if a little ugly: caller can provide method for deciding which encoding to use (so we can ensure we don't use video for anything but the current subregion)

@totaam
Copy link
Collaborator Author

totaam commented Feb 12, 2014

2014-02-12 10:14:14: antoine uploaded file video-subregion-v6.patch (25.4 KiB)

ensure we don't end up using the video encoder whilst we still have a video region

@totaam
Copy link
Collaborator Author

totaam commented Feb 13, 2014

2014-02-13 12:21:22: antoine commented


Big commit in r5437 with lots of details, small fix in r5441.

Bigger problem with nvenc in #517 blocks further work on this one.

@totaam
Copy link
Collaborator Author

totaam commented May 19, 2014

2014-05-19 12:56:40: totaam changed status from assigned to new

@totaam
Copy link
Collaborator Author

totaam commented May 19, 2014

2014-05-19 12:56:40: totaam changed owner from antoine to afarr

@totaam
Copy link
Collaborator Author

totaam commented May 19, 2014

2014-05-19 12:56:40: totaam commented


Was actually enabled in 0.12.x and has been working OK so far.

afarr / smo: worth knowing about / testing as part of #419.

@totaam
Copy link
Collaborator Author

totaam commented Jun 20, 2014

2014-06-20 02:00:31: totaam commented


Follow up work in #596, should have been closed by now: done 4 months ago..

@totaam
Copy link
Collaborator Author

totaam commented Jun 20, 2014

2014-06-20 23:11:33: maxmylyn commented


Did some testing with some quality changes from #596#comment:2 :
Tested with a r6853 Win7 64-bit client against a r6853 Fedora 20 client (with a -d refresh piped into a .txt):

  • Opened up a YouTube video
  • Started a video
  • The whole webpage renders in a low quality.

After changing the min-quality setting to a higher number, the whole webpage starts rendering much nicer. Lowering min-quality causes the whole webpage to start looking choppy. When the video is paused, the webpage starts to render clearly.

  • Ran another connection(no logs), this time just leaving it on Wikipedia.
  • Hovering over links(causing them to become underlined, with no other webpage changes) caused the whole webpage to become blurry.

It looks like partial refreshes aren't working on the 14.0 r6853 Windows client for sure. Our OSX build currently doesn't have h.264, so it switched over to PNG, which only allowed for lossless rendering. When Windows was forced onto PNG, it behaved identically; drawing only lossless. However, the video played at a lower framerate due to Xpra not being able to keep up.

That being said, I tested with our CentOS client, which had h.264, and I got the exact same behavior. Partial webpage changes are causing a full-screen refresh. Sometimes with quality extremely low, making the window become very blurry.

In addition, I'll leave a comment on #596 as well.

@totaam
Copy link
Collaborator Author

totaam commented Jun 20, 2014

2014-06-20 23:12:05: maxmylyn uploaded file 410refresh.txt (861.6 KiB)

-d refresh from the server

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 00:58:04: afarr commented


As mentioned in #596, the above behavior shows with chrome, but with firefox (& mostly on lazarus) the refreshes seem to be behaving as expected.

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 04:07:39: totaam changed status from new to closed

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 04:07:39: totaam changed resolution from ** to fixed

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 04:07:39: totaam commented


This ticket is about xpra detecting that there is video on screen, and where it is. Judging by the log samples in #596, it does that.
Another way of verifying this is with:

xpra info | grep "\.video_subregion="

[[BR]]

.. caused the whole webpage to become blurry
It looks like partial refreshes aren't working on the 14.0 r6853 Windows client for sure
[[BR]]
Not necessarily. Just because you don't see anything refreshing visually, does not mean that the browser isn't repainting that part of the screen (often unnecessarily). This belongs in #596 anyway. Closing.

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 04:20:18: totaam changed status from closed to reopened

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 04:20:18: totaam changed resolution from fixed to **

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 04:20:18: totaam commented


Actually re-opening this bug: the issue from #596#comment:4 belongs here instead: not detecting the video region with chrome.

Extracts:

auto refresh: h264 screen update (quality= 37), keeping existing timer \
    (region=rectangle[0, 0, 1450, 894], refresh regions=[R[0, 0, 1450, 894]])

The video region is not detected. All I see is many like this one:

auto refresh: jpeg screen update (quality= 34), scheduling refresh \
    (region=rectangle[0, 0, 213, 26], refresh regions=[R[0, 0, 213, 26]])

Where was the video showing in the window? (213x26 at 0x0 looks like the address bar)
Please post the server's -d subregion log output of when the video is playing and not being detected. Also: which page can allow me to reproduce? Which screen settings, etc.

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 04:20:30: totaam changed status from reopened to new

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 04:20:30: totaam changed owner from afarr to maxmylyn

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 05:00:25: maxmylyn commented


I'll attach a screenshot that I took earlier

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 05:01:04: maxmylyn uploaded file 410videobeingblurry.png (352.9 KiB)

A screenshot depicting where the video was usually located while being watched in chrome.
410videobeingblurry.png

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 05:04:31: totaam commented


Please post more information: see comment:10, comment:9, include -d refresh, -d compress, -d damage, -d encoding. And maybe all of them together -d compress,damage,encoding.

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 05:05:35: maxmylyn commented


http://www.youtube.com/watch?v=f488uJAQgmw Here is the video. (a little crude humor, but I find it hilarious)

These were watched running full-screen on a 1080p monitor. The window wasn't entirely full-screen. I moved it to the top left and manually dragged to almost full-screen. Basically 1900-1916 x 1040-1075 pixels. All running in chrome - whatever the latest version is.

I'll get you some logs on Monday. (Unless I can get Xpra running at home - a task I'll get to at some point...)

@totaam
Copy link
Collaborator Author

totaam commented Jun 21, 2014

2014-06-21 05:35:09: totaam changed status from new to assigned

@totaam
Copy link
Collaborator Author

totaam commented Nov 25, 2014

2014-11-25 19:10:05: antoine commented


Raising priority. I've seen some bad behaviour in corner cases.

For example: we select the video region correctly, but then when we make the flash player fullscreen, the video region sticks when in fact we should be switching to fullscreen video.

I'll try to make the logging more useful, so we can capture the input to the video region detection state machine, then later replay it so we can tune it better.

@totaam
Copy link
Collaborator Author

totaam commented Dec 2, 2014

2014-12-02 03:22:10: antoine changed status from assigned to new

@totaam
Copy link
Collaborator Author

totaam commented Dec 2, 2014

2014-12-02 03:22:10: antoine changed owner from totaam to maxmylyn

@totaam
Copy link
Collaborator Author

totaam commented Dec 2, 2014

2014-12-02 03:22:10: antoine commented


Improved region detection in r8164, with also more logging.

I will work on feeding the logging back to the test tool so we can more easily debug and fine tune it.

@maxmylyn: In the meantime, can you break it? If so, can you provide log samples with -d regiondetect near the point where region detection does not work as well as you think it should.

@totaam
Copy link
Collaborator Author

totaam commented Jan 6, 2015

2015-01-06 12:43:37: antoine commented


More changes:

See also #615

@totaam
Copy link
Collaborator Author

totaam commented Jan 18, 2015

2015-01-18 08:42:27: totaam changed owner from maxmylyn to afarr

@totaam
Copy link
Collaborator Author

totaam commented Jan 18, 2015

2015-01-18 08:42:27: totaam commented


afarr: unless someone can break it, I think we should close this.

@totaam
Copy link
Collaborator Author

totaam commented Jan 24, 2015

2015-01-24 01:24:50: afarr commented


Can't seem to break anything. I do notice (with opengl paint boxes set on an osx client) that the XPRA_VIDEO_SUBREGION=0 setting seems to lead to youtube, for example, only using rgb24 and webp encodings though. Not sure that turning off video subregions is meant to turn off the h264 encdings... but not sure that it's not meant to.

Assuming that's intended, this looks good to close.

@totaam
Copy link
Collaborator Author

totaam commented Jan 24, 2015

2015-01-24 06:55:21: totaam commented


Not sure that turning off video subregions is meant to turn off the h264 encdings...
[[BR]]
It is not meant to, are you sure that it is?

$ xpra info | grep total_frames
window[2].total_frames[delta]=1
window[2].total_frames[h264]=644
window[2].total_frames[jpeg]=1
window[2].total_frames[png]=2
window[2].total_frames[rgb24]=703

Note: most of the rgb24 frames are edges of the video area (h264 can only encode even sizes, so we use rgb24 for the right and bottom edge)

@totaam
Copy link
Collaborator Author

totaam commented Feb 4, 2015

2015-02-04 01:03:58: afarr changed status from new to closed

@totaam
Copy link
Collaborator Author

totaam commented Feb 4, 2015

2015-02-04 01:03:58: afarr set resolution to fixed

@totaam
Copy link
Collaborator Author

totaam commented Feb 4, 2015

2015-02-04 01:03:58: afarr commented


Hmm... well I stand corrected. I'm not seeing any blue box outlines, but...

[jimador@zapopan ~]$ xpra info :23 | grep total_frames
window[1].total_frames[jpeg]=1
window[1].total_frames[webp]=1
window[2].total_frames[delta]=9
window[2].total_frames[h264]=581
window[2].total_frames[jpeg]=43
window[2].total_frames[png]=12
window[2].total_frames[rgb24]=1180
window[2].total_frames[webp]=3

Since 581 > 0 (significantly so)... I guess the last concern is solved.

Closing.

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

No branches or pull requests

1 participant