Hi Armin, thanks for the quick response...
2016-05-23 12:05 GMT+02:00 Armin Novak <armin.no...@thincast.com>: > Hey Ondrej, > > On 05/23/2016 11:51 AM, Ondrej Holy wrote: > > Hi Armin, > > 2016-05-19 9:18 GMT+02:00 Armin Novak <armin.no...@thincast.com>: > >> Hey Ondrej, >> >> sorry for the late reply, did indeed missed to reply. >> > > neverminds, thanks for the replay. > > > Most of the code works on a big endian, because >> > Stream_Read_/Stream_Write_ macros are used to read/write data from/to >> > server. There is a problem if Stream_Pointer is used to read raw data, >> > because the data are always stored as little endian. It is problem >> > especially with wide strings and colors. It seems to me that usage of >> > something like Stream_Read_Bitmap/Stream_Read_String would be >> > suboptimal, because it requires additional data processing, but maybe >> > this is the only right way to do it. >> Wide String read / write is using internal conversion to UTF-8, so the >> use of raw data should not be to big an issue there. For color >> conversion stuff the data layout for data from RDP is fixed in the spec, >> so the layout will not change there. Converting bitmaps from local to >> RDP is a different beast though, one I still try to fix properly. >> > > Did not know that this is fixed in the documentation, thanks for notice, > have to take a look... > > It is a bit spread through the different documents but so far I could not > find anything that did not have a fixed byte order. The only real variable > here are bitmaps directly send in GDI mode, everything else is already > compressed and needs to be decoded anyway. > > > >> > Some of the color conversions access the data byte per byte, some of >> > them access them as UINT16, or UINT32 currently. This is obviously a >> > problem for a big endian. It seems that your patches fixes this >> >> > Ok, then the following code from xf_client.c is totally wrong... > xfc->invert = (ImageByteOrder(xfc->display) == MSBFirst) ? TRUE : FALSE; > ... > xfc->format = (xfc->invert) ? PIXEL_FORMAT_RGBA32 : PIXEL_FORMAT_BGRA32; > > and something like FREERDP_PIXEL_FORMAT_ENDIANNESS should be added > instead, what do you think about it? > > This looks more like a client issue to determine the actual framebuffer > layout of the backend and not something libfreerdp can handle. I'd like to > keep X11/Wayland/Win/Mac/Android/... stuff out of the core library. > The man page of ImageByteOrder is hard to understand without context > though :/ > As for using fixed byte order, remember X11 has network transparency, so > the stuff actually displaying might have a different endianess than the one > running xfreerdp. > The problem is that freerdp_image_copy can't return bitmap in such format, which is needed by X11 under big endian. Bitmaps are received in little endian order, but needs to be written in the big endian order in certain cases. It seems that color overhaul fixes lot of problems for big endian, but the final bitmap is still with little endian order. Yea, ImageByteOrder and all X11 documentation is a bit mess, but according to my testing it depends on where the X11 server is running. ImageByteOrder is LSBFirst in case of SSH X11 forwarding from big endian machine to little endian machine (because it uses server from the little endian machine), and MSBFirst in case of VNC from big endian machine (because VNC is transfering just image from big endian X11 server). The X11 requires bitmaps in RGBA regardless of LSBFirst/MSBFirst, however it expects little endian order for LSBFirst and big endian order for MSBFirst. So the color needs to be written by Data_Write_UINT32 for LSBFirst and Data_Write_UINT32_BE for MSBFirst. I expect that your color overhaul fixes color for the SSH X11 forwarding case, because you manipulate with bitmaps byte per byte, which is equal to Data_Write_UINT32... but it segfaulted for me (even on little endian) as per the another thread, so I can't prove it. As far as I know also MacOS runs on big endian by default. It seems mfreerdp solves this problem by kCGBitmapByteOrder32Little, so it uses bitmaps in little endian order, but I can't find any alternative for X11. Bytes in the whole bitmap needs to be swapped directly in the X11 client if you think that this should not be in the core library, so something like the following code from cairo: https://cgit.freedesktop.org/cairo/tree/src/cairo-xlib-surface.c#n531 However I think it would be better to do it as a part of freerdp_image_copy to avoid additional processing... that's why I suggested something like FREERDP_PIXEL_FORMAT_ENDIANNESS. Do you still think that this should be handled by xfreerdp itself? If so I can add something like the following in the xfreerdp codes before each XCreateImage and after freerdp_image_copy: if (xfc->big_endian) _swap_ximage_to_native -- Regards Ondrej ------------------------------------------------------------------------------ Mobile security can be enabling, not merely restricting. Employees who bring their own devices (BYOD) to work are irked by the imposition of MDM restrictions. Mobile Device Manager Plus allows you to control only the apps on BYO-devices by containerizing them, leaving personal data untouched! https://ad.doubleclick.net/ddm/clk/304595813;131938128;j _______________________________________________ FreeRDP-devel mailing list FreeRDP-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/freerdp-devel
