We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Created attachment 657 [details]
proposed patch for SDL-1.3 HG

I realize my comment is confusing and I'm not right with all the facts. I'll try again.

In SDL_VideoInit() we find this line:
_this->gl_config.accelerated = -1;

So without explicitly setting a value for SDL_GL_ACCELERATED_VISUAL the value will be always -1.
In the latest HG version we only find these two lines:

*iAttr++ = WGL_ACCELERATION_ARB;
*iAttr++ = (_this->gl_config.accelerated ? WGL_FULL_ACCELERATION_ARB :
                                               WGL_NO_ACCELERATION_ARB);

So what happens is that WGL_FULL_ACCELERATION_ARB is always being added, even if SDL_GL_ACCELERATED_VISUAL was never set! My proposed patch for SDL-1.3 fixes this issue.
Also, SDL_GL_ACCELERATED_VISUAL should be documented to mean:
-1: Software or Hardware rendering
0: only software rendering
1 only hardware rendering

My proposed patch for the SDL-1.2 branch fixes the catalyst driver issues which were fixed in SDL-1.3, but that introduced the incorrect behaviour as outlined above.

*** Bug 1197 has been marked as a duplicate of this bug. ***

These patches are now hg changeset 84d302e859d1 for the 1.3 branch, and hg changeset 181d53fc4e23 for the 1.2 branch.

Thanks!

--ryan.

In an SDL-gl application, is not setting the SDL_GL_ACCELERATED_VISUAL attribute considered a bug?  Whatever would happen if an application compiled using SDL <= 1.2.9 is run using new SDL.dll?

