Re: Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Stefan Dösinger]

Am Montag, 2. Juni 2008 06:50:15 schrieb Vitaliy Margolen:
> This reverts commit 2d0016c5bc872afb562727278cfd341cea182600.
> Partially fixes bug 11584.

I think we can revert that patch(ie, apply this patch). I haven't had time to 
look over the code yet, but I can't think of any problems this would cause. 
If it does cause problems, there's a bug elsewhere.

I think this patch was needed to work around some surface management bugs 
before the ModifyLocation / LoadLocation methods were introduced

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Alexandre Julliard]

Stefan Dösinger  writes:
> Am Montag, 2. Juni 2008 06:50:15 schrieb Vitaliy Margolen:
> > This reverts commit 2d0016c5bc872afb562727278cfd341cea182600.
> > Partially fixes bug 11584.
>
> I think we can revert that patch(ie, apply this patch). I haven't had time to 
> look over the code yet, but I can't think of any problems this would cause.

It does cause problems:

../../../tools/runtest -q -P wine -M d3d8.dll -T ../../.. -p d3d8_test.exe.so visual.c && touch visual.ok
visual.c:709: Test failed: Offscreen failed: Got color 0x00000000, expected 0x00ffffff.
visual.c:712: Test failed: Offscreen failed: Got color 0x00000000, expected 0x00ff00ff.
visual.c:890: Test failed: SRCALPHA on texture returned color 00000000, expected bar
visual.c:897: Test failed: DSTALPHA on texture returned color 00000000, expected foo
make[2]: *** [visual.ok] Error 4

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Zachary Goldberg]

On Tue, Jun 3, 2008 at 5:49 AM, Alexandre Julliard  wrote:
> Stefan Dösinger  writes:
> > Am Montag, 2. Juni 2008 06:50:15 schrieb Vitaliy Margolen:
> > > This reverts commit 2d0016c5bc872afb562727278cfd341cea182600.
> > > Partially fixes bug 11584.
> >
> > I think we can revert that patch(ie, apply this patch). I haven't had time to
> > look over the code yet, but I can't think of any problems this would cause.
>
> It does cause problems:
>
> ../../../tools/runtest -q -P wine -M d3d8.dll -T ../../.. -p d3d8_test.exe.so visual.c && touch visual.ok
> visual.c:709: Test failed: Offscreen failed: Got color 0x00000000, expected 0x00ffffff.
> visual.c:712: Test failed: Offscreen failed: Got color 0x00000000, expected 0x00ff00ff.
> visual.c:890: Test failed: SRCALPHA on texture returned color 00000000, expected bar
> visual.c:897: Test failed: DSTALPHA on texture returned color 00000000, expected foo
> make[2]: *** [visual.ok] Error 4
>
> --
> Alexandre Julliard
> [e-mail]

Proposal: Revert the commit referenced in bug #11584 before this
Friday, so it can be tested in rc5.  I think the weight of the
following is greater than breaking a test (making the test a todo for
now would be a good idea I think).  Also, there's no reason to keep
the patch reverted post 1.0 as devs continue to make progress towards
a proper solution.

First, the list of applications thought to be broken by the patch as
documented in bugzilla (more may exist):

Call of Duty 4 (11584 itself)
Psychonauts  (11630)
Fifa 2006 world
Supreme Commander
Oblivion (Maybe?)

So why not wait for 1.0.1?  Of the remaining 1.0 bugs this is the
biggest one known to have broken a large number of *popular* games.
In addition as soon as 1.0 is released the floodgates will open for
patches and wine may not be stable enough to release again for a few
weeks.  This leaves all of the gamers who downloaded 1.0 [for these
games] out in the cold without a package which includes a fix and a
sour taste in their mouths.

Steve D. has been making good progress towards finding a proper fix
for this bug; however by no means should we put pressure on him to
have this bug fixed by Friday.  Thus I propose we revert the initial
commit for now, test it during rc5 to make sure things work again, and
then allow Steve to properly fix the bug post 1.0.

-Zach

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Austin English]

On Wed, Jun 11, 2008 at 5:59 PM, Zachary Goldberg  wrote:
> Proposal: Revert the commit referenced in bug #11584 before this
> Friday, so it can be tested in rc5.  I think the weight of the
> following is greater than breaking a test (making the test a todo for
> now would be a good idea I think).  Also, there's no reason to keep
> the patch reverted post 1.0 as devs continue to make progress towards
> a proper solution.
>
> First, the list of applications thought to be broken by the patch as
> documented in bugzilla (more may exist):
>
> Call of Duty 4 (11584 itself)
> Psychonauts  (11630)
> Fifa 2006 world
> Supreme Commander
> Oblivion (Maybe?)
>
> So why not wait for 1.0.1?  Of the remaining 1.0 bugs this is the
> biggest one known to have broken a large number of *popular* games.
> In addition as soon as 1.0 is released the floodgates will open for
> patches and wine may not be stable enough to release again for a few
> weeks.  This leaves all of the gamers who downloaded 1.0 [for these
> games] out in the cold without a package which includes a fix and a
> sour taste in their mouths.
>
> Steve D. has been making good progress towards finding a proper fix
> for this bug; however by no means should we put pressure on him to
> have this bug fixed by Friday.  Thus I propose we revert the initial
> commit for now, test it during rc5 to make sure things work again, and
> then allow Steve to properly fix the bug post 1.0.
>
> -Zach

+1

Stefan/Roderick, any idea how long a patch would take to write to
either fix this properly or revert the broken parts of the patch and
adjust the tests accordingly?

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Vitaliy]

