Color profile check YUV2RGB matrix

[akallabeth]

thanks to @alexandru-bagu for doing such intensive testing :)

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6619/

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6624/

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6625/

[akallabeth]

@alexandru-bagu looks like the sse and neon implementations still are buggy, the comparison tests to generic fail.

[alexandru-bagu]

@alexandru-bagu looks like the sse and neon implementations still are buggy, the comparison tests to generic fail.

Will check them today. I know my ssse tests didnt fail but I did run them on osx. Will setup a docker env for ubuntu.

[alexandru-bagu]

It seems to be an intermittent issue. Sometimes I get a test failure for TestSynchCritical. Sometimes I get a failure for TestPrimitivesYUV. Sometimes I get neither failure... I think I know where the issue is though.

[alexandru-bagu]

I've spent 11 hours I think today working on this.... and it's annoying!
I've figured out most of the SSE stuff and after updating to the correct formula as in Wikipedia some tests still fail in some cases. The commits can be seen here: alexandru-bagu@387996e and alexandru-bagu@9e497cf .

The tests that still fail are TestPrimitiveRgbToLumaChroma when the RGBA values are random. This appears to be because of how _mm_maddubs_epi16 (https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm_maddubs_epi16&ig_expand=4454) is implemented. It uses at most 15 bits from the multiplications, because of this fact and the fact that the generic implementation does not have such a limitation (one could be implemented like the Saturate16). If we were to set the same limitation for the generic implementation then I believe we would have the same results (however I don't believe that to be wanted).

I will need someone to review the SSE changes I made, more specifically the usage of _mm_packus_epi16 instead of _mm_packs_epi16. I had to replace the calls to _mm_packs_epi16 with _mm_packus_epi16 because data was being lost due to the same behaviour seen in _mm_maddubs_epi16. Though since the tests don't fail I guess I did a good enough job there.

I have not touched the neon implementation and I don't plan to, I've wrecked my brains today working on this.

[akallabeth]

@alexandru-bagu yes, intrinsics are annoying, specifically if you are operating at the limit of the datatypes involved. (that is the reason for the conversion matrix in the spec, it is optimized for 8/16 bit data types).
Will have a look when back, I'm not available next week.

[alexandru-bagu]

I figured out the SSE issues that were left in alexandru-bagu@3885e20 . Reverted the test to use rand() and it's all good. Due to the limitations of SSE I had to do 3 additional operations so there's no rounding done anymore.

[alexandru-bagu]

Squashed some commits and created a PR in your branch. akallabeth#3

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6637/

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6638/

[akallabeth]

@alexandru-bagu damn, looks like the different implementations need some more attention, the unit test fails (generic and sse produce differing results)

[alexandru-bagu]

Ok, I will have another look and will let you know.

[alexandru-bagu]

You are missing my last 3 commits. Seems after you merge my pull-request you overwritten your branch with a force push.
https://github.com/alexandru-bagu/FreeRDP/commits/color_profile_check

[akallabeth]

@alexandru-bagu ok, strange, I've merged your pr. well, cherry-picked them now, lets see if that works out.

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6640/

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6642/

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6644/

[akallabeth]

@llyzs maybe you can have a look at this too?

[llyzs]

@akallabeth I don't know what was the original issue that the PR is trying to solve, but we should not use different color matrix than specified in the MS spec. The problem could be elsewhere.

[akallabeth]

@llyzs the issue is only visible with [RDPEVOR] video redirection.
When the h264 data is sent, then the colors do not match the background.
To test, play a video on youtube and wait until the video redirection kicks in.
When that happens, the white area surrounding the video will have a slightly different color from what it previously was. (the redirection does usually contain a few lines around the video as well)

[alexandru-bagu]

Maybe it would be worth considering whether we can have the matrix change only for decoding when we know we are connected to a Windows terminal server implementation, since that's when the issue presents itself.

[akallabeth]

@alexandru-bagu you don´t know that, there is no way to (properly) identify the remote os

[freerdp-bot]

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/6661/