DisplayState interface change

PATCH In review Stefano Stabellini 3 participants 9 messages Dec 18, 2008 – Jan 15, 2009

lore

Share & tools

lore

Stefano Stabellini

author

2008-12-18 19:12 UTC #0 lore raw

PATCH 1/1 DisplayState interface change

Hi all,
this is the forth update of the "DisplayState interface change" series.

This update fixes the few remaining issue raised last time.

The series is made of 7 patches:

1) accessors again
some other substitutions in hw/sm501.c;

2) remove bgr
the new DisplayState interface does not contain any host specific display detail, it is an abstraction of the backend display, hence we don't need to memorize the bgr flag in DisplayState.
The frontend must be able to handle a bgr display by itself, in fact sdl is perfectly capable of that;

3) DisplayState interface change
this is the big patch that actually changes the interface;

4) vnc improvements
this patch introduces DisplaySurfaces in vnc, simplifying the code.

5) graphical_console_init change
this is the patch that changes the graphical_console_init function to return an allocated DisplayState instead of a QEMUConsole, as Anthony suggested.
This patch does *not* include any required changes to any devices, these changes come with the two following patches.

6) machine changes
this patch changes the QEMUMachine init functions of all the machine types not to take a DisplayState as an argument because is not needed any more;

7) graphic device changes
this patch updates the graphic device code to use the new graphical_console_init function.

Patch number 5 requires also 6 and 7 to compile.

More comments on single patches.

Cheers,

Stefano

Anthony Liguori

2008-12-18 19:25 UTC #1 lore raw

Quoted 3 lines · Stefano Stabellini

Hi all,
this is the forth update of the "DisplayState interface change" series.

I'm pretty happy with the state of this patch series. I'm going to do thorough testing of each patch but if all goes well, I'd like to apply them. I think there's room to improve things but I think it's good enough now to take into the tree.

Please take a look through these patches and speak up if you think someone needs to be changed before applying them. I'll wait at least until the middle of next week before applying them to give everyone time to look closely.

Regards,

Anthony Liguori

Quoted 49 lines

This update fixes the few remaining issue raised last time.
The series is made of 7 patches:
1) accessors again
some other substitutions in hw/sm501.c;
2) remove bgr
the new DisplayState interface does not contain any host specific display detail, it is an abstraction of the backend display, hence we don't need to memorize the bgr flag in DisplayState.
The frontend must be able to handle a bgr display by itself, in fact sdl is perfectly capable of that;
3) DisplayState interface change
this is the big patch that actually changes the interface;
4) vnc improvements
this patch introduces DisplaySurfaces in vnc, simplifying the code.
5) graphical_console_init change
this is the patch that changes the graphical_console_init function to return an allocated DisplayState instead of a QEMUConsole, as Anthony suggested.
This patch does *not* include any required changes to any devices, these changes come with the two following patches.
6) machine changes
this patch changes the QEMUMachine init functions of all the machine types not to take a DisplayState as an argument because is not needed any more;
7) graphic device changes
this patch updates the graphic device code to use the new graphical_console_init function.
Patch number 5 requires also 6 and 7 to compile.
More comments on single patches.
Cheers,
Stefano

Andreas Färber

2008-12-18 20:09 UTC #2 lore raw

In reply to Stefano Stabellini

Am 18.12.2008 um 20:12 schrieb Stefano Stabellini:

Quoted 9 lines

this is the forth update of the "DisplayState interface change" series.
This update fixes the few remaining issue raised last time.
The series is made of 7 patches:
1) accessors again
some other substitutions in hw/sm501.c;

Quoted 6 lines

2) remove bgr
the new DisplayState interface does not contain any host specific display detail, it is an abstraction of the backend display, hence we don't need to memorize the bgr flag in DisplayState.
The frontend must be able to handle a bgr display by itself, in fact sdl is perfectly capable of that;

Sorry if I've asked that before, but doesn't this (and 3) require changes to non-SDL backends as well, especially Cocoa? The MacBook display appeared to be bgr so that TCX had to be adapted and bgr forcedly set to 1 in cocoa.m (r2974).

Thanks,
Andreas

Quoted 34 lines

3) DisplayState interface change
this is the big patch that actually changes the interface;
4) vnc improvements
this patch introduces DisplaySurfaces in vnc, simplifying the code.
5) graphical_console_init change
this is the patch that changes the graphical_console_init function to return an allocated DisplayState instead of a QEMUConsole, as Anthony suggested.
This patch does *not* include any required changes to any devices, these changes come with the two following patches.
6) machine changes
this patch changes the QEMUMachine init functions of all the machine types not to take a DisplayState as an argument because is not needed any more;
7) graphic device changes
this patch updates the graphic device code to use the new graphical_console_init function.
Patch number 5 requires also 6 and 7 to compile.
More comments on single patches.
Cheers,
Stefano

