GL_INVALID_ENUM errors with FBO

[Vitaliy Margolen]

I've been trying to track some of the errors that I get with different 
games. To do that I've added some extra traces and this is what I see:

fixme:d3d_surface:read_from_framebuffer_texture glReadBuffer(0x405) was 
0x8ce0 >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) @ 
../../../wine.git/dlls/wined3d/surface.c / 918

0x405 is GL_BACK and 0x8ce0 is GL_COLOR_ATTACHMENT0_EXT. Also when this 
happens I have an inverted image of the entire screen in the left bottom 
corner. Is this an indication that we are reading from the wrong surface? Or 
writing into the wrong surface?

Attached is the patch that produces the above output. BTW if it looks fine 
to you guys (that extra TRACE()/FIXME() call overhead) then I can send it to 
  wine-patches as well.

Vitaliy.

[H. Verbeet]

2008/7/24 Vitaliy Margolen :
> I've been trying to track some of the errors that I get with different
> games. To do that I've added some extra traces and this is what I see:
>
> fixme:d3d_surface:read_from_framebuffer_texture glReadBuffer(0x405) was
> 0x8ce0 >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) @
> ../../../wine.git/dlls/wined3d/surface.c / 918
>
> 0x405 is GL_BACK and 0x8ce0 is GL_COLOR_ATTACHMENT0_EXT. Also when this
> happens I have an inverted image of the entire screen in the left bottom
> corner. Is this an indication that we are reading from the wrong surface? Or
> writing into the wrong surface?

The problem is that GL_BACK isn't a valid read buffer when an FBO is
bound. Most of the problem there comes from the fact that
apply_fbo_state() should happen inside ActivateContext() rather than
before or after it. I'm working on trying to fix this, what I've got
so far is at http://bugs.winehq.org/attachment.cgi?id=15013, you might
want to give that a try to see if it fixes anything. I've already been
told it introduces some other errors though (see bug 14038), so it's
not quite ready yet.
> Attached is the patch that produces the above output. BTW if it looks fine
> to you guys (that extra TRACE()/FIXME() call overhead) then I can send it to
>  wine-patches as well.

checkGLcall is relatively expensive already, but that's mostly due to
the glGetError() it does. I don't think an extra TRACE or FIXME will
make it much worse.

[Vitaliy Margolen]

H. Verbeet wrote:
> 2008/7/24 Vitaliy Margolen :
> > I've been trying to track some of the errors that I get with different
> > games. To do that I've added some extra traces and this is what I see:
> >
> > fixme:d3d_surface:read_from_framebuffer_texture glReadBuffer(0x405) was
> > 0x8ce0 >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) @
> > ../../../wine.git/dlls/wined3d/surface.c / 918
> >
> > 0x405 is GL_BACK and 0x8ce0 is GL_COLOR_ATTACHMENT0_EXT. Also when this
> > happens I have an inverted image of the entire screen in the left bottom
> > corner. Is this an indication that we are reading from the wrong surface? Or
> > writing into the wrong surface?
>
> The problem is that GL_BACK isn't a valid read buffer when an FBO is
> bound. Most of the problem there comes from the fact that
> apply_fbo_state() should happen inside ActivateContext() rather than
> before or after it. I'm working on trying to fix this, what I've got
> so far is at http://bugs.winehq.org/attachment.cgi?id=15013, you might
> want to give that a try to see if it fixes anything. I've already been
> told it introduces some other errors though (see bug 14038), so it's
> not quite ready yet.

Yes the patch does fix the problem for me with at least two programs I've 
tested - Psychonauts demo and Tomb Raider - Legend demo. And didn't see any 
side affects. But the full version of Psychonauts crashes after initial 
videos. Same as it did when we had problems with multi-threaded games.
> > Attached is the patch that produces the above output. BTW if it looks fine
> > to you guys (that extra TRACE()/FIXME() call overhead) then I can send it to
> >  wine-patches as well.
>
> checkGLcall is relatively expensive already, but that's mostly due to
> the glGetError() it does. I don't think an extra TRACE or FIXME will
> make it much worse.

I will see if there is a way to add that and also address the issue of the 
race between two separate messages that Chris was talking about.

Vitaliy.

[H. Verbeet]

2008/7/24 Vitaliy Margolen :
> side affects. But the full version of Psychonauts crashes after initial
> videos. Same as it did when we had problems with multi-threaded games.

Is that a regression introduced by the patch?

[Vitaliy Margolen]

H. Verbeet wrote:
> 2008/7/24 Vitaliy Margolen :
> > side affects. But the full version of Psychonauts crashes after initial
> > videos. Same as it did when we had problems with multi-threaded games.
>
> Is that a regression introduced by the patch?

Yes. Without the patch everything works. With the patch it crashes.

Vitaliy

[Chris Robinson]

On Wednesday 23 July 2008 03:56:14 pm Vitaliy Margolen wrote:
> +#define checkGLcall(A...)                                       \

AFAIK, this valid C. It's a GNU extension. This:
#define checkGLcall(...)
TRACE(__VA_ARGS__);
is valid C, but requires C99.
> - TRACE(A);                                               \
> - TRACE(" @ %s:%d call ok\n", __FILE__, __LINE__);        \

And I think splitting this up potentially introduces a trace race if a thread 
switch happens between the two calls. The other thread's trace would print 
where the second statement should, and the second statement would 
just "float" later in the log.