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

jpeg: Fix wrong JPEC codec GUID #2402

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

metalefty
Copy link
Member

@metalefty metalefty commented Oct 29, 2022

@jsorg71 Jay, I think that using a different GUID from FreeRDP/NeutrinoRDP is not intentional but a mere mistake. Because both are very similar. The only difference is order. Can you have a look?


Both FreeRDP and NeutrinoRDP CODEC_GUID_JPEG define:
430C9EED-1BAF-4CE6-869ACB8B37B66237

However, for some reason, xrdp defines it as:
1BAF4CE6-9EED-430C-869ACB8B37B66237

JPEG codec wasn't working at all due to this for years.

References:

@metalefty metalefty requested a review from jsorg71 October 29, 2022 05:35
Both FreeRDP and NeutrinoRDP CODEC_GUID_JPEG define:
    430C9EED-1BAF-4CE6-869ACB8B37B66237

However, for some reason, xrdp defines it as:
    1BAF4CE6-9EED-430C-869ACB8B37B66237

JPEG codec wasn't working at all due to this for years.

References:
* https://github.com/FreeRDP/FreeRDP/blob/843680e543db0608f5d6d1973ca1ecd3d7a0b817/libfreerdp/core/capabilities.c#L101-L105
* https://github.com/neutrinolabs/NeutrinoRDP/blob/50211223318a02db05b57196d03342edb0207c32/libfreerdp-core/capabilities.c#L65-L66
@jsorg71
Copy link
Contributor

jsorg71 commented Nov 4, 2022

Are you sure? To me it looks like NeutrinoRDP and xrdp are the same and FreeRDP is different. Although the comment in NeutrinoRDP is wrong.
It looks like the comment in FreeRDP is wrong too.

xrdp
/* CODEC_GUID_JPEG     1BAF4CE6-9EED-430C-869ACB8B37B66237 */
#define XR_CODEC_GUID_JPEG \
  "\xE6\x4C\xAF\x1B\xED\x9E\x0C\x43\x86\x9A\xCB\x8B\x37\xB6\x62\x37"

NeutrinoRDP
/* CODEC_GUID_JPEG 0x430C9EED1BAF4CE6869ACB8B37B66237*/
#define CODEC_GUID_JPEG "\xE6\x4C\xAF\x1B\xED\x9E\x0C\x43\x86\x9A\xCB\x8B\x37\xB6\x62\x37"

FreeRDP
/* CODEC_GUID_JPEG 0x430C9EED1BAF4CE6869ACB8B37B66237 */

static const GUID CODEC_GUID_JPEG = {
	0x430C9EED, 0x1BAF, 0x4CE6, { 0x86, 0x9A, 0xCB, 0x8B, 0x37, 0xB6, 0x62, 0x37 }
};

@metalefty
Copy link
Member Author

Hmm, you seem to be correct. FreeRDP and NeutrinoRDP comment is wrong.

@jsorg71
Copy link
Contributor

jsorg71 commented Nov 5, 2022

What a mess.
I think the only safe way to fix it is to treat all 3 the same. Something like.

jay@deb7sparc:/media/sdc/jay/git/neutrinolabs/xrdp$ git diff -u common/ms-rdpbcgr.h libxrdp/xrdp_caps.c
diff --git a/common/ms-rdpbcgr.h b/common/ms-rdpbcgr.h
index 4e5a5e4..ed8c8f5 100644
--- a/common/ms-rdpbcgr.h
+++ b/common/ms-rdpbcgr.h
@@ -392,6 +392,14 @@
 #define XR_CODEC_GUID_JPEG \
     "\xE6\x4C\xAF\x1B\xED\x9E\x0C\x43\x86\x9A\xCB\x8B\x37\xB6\x62\x37"

+/* CODEC_GUID_JPEG_ALT1 ED9E0C43-AF1B-E64C-869ACB8B37B66237 */
+#define XR_CODEC_GUID_JPEG_ALT1 \
+    "\x43\x0C\x9E\xED\x1B\xAF\x4C\xE6\x86\x9A\xCB\x8B\x37\xB6\x62\x37"
+
+/* CODEC_GUID_JPEG_ALT2 430C9EED-1BAF-4CE6-869ACB8B37B66237 */
+#define XR_CODEC_GUID_JPEG_ALT2 \
+    "\xED\x9E\x0C\x43\xAF\x1B\xE6\x4C\x86\x9A\xCB\x8B\x37\xB6\x62\x37"
+
 /* CODEC_GUID_PNG      0E0C858D-28E0-45DB-ADAA-0F83E57CC560 */
 #define XR_CODEC_GUID_PNG \
     "\x8D\x85\x0C\x0E\xE0\x28\xDB\x45\xAD\xAA\x0F\x83\xE5\x7C\xC5\x60"
diff --git a/libxrdp/xrdp_caps.c b/libxrdp/xrdp_caps.c
index 2f3d8e7..4c90e78 100644
--- a/libxrdp/xrdp_caps.c
+++ b/libxrdp/xrdp_caps.c
@@ -577,7 +577,9 @@ xrdp_caps_process_codecs(struct xrdp_rdp *self, struct stream *s, int len)
             g_memcpy(self->client_info.rfx_prop, s->p, i1);
             self->client_info.rfx_prop_len = i1;
         }
-        else if (g_memcmp(codec_guid, XR_CODEC_GUID_JPEG, 16) == 0)
+        else if ((g_memcmp(codec_guid, XR_CODEC_GUID_JPEG, 16) == 0) ||
+                 (g_memcmp(codec_guid, XR_CODEC_GUID_JPEG_ALT1, 16) == 0) ||
+                 (g_memcmp(codec_guid, XR_CODEC_GUID_JPEG_ALT2, 16) == 0))
         {
             LOG(LOG_LEVEL_INFO, "xrdp_caps_process_codecs: JPEG(%s), codec id [%d], properties len [%d]",
                 codec_guid_str, codec_id, codec_properties_length);
@@ -1111,6 +1113,16 @@ xrdp_caps_send_demand_active(struct xrdp_rdp *self)
     out_uint8(s, 0); /* codec id, client sets */
     out_uint16_le(s, 1); /* codecPropertiesLength */
     out_uint8(s, 75); /* jpeg compression ratio */
+    codec_caps_count++;
+    out_uint8a(s, XR_CODEC_GUID_JPEG_ALT1, 16);
+    out_uint8(s, 0); /* codec id, client sets */
+    out_uint16_le(s, 1); /* codecPropertiesLength */
+    out_uint8(s, 75); /* jpeg compression ratio */
+    codec_caps_count++;
+    out_uint8a(s, XR_CODEC_GUID_JPEG_ALT2, 16);
+    out_uint8(s, 0); /* codec id, client sets */
+    out_uint16_le(s, 1); /* codecPropertiesLength */
+    out_uint8(s, 75); /* jpeg compression ratio */
     LOG_DEVEL(LOG_LEVEL_TRACE, "xrdp_caps_send_demand_active: Server Capability "
               "JPEG: "
               "jpeg compression ratio = 75");

@jsorg71
Copy link
Contributor

jsorg71 commented Nov 10, 2022

I created neutrinolabs/NeutrinoRDP#28 to fix the comment for NeutrinoRDP

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.

2 participants