Stefano Stabellini

author

2008-12-19 11:25 UTC #3 lore raw

Quoted 7 lines · Andreas Färber

Quoted 6 lines
2) remove bgr
the new DisplayState interface does not contain any host specific display detail, it is an abstraction of the backend display, hence we don't need to memorize the bgr flag in DisplayState.
The frontend must be able to handle a bgr display by itself, in fact sdl is perfectly capable of that;

Quoted 5 lines

Sorry if I've asked that before, but doesn't this (and 3) require changes to non-SDL backends as well, especially Cocoa?
The MacBook display appeared to be bgr so that TCX had to be adapted and bgr forcedly set to 1 in cocoa.m (r2974).

I forgot about cocoa, and yes, some changes are required.
I am afraid I don't know much about it and I don't have any MacOS so I may not be the right person do it.

In any case I wrote a simple patch that probably won't even compile but will give you an idea about what has to be done.
I am both attaching the patch to this email and commenting it inline so that I can better explain the reason behind each change.

Quoted 9 lines

(quoted diff) +1 -1

-static void cocoa_resize(DisplayState *ds, int w, int h)
+static void cocoa_resize(DisplayState *ds)

{
    COCOA_DEBUG("qemu_cocoa: cocoa_resize\n");

(quoted diff) +1 -1

-    [cocoaView resizeContentToWidth:w height:h displayState:ds];
+    [cocoaView resizeContentToWidth:ds_get_width(ds) height:ds_get_height(ds) displayState:ds];

}

Quoted 3 lines

static void cocoa_refresh(DisplayState *ds)

The new resize callback interface signature is changed: now the only argument is DisplayState, because is the backend that is supposed to set the the properties in the DisplayState structure.

Quoted 6 lines

(quoted diff) +0 -4

-    screenBuffer = malloc( w * 4 * h );
 
-    ds->data = screenBuffer;
-    ds->linesize =  (w * 4);
-    ds->depth = 32;

Review

Quoted 10 lines

(quoted diff) +1 -9

-    ds->width = w;
-    ds->height = h;
-#ifdef __LITTLE_ENDIAN__
-    ds->bgr = 1;
-#else
-    ds->bgr = 0;
-#endif
-
-    dataProviderRef = CGDataProviderCreateWithData(NULL, screenBuffer, w * 4 * h, NULL);
+    dataProviderRef = CGDataProviderCreateWithData(NULL, ds->data, ds_get_linesize(ds) * ds_get_height(ds), NULL);

Again we don't have to set any property in ds anymore, the backend does that for us.
data is already allocated and width, height, linesize already set.

Quoted 27 lines

int gArgc;
char **gArgv;

(quoted diff) +5 ~~-10~~

