Okay, I've spent the last days looking into this matter and I'd like to suggest a way to get it started. So. This is the plan:
1. In winex11.drv:
-INT X11DRV_LockDIBSection(X11DRV_PDEVICE *physDev, INT req, BOOL lossy) +HBITMAP X11DRV_LockDIBSection(X11DRV_PDEVICE *physDev, INT req, BOOL force)
Lossy isn't used anywhere for LockDIBSection anyway. Force means that the function will only lock if the DIB is already in AppMod. The returned bitmap is ofcourse physDev->bitmap->hbitmap.
Rationale: If you lock the DIB to write on it it makes sense that the function actually provides you with that DIB.
2. Export LockDIBSection/Unlock to gdi32.
Adding more exports is not nice but there really is no way around that, right?
3. Move dc->funcs to dc->physDev->funcs. Many changes but mostly mechanical work.
Rationale: This really belongs there. And I need it. :)
4. Now we write dibdrv.c for now just containing DIBDRV_Install and DIBDRV_Remove. That function will go through the physDev->funcs list and overwrite each function pointer which is actually implemented with DIBDRV_PutPixel(), whatever.
DIBDRV_Install/DIBDRV_Remove will be called from BITMAP_SelectObject() when we switch from non-DIB to DIB vice versa.
Note that we can't use DRIVER_load_driver here because of the wanted "forward to original driver when not implemented".
For this we will need to extend the "for DIB objects" part in BITMAPOBJ by
const DC_FUNCTIONS *orig_funcs; DC_FUNCTIONS local_funcs; where orig_funcs to the old physDev->funcs and the new physDev->funcs points to &bmp->local_funcs.
5. In dibdrv.c we get us:
enum { DIBDRV_MIXED, /* Choose driver depending on current AppMod */ DIBDRV_ALWAYS, /* Always use DIBDRV (unless function not implemented) */ DIBDRV_NEVER /* Always use DC driver */ } DIB_Mode = DIBDRV_MIXED;
and DIB_Lock(PHYSDEV physDev) / DIB_Unlock(PHYSDEV). Now, here comes the reason we need the HBITMAP from LockDIBSection() and funcs inside PHYSDEV:
Since we don't have the DC here we have no way of calling LockDIBSection(), forwarding to the original driver if NEVER/MIXED or writing to the actual DIB in case of MIXED/ALWAYS.
6. From this point we can start implementing one function at a time, touching nothing but dibdrv.c.
So far so good. I did all those steps (except 3., I hacked around that in my local tree) in a clean way and it works nice (for 6. I implemented SetPixel() and tried that with a test-app).
I attached patches for the steps 1 2 and 4 so you can see where this is going.
Comments?
Felix
Oops. Forgot the patches. Here they are.
And the very second I sent the last E-Mail the DIB engine work got accepted as a SoC project (by someone else)... I've no idea what this means to me. Someone clarify please. :)
As a returning GSoC student, I had asked about this last year:
It's primarily the student's responsibility to work out the conflicts, but it'd be best for you two to talk to one another. The student's obligation is do do what they said they would, so if you complete their proposal for them it doesn't count as them doing it - they'd have to write an implementation of their own, even if it wasn't accepted, which would be a waste.
However, as long as their mentor is fine with it, it's acceptable to modify their goals - so perhaps they can add more features or otherwise produce a better DIB engine by working with you and the code you've written. If the mentor thinks they've done enough work over the summer, they'll be paid - even if it's different than what they originally proposed.
--Matt Finnicum
On 4/11/07, Felix Nawothnig flexo@holycrap.org wrote:
Oops. Forgot the patches. Here they are.
And the very second I sent the last E-Mail the DIB engine work got accepted as a SoC project (by someone else)... I've no idea what this means to me. Someone clarify please. :)
On 4/11/07, Matt Finnicum mattfinn@gmail.com wrote:
As a returning GSoC student, I had asked about this last year:
It's primarily the student's responsibility to work out the conflicts, but it'd be best for you two to talk to one another. The student's obligation is do do what they said they would, so if you complete their proposal for them it doesn't count as them doing it - they'd have to write an implementation of their own, even if it wasn't accepted, which would be a waste.
However, as long as their mentor is fine with it, it's acceptable to modify their goals - so perhaps they can add more features or otherwise produce a better DIB engine by working with you and the code you've written. If the mentor thinks they've done enough work over the summer, they'll be paid - even if it's different than what they originally proposed.
--Matt Finnicum
On 4/11/07, Felix Nawothnig flexo@holycrap.org wrote:
Oops. Forgot the patches. Here they are.
And the very second I sent the last E-Mail the DIB engine work got accepted as a SoC project (by someone else)... I've no idea what this means to me. Someone clarify please. :)
That'd be me apparently.
Where did you send the patches? Maybe wine-patches? I haven't seen it yet. Honestly there have been attempts before to start the DIB engine and I've seen them already. I don't think it will affect what I do, as nothing has really been accepted yet. But I suggest you find out who my mentor is and show it to him first. See if what you have done can somehow be worked into the mentoring process (hopefully it's good enough :D). I think it's early enough to work it out.
Jesse
On 4/11/07, Jesse Allen the3dfxdude@gmail.com wrote:
That'd be me apparently.
http://code.google.com/soc/wine/about.html
Jesse
The commit 9badfb50cf38e930372d721450581f4597af86c2 is causing an application I am working on (ichitaro) to behave very poorly. It cause the file->open dialog to repaint incorrectly (screen shots attached)
I downloaded utorrent to see the original bug and I do not see the flickering that you where trying to fix. Since you fixed this recently do you have some insight on why the UpdateWindow is causing this issue?
-aric
Felix Nawothnig flexo@holycrap.org writes:
Export LockDIBSection/Unlock to gdi32.
Adding more exports is not nice but there really is no way around that, right?
No, LockDIBSection is a driver internal detail, gdi32 has no business knowing about this.
Move dc->funcs to dc->physDev->funcs. Many changes but mostly mechanical work.
Rationale: This really belongs there. And I need it. :)
No it doesn't, physDev is an opaque structure as far as gdi32 is concerned. Data needed by gdi32 belongs in the DC structure.
Now we write dibdrv.c for now just containing DIBDRV_Install and DIBDRV_Remove. That function will go through the physDev->funcs list and overwrite each function pointer which is actually implemented with DIBDRV_PutPixel(), whatever.
DIBDRV_Install/DIBDRV_Remove will be called from BITMAP_SelectObject() when we switch from non-DIB to DIB vice versa.
Note that we can't use DRIVER_load_driver here because of the wanted "forward to original driver when not implemented".
For this we will need to extend the "for DIB objects" part in BITMAPOBJ by
const DC_FUNCTIONS *orig_funcs; DC_FUNCTIONS local_funcs;
where orig_funcs to the old physDev->funcs and the new physDev->funcs points to &bmp->local_funcs.
You certainly don't want to store the full function table in the BITMAPOBJ, it will be the same for all bitmaps. All you need is one function table for the DIB driver and one for the normal graphics driver. Forwarding to the graphics driver can be done privately in the DIB driver, gdi32 doesn't need to know about it. And you probably want a separate physDev pointer for it, you'll need to maintain state for DIBs too.
Alexandre Julliard wrote:
Export LockDIBSection/Unlock to gdi32.
Adding more exports is not nice but there really is no way around that, right?
No, LockDIBSection is a driver internal detail, gdi32 has no business knowing about this.
In my code the call to LockDIBSection serves two purposes: to lock the DIB section (duh) and to query whether we are in AppMod or GdiMod. The former could be done by triggering an exception and letting the driver catch it that but I'd think that's rather expensive.
The latter can't be done at all if this is an "driver internal detail" so we'd have to always use the DIBDRV implementation when it is there.
This would most likely mean some pretty serious performance regressions until we implemented all used functions (in which case we could say the export is only temporary).
Since we'll at some point probably have all functions implemented the DIB code in the driver will be dead code and it would have to be removed anyway. So I don't see the point of trying to keep a hack clean.
Any another issue: There are cases where we really want to have the GDI operation be done server-side, regardless of whether we have a client-side implementation or not (http://www.winehq.com/pipermail/wine-devel/2007-February/053993.html - I'd agree with that).
So I really think we need that export.
Move dc->funcs to dc->physDev->funcs. Many changes but mostly mechanical work.
Rationale: This really belongs there. And I need it. :)
No it doesn't, physDev is an opaque structure as far as gdi32 is concerned. Data needed by gdi32 belongs in the DC structure.
Well, yes, it's an opaque structure. With the function pointers added to physDev it would no longer be an opaque structure. So?
We'd add a pointer to physDev pointing to an opaque structure of course.
My point is that the addresses of the driver functions are not "data needed by gdi32" in the strictest sense. Those addresses are defined by the driver and the functions live in the driver, so I think it makes sense if the driver would initialize the table, not gdi32. Doing this by GetProcAddress() in gdi32/driver.c is not a very clean design. Really.
You certainly don't want to store the full function table in the BITMAPOBJ, it will be the same for all bitmaps. All you need is one
Will it be the same? I'm not sure if there are situations where an application has multiple drivers providing memory DCs loaded.
function table for the DIB driver and one for the normal graphics driver. Forwarding to the graphics driver can be done privately in the DIB driver, gdi32 doesn't need to know about it. And you probably
Yes, it's possible. But we'd probably have to write a wrapper for every GDI operation. Wouldn't be hard - but it's a lot of code which can be easily avoided by just 10 lines outside of dibdrv (in gdi32/bitmap.c). I don't really see the point.
want a separate physDev pointer for it, you'll need to maintain state for DIBs too.
Well, we have a couple of DIB-dependent members in BITMAPOBJ - I'd put a pointer to this state there. I admit this isn't really clean though.
Felix
Felix Nawothnig [mailto:flexo@holycrap.org]
Okay, I've spent the last days looking into this matter and I'd like to suggest a way to get it started. So. This is the plan:
- In winex11.drv:
-INT X11DRV_LockDIBSection(X11DRV_PDEVICE *physDev, INT req, BOOL lossy) +HBITMAP X11DRV_LockDIBSection(X11DRV_PDEVICE *physDev, INT req, BOOL +force)
Lossy isn't used anywhere for LockDIBSection anyway. Force means that the function will only lock if the DIB is already in AppMod. The returned bitmap is ofcourse physDev->bitmap->hbitmap.
Rationale: If you lock the DIB to write on it it makes sense that the function actually provides you with that DIB.
I had actually a bit a different idea but didn't get around to try it yet. I was thinking about adding an extra value to the DIB section sync state such as DIB_Status_Conf. With this X11DRV_DIB_Coerce would decide based on a configuration setting if it should do DIB_Status_GdiMod (DIBDRV_NEVER in point 5.), DIB_Status_AppMod (DIBDRV_ALWAYS), or DIB_Status_AppMod if the DIB is already in DIB_Status_AppMod (DIBDRV_MIXED). The return value would someow indicate to that driver function if it should just simply return with a failure leaving the rest to GDI32 to deal with or do for the time being the actual work as it does now.
GDI32 would on driver failure (or non-existing driver function) invoke the according DIBDRV function for non-meta DCs. A non-existing driver function would still work thanks to the exception handling for DIBs being accessed in application mode, although it is not the preferd way to deal with this until the DIB engine is fully functional (and the DIB handling could then be consequently removed from x11drv entirely).
This would reduce the modifications to x11drv to a minimum and also GDI32 wouldn't need much changes at all. Adding a new DIBDRV function would be a change in GDI32 to add the DIBDRV call on failure of the driver function and then switching the DIB_Status value in that driver function to DIB_Status_Conf instead of DIB_Status_GdiMod.
Export LockDIBSection/Unlock to gdi32.
Adding more exports is not nice but there really is no way around that, right?
That and a lot of the other points wouldn't be necessary with above approach.
But there might be another complication with this I haven't seen yet.
Rolf Kalbermatter
On 4/11/07, Felix Nawothnig flexo@holycrap.org wrote:
Okay, I've spent the last days looking into this matter and I'd like to suggest a way to get it started. So. This is the plan:
Now I see the emails. I found it in the gmail trash. I don't know why it was there. Anyway yesterday was a long day--driving 600 miles. I'll wait a little bit longer so it gets disected more before reading it :D BTW, thanks for your last email.
-
Alexandre Julliard -
Aric Stewart -
Felix Nawothnig -
Jesse Allen -
Matt Finnicum -
Rolf Kalbermatter -
Tom Wickline