Re: [2/2] gdiplus: Implement GdipBeginContainer2 and GdipEndContainer
Andrew Eikum wrote:
--- dlls/gdiplus/gdiplus_private.h | 3 + dlls/gdiplus/graphics.c | 154 ++++++++++++++++++++++++++++++++++++++-- include/gdiplusflat.h | 1 + 3 files changed, 152 insertions(+), 6 deletions(-)
This looks wrong for me cause I don't think you could pass container id created with one Graphics to Graphics.EndContainer() of another. I've just checked it and it doesn't return any error code, but generated container ids aren't the same when you call BeginContainer() for e.g. two objects as sequent steps. It looks like on native these value are module unique, some kind of handle table needed. This kills the first problem I described - passing strange value to another Graphics. Also note that Save()/Restore() functionality uses the same stack with container calls (at least msdn tells us that), for that Save/Restore we already have a test: --- static void test_save_restore(void) .... /* The same state value should never be returned twice. */ todo_wine check_no_duplicates(state_log); .... --- It clearly says that no duplicates allowed, even after multiple object deletion - it checks for values returned during the whole test.
Nikolay Sivov wrote:
Andrew Eikum wrote:
--- dlls/gdiplus/gdiplus_private.h | 3 + dlls/gdiplus/graphics.c | 154 ++++++++++++++++++++++++++++++++++++++-- include/gdiplusflat.h | 1 + 3 files changed, 152 insertions(+), 6 deletions(-)
This looks wrong for me cause I don't think you could pass container id created with one Graphics to Graphics.EndContainer() of another. I've just checked it and it doesn't return any error code, but generated container ids aren't the same when you call BeginContainer() for e.g. two objects as sequent steps. It looks like on native these value are module unique, some kind of handle table needed. This kills the first problem I described - passing strange value to another Graphics.
Also note that Save()/Restore() functionality uses the same stack with container calls (at least msdn tells us that), for that Save/Restore we already have a test: --- static void test_save_restore(void) .... /* The same state value should never be returned twice. */ todo_wine check_no_duplicates(state_log); .... ---
It clearly says that no duplicates allowed, even after multiple object deletion - it checks for values returned during the whole test.
I agree that the behavior of the container IDs doesn't match native. However, do we really care to duplicate that behavior? I can't see any documentation in MSDN that says container IDs must be unique across either graphics objects or within a single graphics object. I can't imagine any application depending on the uniqueness of container IDs; if they try to pass a container for graphics1 to graphics2, nothing happens, so there's no behavior to depend on. Ditto for invalid or already-used containers. The only scenario I can think of where the uniqueness of container IDs would be a dependency would be an application error calling EndContainer() with invalid values. I don't know if it's worth the effort of making a handle table and whatnot to work around an application glitch that might not even exist. So, I would argue against the validity of the test you quote. Why should the same value not be returned twice? Andrew
On Fri, Jul 3, 2009 at 10:00 AM, Andrew Eikum<andrew(a)brightnightgames.com> wrote:
I agree that the behavior of the container IDs doesn't match native. However, do we really care to duplicate that behavior? I can't see any documentation in MSDN that says container IDs must be unique across either graphics objects or within a single graphics object.
I can't imagine any application depending on the uniqueness of container IDs; if they try to pass a container for graphics1 to graphics2, nothing happens, so there's no behavior to depend on. Ditto for invalid or already-used containers. The only scenario I can think of where the uniqueness of container IDs would be a dependency would be an application error calling EndContainer() with invalid values. I don't know if it's worth the effort of making a handle table and whatnot to work around an application glitch that might not even exist.
So, I would argue against the validity of the test you quote. Why should the same value not be returned twice?
+1 If an application does need the ID's to be more like Windows, the code can be changed to account for it. We don't have to match every quirk of Windows the first time something is implemented. -- Vincent Povirk
Vincent Povirk wrote:
On Fri, Jul 3, 2009 at 10:00 AM, Andrew Eikum<andrew(a)brightnightgames.com> wrote:
I agree that the behavior of the container IDs doesn't match native. However, do we really care to duplicate that behavior? I can't see any documentation in MSDN that says container IDs must be unique across either graphics objects or within a single graphics object.
I can't imagine any application depending on the uniqueness of container IDs; if they try to pass a container for graphics1 to graphics2, nothing happens, so there's no behavior to depend on. Ditto for invalid or already-used containers. The only scenario I can think of where the uniqueness of container IDs would be a dependency would be an application error calling EndContainer() with invalid values. I don't know if it's worth the effort of making a handle table and whatnot to work around an application glitch that might not even exist.
So, I would argue against the validity of the test you quote. Why should the same value not be returned twice?
+1
If an application does need the ID's to be more like Windows, the code can be changed to account for it. We don't have to match every quirk of Windows the first time something is implemented.
Exactly. I'm not talking about that. Allocating handle on Begin..() or Save..() and releasing it on End...() or Restore...() should be enough. After that we will probably never want to change that but for a first time it's necessary I think.
Nikolay Sivov wrote:
Vincent Povirk wrote:
On Fri, Jul 3, 2009 at 10:00 AM, Andrew Eikum<andrew(a)brightnightgames.com> wrote:
I agree that the behavior of the container IDs doesn't match native. However, do we really care to duplicate that behavior? I can't see any documentation in MSDN that says container IDs must be unique across either graphics objects or within a single graphics object.
I can't imagine any application depending on the uniqueness of container IDs; if they try to pass a container for graphics1 to graphics2, nothing happens, so there's no behavior to depend on. Ditto for invalid or already-used containers. The only scenario I can think of where the uniqueness of container IDs would be a dependency would be an application error calling EndContainer() with invalid values. I don't know if it's worth the effort of making a handle table and whatnot to work around an application glitch that might not even exist.
So, I would argue against the validity of the test you quote. Why should the same value not be returned twice?
+1
If an application does need the ID's to be more like Windows, the code can be changed to account for it. We don't have to match every quirk of Windows the first time something is implemented.
Exactly. I'm not talking about that. Allocating handle on Begin..() or Save..() and releasing it on End...() or Restore...() should be enough. After that we will probably never want to change that but for a first time it's necessary I think.
Okay, how about this patch? It creates a list of available IDs and assigns/releases them as needed. Seems to work fine from my testing. It also adds some fixes for things Vincent pointed out last night. Can you guys check if everything is done correctly from a C/Wine perspective? I notice that the objects in availableGCIDs are never explicitly freed; can we just let this happen during cleanup as the program exits?
Andrew Eikum wrote:
Nikolay Sivov wrote:
Vincent Povirk wrote:
On Fri, Jul 3, 2009 at 10:00 AM, Andrew Eikum<andrew(a)brightnightgames.com> wrote:
I agree that the behavior of the container IDs doesn't match native. However, do we really care to duplicate that behavior? I can't see any documentation in MSDN that says container IDs must be unique across either graphics objects or within a single graphics object.
I can't imagine any application depending on the uniqueness of container IDs; if they try to pass a container for graphics1 to graphics2, nothing happens, so there's no behavior to depend on. Ditto for invalid or already-used containers. The only scenario I can think of where the uniqueness of container IDs would be a dependency would be an application error calling EndContainer() with invalid values. I don't know if it's worth the effort of making a handle table and whatnot to work around an application glitch that might not even exist.
So, I would argue against the validity of the test you quote. Why should the same value not be returned twice?
+1
If an application does need the ID's to be more like Windows, the code can be changed to account for it. We don't have to match every quirk of Windows the first time something is implemented.
Exactly. I'm not talking about that. Allocating handle on Begin..() or Save..() and releasing it on End...() or Restore...() should be enough. After that we will probably never want to change that but for a first time it's necessary I think.
Okay, how about this patch? It creates a list of available IDs and assigns/releases them as needed. Seems to work fine from my testing. It also adds some fixes for things Vincent pointed out last night.
Can you guys check if everything is done correctly from a C/Wine perspective? I notice that the objects in availableGCIDs are never explicitly freed; can we just let this happen during cleanup as the program exits? Appears it didn't attach for some reason? Attempting to re-send, also available at http://www.brightnightgames.com/wine/GdipBeginContainer2.patch
I don't think it's a good idea to rely on gdiplus functions being called from only one thread. Most of gdiplus currently is not thread-safe (maybe it should be), but it will work as long as a single object is only used from a single thread at a time. Adding global state makes things more complex, and that's part of the reason I think it's better not to put it in the initial implementation. If you do add it, I think you should do it in a separate patch. (I must confess my implementation of GdipNewInstalledFontCollection breaks the little thread-safety we have, but the race condition happens only the first time it's called.) You can clean up global data in DllMain in the PROCESS_DETACH case. -- Vincent Povirk
Andrew Eikum wrote:
Nikolay Sivov wrote:
Andrew Eikum wrote:
--- dlls/gdiplus/gdiplus_private.h | 3 + dlls/gdiplus/graphics.c | 154 ++++++++++++++++++++++++++++++++++++++-- include/gdiplusflat.h | 1 + 3 files changed, 152 insertions(+), 6 deletions(-)
This looks wrong for me cause I don't think you could pass container id created with one Graphics to Graphics.EndContainer() of another. I've just checked it and it doesn't return any error code, but generated container ids aren't the same when you call BeginContainer() for e.g. two objects as sequent steps. It looks like on native these value are module unique, some kind of handle table needed. This kills the first problem I described - passing strange value to another Graphics.
Also note that Save()/Restore() functionality uses the same stack with container calls (at least msdn tells us that), for that Save/Restore we already have a test: --- static void test_save_restore(void) .... /* The same state value should never be returned twice. */ todo_wine check_no_duplicates(state_log); .... ---
It clearly says that no duplicates allowed, even after multiple object deletion - it checks for values returned during the whole test.
I agree that the behavior of the container IDs doesn't match native. However, do we really care to duplicate that behavior? I can't see any documentation in MSDN that says container IDs must be unique across either graphics objects or within a single graphics object. It doesn't matter is it explicitly documented or not. We should care only about how it really works. I can't imagine any application depending on the uniqueness of container IDs; if they try to pass a container for graphics1 to graphics2, nothing happens, so there's no behavior to depend on. If you create container c1 for graphics1 (0 numbered), then c2 for graphics2 (0 numbered too), then pop container for graphics2 with c1 value (c1 == c2) you implementation will successfully restore graphics2 to previous state. This doesn't happen on native - it will return Ok status, but status will be retained. This is a behavior we should depend on. Ditto for invalid or already-used containers. The only scenario I can think of where the uniqueness of container IDs would be a dependency would be an application error calling EndContainer() with invalid values. I don't know if it's worth the effort of making a handle table and whatnot to work around an application glitch that might not even exist. What you mean it might not? If somebody (e.g. me) will write a test for that will it exist then?
So, I would argue against the validity of the test you quote. Why should the same value not be returned twice? Agreed, test isn't so obvious cause it shows that id value doesn't reoccur even after releasing containers. But test passes well on Windows, and only this matters here.
To simplify this we possibly could not to reproduce this exactly, but only guaranty ID uniqueness for currently valid Graphics objects in a process wide way. After that we could consider making this test pass as a second step to get closer to native behavior.
participants (3)
-
Andrew Eikum -
Nikolay Sivov -
Vincent Povirk