@@ -316,18 +317,13 @@
    // draw screen bitmap directly to Core Graphics context
    if (dataProviderRef) {
        CGImageRef imageRef = CGImageCreate(
-            screen.width, //width
-            screen.height, //height
-            screen.bitsPerComponent, //bitsPerComponent
-            screen.bitsPerPixel, //bitsPerPixel
-            (screen.width * 4), //bytesPerRow
-#if __LITTLE_ENDIAN__
-            CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB), //colorspace for OS X >= 10.4
-            kCGImageAlphaNoneSkipLast,
-#else
+            ds_get_width(ds), //width
+            ds_get_height(ds), //height
+            8, //bitsPerComponent
+            ds_get_bits_per_pixel(ds), //bitsPerPixel
+            ds_get_linesize(ds), //bytesPerRow
            CGColorSpaceCreateDeviceRGB(), //colorspace for OS X < 10.4 (actually ppc)
            kCGImageAlphaNoneSkipFirst, //bitmapInfo
-#endif
            dataProviderRef, //provider
            NULL, //decode
            0, //interpolate

Review line 395

Quoted 3 lines

(quoted diff) +0 -0

@@ -397,20 +393,8 @@
        CGDataProviderRelease(dataProviderRef);
    if (screenBuffer)
        free(screenBuffer);

This is the critical part where we ask cocoa to do any needed conversions between the source image format given by the backend and the real display format.
In order to do this we call CGImageCreate with the image source properties from DisplayState.

Quoted 1 line

From a little googling it seems to me that CGImageCreate and

CGContextDrawImage are able to handle the conversions.

Quoted 11 lines

(quoted diff) +0 -6

@@ -983,12 +967,13 @@
    COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
 
    // register vga outpu callbacks
-    ds->dpy_update = cocoa_update;
-    ds->dpy_resize = cocoa_resize;
-    ds->dpy_refresh = cocoa_refresh;
-
-    // give window a initial Size
-    cocoa_resize(ds, 640, 400);

Review line 979

Quoted 10 lines

(quoted diff) +7 -0

+    dcl = qemu_mallocz(sizeof(DisplayChangeListener));
+    if (!dcl)
+        exit(1);
+    dcl->dpy_update = cocoa_update;
+    dcl->dpy_resize = cocoa_resize;
+    dcl->dpy_refresh = cocoa_refresh;
+    register_displaychangelistener(ds, dcl);
 
    // register cleanup function
    atexit(cocoa_cleanup);

These changes are needed to support the new register_displaychangelistener interface, they shouldn't need any explanation.

It could also be useful to look at the changes I made to vnc.c and sdl.c in the third patch.

Please ask if you need anything else.

Cheers,

Stefano Stabellini

cocoa.m +16 ~~-31~~

diff -r 51cf78066aca cocoa.m
@@ -58,6 +58,7 @@
 NSWindow *normalWindow;
 id cocoaView;
 static void *screenBuffer;
+static DisplayChangeListener *dcl;
 
 int gArgc;
 char **gArgv;

@@ -316,18 +317,13 @@
     // draw screen bitmap directly to Core Graphics context
     if (dataProviderRef) {
         CGImageRef imageRef = CGImageCreate(
-            screen.width, //width
-            screen.height, //height
-            screen.bitsPerComponent, //bitsPerComponent
-            screen.bitsPerPixel, //bitsPerPixel
-            (screen.width * 4), //bytesPerRow
-#if __LITTLE_ENDIAN__
-            CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB), //colorspace for OS X >= 10.4
-            kCGImageAlphaNoneSkipLast,
-#else
+            ds_get_width(ds), //width
+            ds_get_height(ds), //height
+            8, //bitsPerComponent
+            ds_get_bits_per_pixel(ds), //bitsPerPixel
+            ds_get_linesize(ds), //bytesPerRow
             CGColorSpaceCreateDeviceRGB(), //colorspace for OS X < 10.4 (actually ppc)
             kCGImageAlphaNoneSkipFirst, //bitmapInfo
-#endif
             dataProviderRef, //provider
             NULL, //decode
             0, //interpolate

@@ -397,20 +393,8 @@
         CGDataProviderRelease(dataProviderRef);
     if (screenBuffer)
         free(screenBuffer);
-    screenBuffer = malloc( w * 4 * h );
 
-    ds->data = screenBuffer;
-    ds->linesize =  (w * 4);
-    ds->depth = 32;
-    ds->width = w;
-    ds->height = h;
-#ifdef __LITTLE_ENDIAN__
-    ds->bgr = 1;
-#else
-    ds->bgr = 0;
-#endif
-
-    dataProviderRef = CGDataProviderCreateWithData(NULL, screenBuffer, w * 4 * h, NULL);
+    dataProviderRef = CGDataProviderCreateWithData(NULL, ds->data, ds_get_linesize(ds) * ds_get_height(ds), NULL);
 
     // update windows
     if (isFullscreen) {

@@ -939,11 +923,11 @@
     [cocoaView displayRect:rect];
 }
 
-static void cocoa_resize(DisplayState *ds, int w, int h)
+static void cocoa_resize(DisplayState *ds)
 {
     COCOA_DEBUG("qemu_cocoa: cocoa_resize\n");
 
-    [cocoaView resizeContentToWidth:w height:h displayState:ds];
+    [cocoaView resizeContentToWidth:ds_get_width(ds) height:ds_get_height(ds) displayState:ds];
 }
 
 static void cocoa_refresh(DisplayState *ds)

@@ -983,12 +967,13 @@
     COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
 
     // register vga outpu callbacks
-    ds->dpy_update = cocoa_update;
-    ds->dpy_resize = cocoa_resize;
-    ds->dpy_refresh = cocoa_refresh;
-
-    // give window a initial Size
-    cocoa_resize(ds, 640, 400);
+    dcl = qemu_mallocz(sizeof(DisplayChangeListener));
+    if (!dcl)
+        exit(1);
+    dcl->dpy_update = cocoa_update;
+    dcl->dpy_resize = cocoa_resize;
+    dcl->dpy_refresh = cocoa_refresh;
+    register_displaychangelistener(ds, dcl);
 
     // register cleanup function
     atexit(cocoa_cleanup);

Stefano Stabellini

author

2009-01-15 17:05 UTC #4 lore raw

Quoted 8 lines · Stefano Stabellini

I forgot about cocoa, and yes, some changes are required.
I am afraid I don't know much about it and I don't have any MacOS so I may not be the right person do it.
In any case I wrote a simple patch that probably won't even compile but will give you an idea about what has to be done.
I am both attaching the patch to this email and commenting it inline so that I can better explain the reason behind each change.

Can someone please take a look at the cocoa patch I sent and fix it? It shouldn't take long at all.

Or you could just apply the series and wait for someone to fix the cocoa interface later :)

I think this patch series has already been delayed enough...

Anthony Liguori

2009-01-15 17:20 UTC #5 lore raw

Quoted 13 lines · Stefano Stabellini

Quoted 9 lines · Stefano Stabellini
I forgot about cocoa, and yes, some changes are required.
I am afraid I don't know much about it and I don't have any MacOS so I may not be the right person do it.
In any case I wrote a simple patch that probably won't even compile but will give you an idea about what has to be done.
I am both attaching the patch to this email and commenting it inline so that I can better explain the reason behind each change.

Quoted 6 lines

Can someone please take a look at the cocoa patch I sent and fix it? It shouldn't take long at all.
Or you could just apply the series and wait for someone to fix the cocoa interface later :)