Austin English wrote:
> On Wed, Jun 11, 2008 at 5:59 PM, Zachary Goldberg  wrote:
> > Proposal: Revert the commit referenced in bug #11584 before this
> > Friday, so it can be tested in rc5.  I think the weight of the
> > following is greater than breaking a test (making the test a todo for
> > now would be a good idea I think).  Also, there's no reason to keep
> > the patch reverted post 1.0 as devs continue to make progress towards
> > a proper solution.
> >
> > First, the list of applications thought to be broken by the patch as
> > documented in bugzilla (more may exist):
> >
> > Call of Duty 4 (11584 itself)
> > Psychonauts  (11630)
> > Fifa 2006 world
> > Supreme Commander
> > Oblivion (Maybe?)
> >
> > So why not wait for 1.0.1?  Of the remaining 1.0 bugs this is the
> > biggest one known to have broken a large number of *popular* games.
> > In addition as soon as 1.0 is released the floodgates will open for
> > patches and wine may not be stable enough to release again for a few
> > weeks.  This leaves all of the gamers who downloaded 1.0 [for these
> > games] out in the cold without a package which includes a fix and a
> > sour taste in their mouths.
> >
> > Steve D. has been making good progress towards finding a proper fix
> > for this bug; however by no means should we put pressure on him to
> > have this bug fixed by Friday.  Thus I propose we revert the initial
> > commit for now, test it during rc5 to make sure things work again, and
> > then allow Steve to properly fix the bug post 1.0.
> >
> > -Zach
>
> +1
>
> Stefan/Roderick, any idea how long a patch would take to write to
> either fix this properly or revert the broken parts of the patch and
> adjust the tests accordingly?

IMHO this is too little too late. There are at least 2 more calls to Preload 
that have exactly the same affect on some of my test games. It sure would be 
nice to "fix" at least one case which looks to be common for number of games.

Vitaliy.

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Stefan Dösinger]

> IMHO this is too little too late. There are at least 2 more calls to
> Preload
> that have exactly the same affect on some of my test games. It sure
> would be
> nice to "fix" at least one case which looks to be common for number of
> games.

I am aware of one. Where is the other one?

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Vitaliy Margolen]

Stefan Dösinger wrote:
> > IMHO this is too little too late. There are at least 2 more calls to
> > Preload
> > that have exactly the same affect on some of my test games. It sure
> > would be
> > nice to "fix" at least one case which looks to be common for number of
> > games.
>
> I am aware of one. Where is the other one?

Two Preload calls in FindContext. And bug 13079: 
http://bugs.winehq.org/show_bug.cgi?id=13079

Vitaliy.

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Stefan Dösinger]

> Two Preload calls in FindContext. And bug 13079:
> http://bugs.winehq.org/show_bug.cgi?id=13079

Ah, I think 13079 is an entirely different bug. Vertex buffers aren't
involved in offscreen rendering readback, nor in render target selection.
(Currently. D3D10 changes that)

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Vitaliy Margolen]

Stefan Dösinger wrote:
> > Two Preload calls in FindContext. And bug 13079:
> > http://bugs.winehq.org/show_bug.cgi?id=13079
>
> Ah, I think 13079 is an entirely different bug. Vertex buffers aren't
> involved in offscreen rendering readback, nor in render target selection.
> (Currently. D3D10 changes that)

It could be different - I can't tell for sure. This game doesn't produce any 
reasonable crash dump, just a message that it crashed (so not even winedbg's 
output). However the problems start with call to... Preload.

Vitaliy.

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Stefan Dösinger]

> It could be different - I can't tell for sure. This game doesn't
> produce any
> reasonable crash dump, just a message that it crashed (so not even
> winedbg's
> output). However the problems start with call to... Preload.

Vertexbuffer::PreLoad has nothing in common with Surface::PreLoad or
Texture::PreLoad, other than that the name(since they inherit from
Resource::PreLoad, which is a No-op)

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Stefan Dösinger]

> Two Preload calls in FindContext

I just sent 3 patches in an attempt to fix the bug. Please give them a try.
The tests work for me, and I'll test my other games for regressions. Please
also see if you see the ERR("Wine 1.0 safety path hit\n"); error message.

Stefan

PS: Does Outlook set the In-Reply-To tags properly? Are my mails readable
properly in any client that supports a threaded mail view?

PS2: Are the patches sent properly?

Revert "wined3d: Preload target in ActivateContext() for ORM_BACKBUFFER/ORM_PBUFFER."

[Stefan Dösinger]

> Proposal: Revert the commit referenced in bug #11584 before this
> Friday, so it can be tested in rc5.  I think the weight of the
> following is greater than breaking a test (making the test a todo for
> now would be a good idea I think).

The problem is that I added this broken PreLoad line(without understanding
that it was broken) because some game needed it. So its not just the test
that is broken. Reverting it will just trade broken app A against broken app
B.

I have one exam and one excersise done now, and I may have a bit of free
time the next days. Maybe I can find out something more about this problem,
but I can't promise. And if I find the real issue, the fix may be too
invasive for committing.

What I am a bit annoyed about this issue is that many people talked about
how important it is to get this fixed, or talked about reverting patch XYZ,
but nobody tried to find the real issue. Vitaliy did a lot of testing and
experimenting, which was very helpful to understand the bug better, and to
understand what is not a right fix. But all in all I think everybody waited
for one of the main d3d devs to magically fix this.

The bottom line: The next time don't release in June. Many Wine developers,
especially in the game area, are students, and May/June is exam season.