Starting with SDL-1.2/181d53fc4e23, such an application (specifically quakespasm, see http://quakespasm.sourceforge.net/) only renders an ugly red-filled window and only if I explicitly set SDL_GL_ACCELERATED_VISUAL I get a properly rendered screen.  This is on win64 (vista) with ATI Catalyst drivers 8.563 provided by HP.

I do not know any details about the Catalyst driver issues mentioned in comment #2, but I think this is a regression.

(In reply to comment #5)
> In an SDL-gl application, is not setting the SDL_GL_ACCELERATED_VISUAL
> attribute considered a bug?  Whatever would happen if an application compiled
> using SDL <= 1.2.9 is run using new SDL.dll?

I don't think so.

> Starting with SDL-1.2/181d53fc4e23, such an application (specifically
> quakespasm, see http://quakespasm.sourceforge.net/) only renders an ugly
> red-filled window and only if I explicitly set SDL_GL_ACCELERATED_VISUAL I get
> a properly rendered screen.  This is on win64 (vista) with ATI Catalyst drivers
> 8.563 provided by HP.
>
> I do not know any details about the Catalyst driver issues mentioned in comment
> #2, but I think this is a regression.

I thought I tested running a game without adding any SDL_GL_ACCELERATED_VISUAL attribute with windows ati drivers and with my patch. If I remember correctly, it worked. I don't regularly have access to ATI hardware, though, so I cannot test this.

As to what you call a "regression": The fault clearly lies with the ATI driver. Before my patch, you would have red screens on software for the 1.2 branch if you explicitly SET the SDL_GL_ACCELERATED_VISUAL. So it was broken too, but in a different way.
That sucks for old software that needs to work around the ATI driver bugs, but at least SDL works as documented now.

(In reply to comment #6)
> 
>  The fault clearly lies with the ATI driver.
> Before my patch, you would have red screens on software for the 1.2 branch if
> you explicitly SET the SDL_GL_ACCELERATED_VISUAL. So it was broken too, but
> in a different way.
> That sucks for old software that needs to work around the ATI driver bugs, but
> at least SDL works as documented now.

Well, then setting acceleration bits unconditionally and not doing anything acc. to gl_config.accelerated setting, i.e. SDL_GL_ACCELERATED_VISUAL, would be the foolproof way of making things work.  i.e. reverting 181d53fc4e23 _and_ commenting out the code which is run if this->gl_config.accelerated >= 0.  Something like:

--- SDL_wingl.c~
+++ SDL_wingl.c
@@ -220,6 +220,10 @@ int WIN_GL_SetupWindow(_THIS)
 
 	*iAttr++ = WGL_DRAW_TO_WINDOW_ARB;
 	*iAttr++ = GL_TRUE;
+#if 1
+	*iAttr++ = WGL_ACCELERATION_ARB;
+	*iAttr++ = WGL_FULL_ACCELERATION_ARB;
+#endif
 	*iAttr++ = WGL_RED_BITS_ARB;
 	*iAttr++ = this->gl_config.red_size;
 	*iAttr++ = WGL_GREEN_BITS_ARB;
@@ -278,10 +282,12 @@ int WIN_GL_SetupWindow(_THIS)
 		*iAttr++ = this->gl_config.multisamplesamples;
 	}
 
+#if 0
 	if ( this->gl_config.accelerated >= 0 ) {
 		*iAttr++ = WGL_ACCELERATION_ARB;
 		*iAttr++ = (this->gl_config.accelerated ? WGL_FULL_ACCELERATION_ARB : WGL_NO_ACCELERATION_ARB);
 	}
+#endif
 
 	*iAttr = 0;

(In reply to comment #7)
> Well, then setting acceleration bits unconditionally and not doing anything
> acc. to gl_config.accelerated setting, i.e. SDL_GL_ACCELERATED_VISUAL, would be
> the foolproof way of making things work.  i.e. reverting 181d53fc4e23 _and_
> commenting out the code which is run if this->gl_config.accelerated >= 0. 
> Something like:

It would also break the functionality that setting was meant to introduce and lock out software renderers. We really don't want that.

(In reply to comment #8)
> (In reply to comment #7)
> > Well, then setting acceleration bits unconditionally and not doing anything
> > acc. to gl_config.accelerated setting, i.e. SDL_GL_ACCELERATED_VISUAL, would be
> > the foolproof way of making things work.  i.e. reverting 181d53fc4e23 _and_
> > commenting out the code which is run if this->gl_config.accelerated >= 0. 
> > Something like:
> 
> It would also break the functionality that setting was meant to introduce and
> lock out software renderers. We really don't want that.

What is the solution then? As it is now, 1.2 tree is unusable on windows, at least for my app.

Created attachment 709 [details]
Default gl_config.accelerated to 1 on SDL_1.2

This would be a better fix to the problem of supporting old legacy software relying on the old incorrect SDL behaviour, but only on Windows. SDL_VideoInit defaults to accelerated visuals now, but the application can change this at any point in time.

However, this comes with a drawback: enabling this will make software fail on X11 where users have no hardware acceleration, and where the software does not explicitly set SDL_GL_ACCELERATED_VISUAL to -1

Better let Ryan decide what he wants. Maybe wrap my changes around #ifdefs for windows (ugly). Or just live with brokenness for old legacy software on windows.

> What is the solution then? As it is now, 1.2 tree is unusable on windows, at
> least for my app.

Also try updating to newer drivers before you require the SDL guys to make any changes. Catalyst 8.* is rather old, isn't it?

(In reply to comment #11)
> Also try updating to newer drivers before you require the SDL guys to make any
> changes. Catalyst 8.* is rather old, isn't it?

I can't do that. HP is the only driver provider for the on board hardware of my HP laptop, and what I am using is the latest.

(In reply to comment #8)
> (In reply to comment #7)
> > Well, then setting acceleration bits unconditionally and not doing anything
> > acc. to gl_config.accelerated setting, i.e. SDL_GL_ACCELERATED_VISUAL, would be
> > the foolproof way of making things work.  i.e. reverting 181d53fc4e23 _and_
> > commenting out the code which is run if this->gl_config.accelerated >= 0. 
> > Something like:
> 
> It would also break the functionality that setting was meant to introduce and
> lock out software renderers. We really don't want that.

Thinking about this more carefully, setting the SDL_GL_ACCELERATED_VISUAL attribute must not be the issue here. What you are interested in must be what you _get_ from SDL_GL_GetAttribute(SDL_GL_ACCELERATED_VISUAL,&value) call in the value variable, 1 or 0.  Therefore your above concern should be a no problem.

(In reply to comment #13)
> Thinking about this more carefully, setting the SDL_GL_ACCELERATED_VISUAL
> attribute must not be the issue here. What you are interested in must be what
> you _get_ from SDL_GL_GetAttribute(SDL_GL_ACCELERATED_VISUAL,&value) call in
> the value variable, 1 or 0.  Therefore your above concern should be a no
> problem.

No, the current code conforms to the WGL_ARB_pixel_format spec for all three possible values of SDL_GL_ACCELERATED_VISUAL, the old code did not for values 0 or 1, because it passed two conflicting values for the WGL_ACCELERATION_ARB attribute !

It does trigger a *driver bug* on old ATI drivers, when no WGL_ACCELERATION_ARB is passed. Unfortunately you cannot pass a DONT_CARE value for WGL_ACCELERATION_ARB, so the "correct" implementation of the -1 case is to pass no WGL_ACCELERATION_ARB attribute, as Thilo implemented.

The only way I see to implement mode -1 without triggering the ATI bug would be something like

    pixel_format = 0;
    if (!pixel_format && _this->gl_config.accelerated != 0) {
        iAttribs[something] = WGL_FULL_ACCELERATION_ARB;
        pixel_format = WIN_GL_ChoosePixelFormatARB(_this, iAttribs, fAttribs);
    }
    if (!pixel_format && _this->gl_config.accelerated != 1) {
        iAttribs[something] = WGL_NO_ACCELERATION_ARB;
        pixel_format = WIN_GL_ChoosePixelFormatARB(_this, iAttribs, fAttribs);
    }

(In reply to comment #14)
> (In reply to comment #13)
> > Thinking about this more carefully, setting the SDL_GL_ACCELERATED_VISUAL
> > attribute must not be the issue here. What you are interested in must be what
> > you _get_ from SDL_GL_GetAttribute(SDL_GL_ACCELERATED_VISUAL,&value) call in
> > the value variable, 1 or 0.  Therefore your above concern should be a no
> > problem.
> 
> No, the current code conforms to the WGL_ARB_pixel_format spec for all three
> possible values of SDL_GL_ACCELERATED_VISUAL, the old code did not for values 0
> or 1, because it passed two conflicting values for the WGL_ACCELERATION_ARB
> attribute !
> 
> It does trigger a *driver bug* on old ATI drivers, when no WGL_ACCELERATION_ARB
> is passed. Unfortunately you cannot pass a DONT_CARE value for
> WGL_ACCELERATION_ARB, so the "correct" implementation of the -1 case is to pass
> no WGL_ACCELERATION_ARB attribute, as Thilo implemented.
> 
> The only way I see to implement mode -1 without triggering the ATI bug would be
> something like
> 
>     pixel_format = 0;
>     if (!pixel_format && _this->gl_config.accelerated != 0) {
>         iAttribs[something] = WGL_FULL_ACCELERATION_ARB;
>         pixel_format = WIN_GL_ChoosePixelFormatARB(_this, iAttribs, fAttribs);
>     }
>     if (!pixel_format && _this->gl_config.accelerated != 1) {
>         iAttribs[something] = WGL_NO_ACCELERATION_ARB;
>         pixel_format = WIN_GL_ChoosePixelFormatARB(_this, iAttribs, fAttribs);
>     }


Well, if I patch SDL like

--- SDL_wingl.c~
+++ SDL_wingl.c
@@ -278,10 +278,15 @@ int WIN_GL_SetupWindow(_THIS)
 		*iAttr++ = this->gl_config.multisamplesamples;
 	}
 
+#if 1
+	*iAttr++ = WGL_ACCELERATION_ARB;
+	*iAttr++ = WGL_FULL_ACCELERATION_ARB;
+#else
 	if ( this->gl_config.accelerated >= 0 ) {
 		*iAttr++ = WGL_ACCELERATION_ARB;
 		*iAttr++ = (this->gl_config.accelerated ? WGL_FULL_ACCELERATION_ARB : WGL_NO_ACCELERATION_ARB);
 	}
+#endif
 
 	*iAttr = 0;
 

.. then:
- it works with my old broken ati driver
- and I get true in value when I do SDL_GL_GetAttribute(SDL_GL_ACCELERATED_VISUAL,&value);

Do you see anything wrong with this?

Your concern is, correct me if I'm wrong, that "not setting SDL_GL_ACCELERATED_VISUAL should mean user wants no acceleration" and you want to setup your context that way and the old SDL+driver combination did it wrong. I am curious as to why would anyone want no acceleration when using opengl? (again, _if_ I am reading you correctly)

> Do you see anything wrong with this?

Yes, you take the choice away from users to run in software rendering mode (e.g. for people that don't have hardware acceleration for whatever reason), even if the drivers they have are not broken.

(In reply to comment #16)
> > Do you see anything wrong with this?
> 
> Yes, you take the choice away from users to run in software rendering mode
> (e.g. for people that don't have hardware acceleration for whatever reason),
> even if the drivers they have are not broken.

IMO, this is not a good enough argument when the current state breaks things and the old state being also buggy, as you mentioned, when SDL_GL_ACCELERATED_VISUAL is set, meaning that setting that attribute in my app cannot be a solution either.  Let's see what the maintainers have to say.

Will this be resolved anytime soon? Unmodified 1.2 is useless for me at the moment on windows (I can only use it patched as shown in comment #15.)

Created attachment 726 [details]
Always choose an accel setting, default to FULL.


There seems to be a disconnect between "don't care" and "let the OS decide."

Is there any reason we can't force hardware acceleration unless the user sets SDL_GL_ACCELERATED_VISUAL to zero? Always forcing hardware acceleration inside SDL, when the app doesn't care what you get from the OS, would fulfill the API contract.

Also, on a purely practical level: has anyone that left this attribute unset been happy if they got a software renderer? My guess would be it isn't a declaration that they don't care, it's more likely the app just forgot to explicitly set it.

Convince me otherwise, or I'll be pushing the attached changeset.

I think this will make everyone happy: the SDL API works as expected, the ATI driver bug should be avoided, and we don't have to wedge a hack in at the higher level for all platforms.

Yes?

--ryan.

(In reply to comment #19)
> Created attachment 726 [details]
> Always choose an accel setting, default to FULL.

That's for 1.2, btw...the 1.3 change is basically the same, though.

--ryan.

(In reply to comment #19)
> Created attachment 726 [details]
> Always choose an accel setting, default to FULL.
> 
> 
> There seems to be a disconnect between "don't care" and "let the OS decide."
> 
> Is there any reason we can't force hardware acceleration unless the user sets
> SDL_GL_ACCELERATED_VISUAL to zero? Always forcing hardware acceleration inside
> SDL, when the app doesn't care what you get from the OS, would fulfill the API
> contract.
> 
> Also, on a purely practical level: has anyone that left this attribute unset
> been happy if they got a software renderer? My guess would be it isn't a
> declaration that they don't care, it's more likely the app just forgot to
> explicitly set it.
> 
> Convince me otherwise, or I'll be pushing the attached changeset.
> 
> I think this will make everyone happy: the SDL API works as expected, the ATI
> driver bug should be avoided, and we don't have to wedge a hack in at the
> higher level for all platforms.
> 
> Yes?
> 
> --ryan.

I am OK with this patch: I do not specify anything and I get properly accelerated ogl on my buggy ATI testbed, so I'm happy.

Hmm.. well, this will likely break OpenGL for most SDL dependent software on computers without hardware acceleration, where software rendering would be sufficient.
However, yes, it's a border case as opposed to the AMD drivers issue which isn't. If you want to go your route, Ryan, please consider the patch I obsoleted (which would probably have to be adapted for SDL 1.3):
http://bugzilla.libsdl.org/attachment.cgi?id=709

It sets the default to "full acceleration" and will go with that value if software doesn't specify anything, but you can still explicitly choose "don't care".

Also consider this fix: http://bugzilla.libsdl.org/attachment.cgi?id=725&action=diff#a/src/video/windows/SDL_windowsopengl.c_sec3

In the don't care case, it first looks for an accelerated pixel format and only if that fails, it looks for an unaccelerated pixel format.

(In reply to comment #23)
> Also consider this fix:
> In the don't care case, it first looks for an accelerated pixel format and only
> if that fails, it looks for an unaccelerated pixel format.

That's probably worthwhile for the sake of robustness.


(In reply to comment #22)
> It sets the default to "full acceleration" and will go with that value if
> software doesn't specify anything, but you can still explicitly choose "don't
> care".

How about this: instead of setting the default at the higher level to 1 (full accel) as opposed to -1 (don't care), we make it -2 (we'll call this "unspecified").

Then on Windows, we'll treat -2 as FULL (if that fails, which it might not even if it should, we'll try NO), -1 will not set the attribute at all (which will make Windows treat it as a true "don't care"), 0 will be NO, and >= 1 will be FULL.

This means you get a default that fixes the ATI drivers and satisfies the common use case, you can still explicitly set FULL, NO, or DONTCARE, and the API contract works as expected in any case.

Is that acceptable to everyone?

--ryan.

(In reply to comment #24)
> How about this: instead of setting the default at the higher level to 1 (full
> accel) as opposed to -1 (don't care), we make it -2 (we'll call this
> "unspecified").

(Actually, I'll probably leave it as -1, and have a separate flag to note that it's been explictly set, rather than worry about every target checking for -2...same results, though.)

--ryan.

Actually, this is goofy.

If the app doesn't care if it gets software rendering, they're just going to have to suffer with the awful results of full acceleration. There's just so few cases where one would say "don't care" and actually mean it.

And few cases where one would actually _get_ software rendering on modern systems.

Also: some well-meaning app explicitly saying "don't care" so they, what? Don't eat GPU resources? They will eat the more expensive CPU resources, and will break these ATI drivers just as easily, probably without any way for the end user to fix it...setting the GL acceleration value isn't usually something in the config file, and it also pushes this bug off onto apps and users.

Also: making everyone happy in principle just makes the code more complex in SDL...in practice, everyone will be happy with the current patch, I believe.

--ryan.

(sorry for all the bug report spam on this one...)

Ok, my first patch is now hg changeset d0b7c45e982e for the 1.2 branch. It's what I've talked myself into.

Trying NO accel if FULL fails when the app didn't specify anything, as Comment #14 suggested, is hg changeset a04171d6fa11.

I'm leaving this bug open until I do the same for the 1.3 branch.

--ryan.

Currently, ioquake3 has the cvar r_allowSoftwareGL (which defaults to 0, by the way, so it will always choose the FULL acceleration pixel format), which it imported from the original idq3 version still.
That name only makes sense if an application is able to explicitly set a "don't care" behaviour. I also thought the need for the support of legacy software was less pressing for the 1.3 SDL branch, as software developers would have to adapt to the new API anyways. What I proposed gives a software developer most direct control over what pixel formats he can choose (full accel/no accel/let driver choose) what may or may not be desirable for SDL which is just a means to abstract these basic calls.

Really, that issue is probably so minor that I won't mind if you decide either way. It's your call Ryan.

Ryan, can you go ahead and commit this in 1.3?

Ryan, did you ever take care of this for 2.0?

(In reply to comment #30)
> Ryan, did you ever take care of this for 2.0?

Not yet, will do.

--ryan.

(In reply to comment #31)
> Not yet, will do.

This is now handled in SDL2 in hg changeset 698995795574.

--ryan.