If noone volunteers to do the cocoa patch, I'll do this.

Quoted 2 lines

I think this patch series has already been delayed enough...

Yes, I'm pretty behind right now but I'm trying to catch up. Your series is high on my TODO.

Regards,

Anthony Liguori

Quoted 2 lines

Andreas Färber

2009-01-15 17:23 UTC #7 lore raw

In reply to Stefano Stabellini

Hi,

Am 15.01.2009 um 18:03 schrieb Stefano Stabellini:

Quoted 11 lines

Quoted 8 lines · Stefano Stabellini
I forgot about cocoa, and yes, some changes are required.
I am afraid I don't know much about it and I don't have any MacOS so I may not be the right person do it.
In any case I wrote a simple patch that probably won't even compile but will give you an idea about what has to be done.
I am both attaching the patch to this email and commenting it inline so that I can better explain the reason behind each change.

Quoted 7 lines

Can someone please take a look at the cocoa patch I sent and fix it? It shouldn't take long at all.
Or you could just apply the series and wait for someone to fix the cocoa interface later :)
I think this patch series has already been delayed enough...

Sorry, I am currently in the process of moving. Do you have a Git repo I could pull from, to speed things up?

Andreas

Stefano Stabellini

author

2009-01-15 17:36 UTC #6 lore raw

In reply to Anthony Liguori

Quoted 21 lines · Anthony Liguori

Quoted 13 lines · Stefano Stabellini
Quoted 9 lines · Stefano Stabellini
I forgot about cocoa, and yes, some changes are required.
I am afraid I don't know much about it and I don't have any MacOS so I may not be the right person do it.
In any case I wrote a simple patch that probably won't even compile but will give you an idea about what has to be done.
I am both attaching the patch to this email and commenting it inline so that I can better explain the reason behind each change.
Quoted 6 lines
Can someone please take a look at the cocoa patch I sent and fix it? It shouldn't take long at all.
Or you could just apply the series and wait for someone to fix the cocoa interface later :)

Quoted 8 lines

If noone volunteers to do the cocoa patch, I'll do this.
Quoted 2 lines
I think this patch series has already been delayed enough...
Yes, I'm pretty behind right now but I'm trying to catch up. Your series is high on my TODO.

Thanks, appreciated.

Stefano Stabellini

author

2009-01-15 17:45 UTC #8 lore raw

In reply to Andreas Färber

Quoted 18 lines · Andreas Färber

Hi,
Am 15.01.2009 um 18:03 schrieb Stefano Stabellini:
Quoted 14 lines
Quoted 8 lines · Stefano Stabellini
I forgot about cocoa, and yes, some changes are required.
I am afraid I don't know much about it and I don't have any MacOS so I may not be the right person do it.
In any case I wrote a simple patch that probably won't even compile but will give you an idea about what has to be done.
I am both attaching the patch to this email and commenting it inline so that I can better explain the reason behind each change.
Can someone please take a look at the cocoa patch I sent and fix it? It shouldn't take long at all.

Quoted 8 lines

Quoted 4 lines
Or you could just apply the series and wait for someone to fix the cocoa interface later :)
I think this patch series has already been delayed enough...
Sorry, I am currently in the process of moving. Do you have a Git repo I could pull from, to speed things up?

I am afraid not: I am using a mercurial patchqueue and currently is not even public.
However if you have issues applying my patches I can send another update of the series, rediffed against the current qemu.