| Message ID | 1F47539E-5ACE-4AFD-956B-266FAA04098D@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi, > + /* Determines the pixel format of the frame buffer */ > + if (surface->format == PIXMAN_b8g8r8x8) { > + bitmap_info = kCGBitmapByteOrder32Big | kCGImageAlphaNoneSkipFirst; > + } That certainly goes into the right direction. PIXMAN_* is native endian though, so I expect this will work on the intel macos host you are testing on but will fail on powerpc macos hosts. I suggest to add fixed endian defines for 32bpp to include/ui/qemu-pixman.h (there already is one for 24bpp), then use these to avoid cluttering the cocoa code with HOST_WORDS_BIGENDIAN #defines. The colorspace bits look sane to me, I'm not macos x expert enough to really justify. cheers, Gerd
On Jan 12, 2015, at 4:12 AM, Gerd Hoffmann wrote: > Hi, > >> + /* Determines the pixel format of the frame buffer */ >> + if (surface->format == PIXMAN_b8g8r8x8) { >> + bitmap_info = kCGBitmapByteOrder32Big | kCGImageAlphaNoneSkipFirst; >> + } > > That certainly goes into the right direction. Thank you. > > PIXMAN_* is native endian though, so I expect this will work on the > intel macos host you are testing on but will fail on powerpc macos > hosts. Unfortunately there appears to be no way to know. The last PowerPC Macs came out over 9 years ago. There probably isn't anyone on the list who uses one. > > I suggest to add fixed endian defines for 32bpp to > include/ui/qemu-pixman.h (there already is one for 24bpp), then use > these to avoid cluttering the cocoa code with HOST_WORDS_BIGENDIAN > #defines. > > The colorspace bits look sane to me, I'm not macos x expert enough to > really justify. If someone volunteered to test any code changes on their PowerPC Mac, then I would try this.
On 12 January 2015 at 14:51, Programmingkid <programmingkidx@gmail.com> wrote: > On Jan 12, 2015, at 4:12 AM, Gerd Hoffmann wrote: >> I suggest to add fixed endian defines for 32bpp to >> include/ui/qemu-pixman.h (there already is one for 24bpp), then use >> these to avoid cluttering the cocoa code with HOST_WORDS_BIGENDIAN >> #defines. >> >> The colorspace bits look sane to me, I'm not macos x expert enough to >> really justify. > > If someone volunteered to test any code changes on their PowerPC Mac, > then I would try this. It generally works the other way around -- if the documentation for something says X, then you write the code assuming that, and test it on the platforms you have access to, and trust that the others will work anyway. You don't write code that the documentation says won't work portably just because it happens to work on the platforms you have access to... [In this case "X" is "pixman formats are host-endian".] Also, this should almost certainly be a switch (surface->format) and bail out on things we can't handle. thanks -- PMM
On 12/01/2015 15:51, Programmingkid wrote: >>>>> + /* Determines the pixel format of the frame buffer */ + >>>>> if (surface->format == PIXMAN_b8g8r8x8) { + >>>>> bitmap_info = kCGBitmapByteOrder32Big | >>>>> kCGImageAlphaNoneSkipFirst; + } >>> >>> That certainly goes into the right direction. > Thank you. > >>> PIXMAN_* is native endian though, so I expect this will work on >>> the intel macos host you are testing on but will fail on powerpc >>> macos hosts. > Unfortunately there appears to be no way to know. The last PowerPC > Macs came out over 9 years ago. There probably isn't anyone on the > list who uses one. I have one, though it does not have enough memory to run Mac OS X guests. In any case, pixman clearly says that b8g8r8x8 is BGRA in host-endianness, so not the same as kCGBitmapByteOrder32Big. So your patch just needs something like this in ui/cocoa.m: #ifdef HOST_WORDS_BIGENDIAN #define PIXMAN_BE_b8g8r8x8 PIXMAN_b8g8r8x8 #else #define PIXMAN_BE_b8g8r8x8 PIXMAN_x8r8g8b8 #endif so that you can replace PIXMAN_b8g8r8x8 with PIXMAN_BE_x8r8g8b8 in your test. (You'll also need a matching "else" that restores kCGBitmapByteOrder32Little---if only for clarity: assuming little-endian in the initializer is ugly). Paolo
On 12 January 2015 at 15:11, Paolo Bonzini <pbonzini@redhat.com> wrote: > So your patch just needs something like this in ui/cocoa.m: > > #ifdef HOST_WORDS_BIGENDIAN > #define PIXMAN_BE_b8g8r8x8 PIXMAN_b8g8r8x8 > #else > #define PIXMAN_BE_b8g8r8x8 PIXMAN_x8r8g8b8 > #endif In qemu-pixman.h, surely? (goes with the existing one we have). -- PMM
On 12/01/2015 16:14, Peter Maydell wrote: >> > #ifdef HOST_WORDS_BIGENDIAN >> > #define PIXMAN_BE_b8g8r8x8 PIXMAN_b8g8r8x8 >> > #else >> > #define PIXMAN_BE_b8g8r8x8 PIXMAN_x8r8g8b8 >> > #endif > In qemu-pixman.h, surely? (goes with the existing one we have). Oh, yes, I wasn't aware of PIXMAN_BE_r8g8b8. In euther case it is kCGImageAlphaNoneSkipFirst, since the unused bits are in the most significant bits when interpreted in the "correct" endianness. Paolo
On Jan 12, 2015, at 10:11 AM, Paolo Bonzini wrote: > > > On 12/01/2015 15:51, Programmingkid wrote: >>>>>> + /* Determines the pixel format of the frame buffer */ + >>>>>> if (surface->format == PIXMAN_b8g8r8x8) { + >>>>>> bitmap_info = kCGBitmapByteOrder32Big | >>>>>> kCGImageAlphaNoneSkipFirst; + } >>>> >>>> That certainly goes into the right direction. >> Thank you. >> >>>> PIXMAN_* is native endian though, so I expect this will work on >>>> the intel macos host you are testing on but will fail on powerpc >>>> macos hosts. >> Unfortunately there appears to be no way to know. The last PowerPC >> Macs came out over 9 years ago. There probably isn't anyone on the >> list who uses one. > > I have one, though it does not have enough memory to run Mac OS X > guests. In any case, pixman clearly says that b8g8r8x8 is BGRA in > host-endianness, so not the same as kCGBitmapByteOrder32Big. > > So your patch just needs something like this in ui/cocoa.m: > > #ifdef HOST_WORDS_BIGENDIAN > #define PIXMAN_BE_b8g8r8x8 PIXMAN_b8g8r8x8 > #else > #define PIXMAN_BE_b8g8r8x8 PIXMAN_x8r8g8b8 > #endif > > so that you can replace PIXMAN_b8g8r8x8 with PIXMAN_BE_x8r8g8b8 in your > test. (You'll also need a matching "else" that restores > kCGBitmapByteOrder32Little---if only for clarity: assuming little-endian > in the initializer is ugly). > > Paolo It doesn't look like my color patch is needed anymore. This repo fixes all color problems - without touching a line of code in cocoa.m! git://git.kraxel.org/qemu branch rebase/console-wip
On Jan 12, 2015, at 10:04 AM, Peter Maydell wrote: > On 12 January 2015 at 14:51, Programmingkid <programmingkidx@gmail.com> wrote: >> On Jan 12, 2015, at 4:12 AM, Gerd Hoffmann wrote: >>> I suggest to add fixed endian defines for 32bpp to >>> include/ui/qemu-pixman.h (there already is one for 24bpp), then use >>> these to avoid cluttering the cocoa code with HOST_WORDS_BIGENDIAN >>> #defines. >>> >>> The colorspace bits look sane to me, I'm not macos x expert enough to >>> really justify. >> >> If someone volunteered to test any code changes on their PowerPC Mac, >> then I would try this. > > It generally works the other way around -- if the documentation for > something says X, then you write the code assuming that, and test it > on the platforms you have access to, and trust that the others will > work anyway. You don't write code that the documentation says won't > work portably just because it happens to work on the platforms you > have access to... > [In this case "X" is "pixman formats are host-endian".] Interesting idea. Will try it this way in future patches.
diff --git a/ui/cocoa.m b/ui/cocoa.m index 704d199..685081e 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -64,6 +64,10 @@ static int last_buttons; int gArgc; char **gArgv; +/* bitmap_info is used in drawRect:. Starts with little endian format. */ +static int bitmap_info = kCGBitmapByteOrder32Little | kCGImageAlphaNoneSkipFirst; +SInt32 current_mac_os_version; + // keymap conversion int keymap[] = { @@ -238,7 +242,15 @@ static int cocoa_keycode_to_qemu(int keycode) return keymap[keycode]; } - +/* Finds out what version of the Mac OS your computer is using. */ +static void determineMacOSVersion() +{ + OSErr err_num = Gestalt(gestaltSystemVersion, ¤t_mac_os_version); + if(err_num != noErr) { + current_mac_os_version = -1; + fprintf(stderr, "\nWarning: Failed to determine Mac OS version of your system!\n"); + } +} /* ------------------------------------------------------ @@ -257,6 +269,7 @@ static int cocoa_keycode_to_qemu(int keycode) BOOL isAbsoluteEnabled; BOOL isMouseDeassociated; NSDictionary * window_mode_dict; /* keeps track of the guest' graphic settings */ + CGColorSpaceRef color_space; /* used in drawRect: */ } - (void) switchSurface:(DisplaySurface *)surface; - (void) grabMouse; @@ -299,6 +312,13 @@ QemuCocoaView *cocoaView; screen.width = frameRect.size.width; screen.height = frameRect.size.height; [self updateWindowModeSettings]; + + if (current_mac_os_version >= MAC_OS_X_VERSION_10_4) + color_space = CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB); + else { + /* Using this in Mac OS 10.6 causes occasional crashes in drawRect:. */ + color_space = CGColorSpaceCreateDeviceRGB(); + } } return self; } @@ -361,13 +381,8 @@ QemuCocoaView *cocoaView; screen.bitsPerComponent, //bitsPerComponent screen.bitsPerPixel, //bitsPerPixel (screen.width * (screen.bitsPerComponent/2)), //bytesPerRow -#ifdef __LITTLE_ENDIAN__ - CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB), //colorspace for OS X >= 10.4 - kCGBitmapByteOrder32Little | kCGImageAlphaNoneSkipFirst, -#else - CGColorSpaceCreateDeviceRGB(), //colorspace for OS X < 10.4 (actually ppc) - kCGImageAlphaNoneSkipFirst, //bitmapInfo -#endif + color_space, + bitmap_info, dataProviderRef, //provider NULL, //decode 0, //interpolate @@ -835,6 +850,7 @@ QemuCocoaView *cocoaView; self = [super init]; if (self) { + determineMacOSVersion(); // create a view and add it to the window cocoaView = [[QemuCocoaView alloc] initWithFrame:NSMakeRect(0.0, 0.0, 640.0, 480.0)]; @@ -1072,16 +1088,13 @@ int main (int argc, const char * argv[]) { return 0; } - - #pragma mark qemu static void cocoa_update(DisplayChangeListener *dcl, int x, int y, int w, int h) { - NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; - COCOA_DEBUG("qemu_cocoa: cocoa_update\n"); - + NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; + NSRect rect; rect = NSMakeRect(x, [cocoaView gscreen].height - y - h, w, h); [cocoaView setNeedsDisplayInRect:rect]; @@ -1094,6 +1107,12 @@ static void cocoa_switch(DisplayChangeListener *dcl, NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; COCOA_DEBUG("qemu_cocoa: cocoa_switch\n"); + + /* Determines the pixel format of the frame buffer */ + if (surface->format == PIXMAN_b8g8r8x8) { + bitmap_info = kCGBitmapByteOrder32Big | kCGImageAlphaNoneSkipFirst; + } + [cocoaView switchSurface:surface]; [pool release]; }
This patch fixes the Mac OS X guest color problem on Mac OS X hosts. Tested using Mac OS 10.2 and Debian Linux 5 operating systems for the guest on qemu-system-ppc. Also tested using Windows XP as a guest in qemu-system-i386. Signed-off-by: John Arbuckle <programmingkidx@gmail.com> --- v2: Eliminated the -display-endian-big command line switch and replaced with code that automatically detects the correct pixel format. ui/cocoa.m | 45 ++++++++++++++++++++++++++++++++------------- 1 files changed, 32 insertions(+), 13 deletions(-)