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

Progressive RFX merge #50

Merged
merged 1 commit into from
May 9, 2022
Merged

Conversation

Nexarian
Copy link
Contributor

This is simply a consolidation of https://github.com/jsorg71/librfxcodec/commits/egfx rebased on the latest devel.

Haven't tested it yet.

@matt335672
Copy link
Member

Happy to merge if it doesn't break any existing RFX functionality, if that would help.

@Nexarian
Copy link
Contributor Author

It's important to get this merged as a first step to integrating GFX, because otherwise you'll have build breaks in the progressive RFX part. Other than xorgxrdp, this is the only other dependency that needs updating to start that integration.

I will commit to doing some testing of this branch to verify that regular RFX still works, that Progressive RFX still works with my latest mainline_merge branch, and that the performance isn't degraded in any meaningful way. In order to test with MSTSC and the Mac OS Client, I have to actually disable the H264 capability set manually to force the progressive mode, but other clients are easier such as Microsoft Remote Desktop on Windows and FreeRDP (where I can manually specify the protocol).

@matt335672
Copy link
Member

Please do some regular testing. When you're happy with it, I'll merge it in.

@Nexarian Nexarian force-pushed the egfx_mainline_merge branch 3 times, most recently from e92ab8e to c0e6938 Compare March 22, 2022 04:11
@metalefty metalefty changed the title {Work In Progress] Progressive RFX merge [Work In Progress] Progressive RFX merge Mar 22, 2022
@Nexarian
Copy link
Contributor Author

Nexarian commented May 6, 2022

Verified that it doesn't break vanilla RFX. That is, connecting with MSTSC on a 2560x1440 monitor to an Ubuntu 20.04 machine running in VirtualBox. That works.

Next is to test: Dynamic resizing with MSTSC, RemoteFX with FreeRDP, and make sure this works correctly with Progressive RFX on the GFX branch(es)

@Nexarian
Copy link
Contributor Author

Nexarian commented May 6, 2022

Verified that with /rfx and FreeRDP on a 2560x1440 monitor it works.

Will test MSTSC dynamic resize and Progressive RFX tomorrow.

Largely a consolidation of the work by jsorg71, with a few minor bug
fixes.

Verified that RFX Progressive works.
@Nexarian
Copy link
Contributor Author

Nexarian commented May 9, 2022

@matt335672 This works with Progressive RFX now as well. Didn't test with MSTSC, but I don't think I need to. This also works with FreeRDP RFX.

I recommend we merge this if @jsorg71 is OK with it. I've at least verified that it doesn't make anything worse.

@Nexarian Nexarian changed the title [Work In Progress] Progressive RFX merge Progressive RFX merge May 9, 2022
@metalefty
Copy link
Member

Let's merge 'cause librfxcodec is a submodule.

@@ -53,7 +53,7 @@ rfx_encode_component_rlgr1_amd64_sse2(struct rfxencode *enc, const char *qtable,
{
return 1;
}
*size = rfx_encode_diff_rlgr1(enc->dwt_buffer1, buffer, buffer_size);
*size = rfx_encode_diff_rlgr1(enc->dwt_buffer1, buffer, buffer_size, 64);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, where is this 64 come from? How to calculate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Two steps are done, a diff and rlgr1. LL3 is the band that gets diffed. In the original RemoteFX, LL3 was 8x8. In Progressive RemoteFX, LL3 is 9x9. These should be defines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should the defines be? Maybe a second pull request to clean it up?

@metalefty
Copy link
Member

Tested by MSTSC without Progressive RFX. Nothing's broken and it works well. Thanks!

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.

4 participants