This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• proposed patch for SDL-1.2 HG (sdl12_accel.diff, text/plain, 2011-07-25 09:00:16 +0000, 790 bytes)
• proposed patch for SDL-1.3 HG (sdl13_accel.diff, text/plain, 2011-07-25 09:01:40 +0000, 759 bytes)
• Default gl_config.accelerated to 1 on SDL_1.2 (sgl_glaccelerated.patch, text/plain, 2011-09-27 06:19:36 +0000, 505 bytes)
• Always choose an accel setting, default to FULL. (SDL12-wingl-accel-default-full-RYAN1.diff, text/plain, 2011-11-06 13:37:11 +0000, 1562 bytes)

Reported in version: HG 2.0
Reported for operating system, platform: Windows (All), All

Comments on the original bug report:

On 2011-07-25 09:00:16 +0000, Thilo Schulz wrote:

Created attachment 656
proposed patch for SDL-1.2 HG

Hello,

SDL_GL_ACCELERATED_VISUAL is supposed to ensure accelerated visuals, according to http://www.libsdl.org/release/changes-1.2.html.
However, if SDL_GL_ACCELERATED_VISUAL is not explicitly set, then the integer Attribute:

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

WGL_NO_ACCELERATION_ARB is chosen.
Doesn't this explicitly disable hardware acceleration then and enforce slow software rendering?

It would make more sense to keep this part from SDL-1.2:

    if ( this->gl_config.accelerated >= 0 ) {
            *iAttr++ = WGL_ACCELERATION_ARB;
            *iAttr++ = (this->gl_config.accelerated ? WGL_FULL_ACCELERATION_ARB : WGL_NO_ACCELERATION_ARB);
    }

At least on SDL-1.2, if SDL_GL_ACCELERATED_VISUAL is not set, then this->gl_config.accelerated seems to be -1 and no demands on acceleration are made, as the WGL_ACCELERATION_ARB is never used. Thus either software or hardware rendering can be chosen.
If accelerated is set to 1, then acceleration is enforced what is what we want to do.

I realize there was a previous bug, most noticable on ATI hardware which would turn a red screen when SDL_GL_ACCELERATED_VISUAL is enabled, and the usage of MCD vs ICD drivers. This wouldn't be triggerd again by my proposed changes.

I have attached patches for both SDL-1.2 and 1.3 that should resolve this issue

On 2011-07-25 09:01:40 +0000, Thilo Schulz wrote:

Created attachment 657
proposed patch for SDL-1.3 HG

On 2011-07-29 05:43:05 +0000, Thilo Schulz wrote:

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.

On 2011-08-21 09:14:22 +0000, Ryan C. Gordon wrote:

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

On 2011-08-23 13:53:09 +0000, Ryan C. Gordon wrote:

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

Thanks!

--ryan.

On 2011-09-25 00:48:54 +0000, Ozkan Sezer wrote:

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.

On 2011-09-27 05:03:20 +0000, Thilo Schulz wrote:

(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.

On 2011-09-27 05:20:17 +0000, Ozkan Sezer wrote:

(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;

On 2011-09-27 06:09:58 +0000, Thilo Schulz wrote:

(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.

On 2011-09-27 06:12:48 +0000, Ozkan Sezer wrote:

(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.

On 2011-09-27 06:19:36 +0000, Thilo Schulz wrote:

Created attachment 709
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.

On 2011-09-27 06:24:00 +0000, Thilo Schulz wrote:

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?

On 2011-09-27 06:29:23 +0000, Ozkan Sezer wrote:

(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.

On 2011-09-28 01:17:24 +0000, Ozkan Sezer wrote:

(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.

On 2011-09-28 02:28:02 +0000, Matthias Bentrup wrote:

(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);
}

On 2011-09-28 03:57:35 +0000, Ozkan Sezer wrote:

(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)

On 2011-09-28 05:01:19 +0000, Thilo Schulz wrote:

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.

On 2011-09-28 05:29:13 +0000, Ozkan Sezer wrote:

(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.

On 2011-10-12 00:01:43 +0000, Ozkan Sezer wrote:

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.)

On 2011-11-06 13:37:11 +0000, Ryan C. Gordon wrote:

Created attachment 726
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.

On 2011-11-06 13:38:08 +0000, Ryan C. Gordon wrote:

(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.

On 2011-11-06 14:17:11 +0000, Ozkan Sezer wrote:

(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.

On 2011-11-07 03:26:13 +0000, Thilo Schulz wrote:

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".

On 2011-11-07 10:51:13 +0000, Matthias Bentrup wrote:

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.

On 2011-11-07 12:19:58 +0000, Ryan C. Gordon wrote:

(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.

On 2011-11-07 12:24:06 +0000, Ryan C. Gordon wrote:

(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.

On 2011-11-07 12:44:46 +0000, Ryan C. Gordon wrote:

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.

On 2011-11-07 13:25:07 +0000, Ryan C. Gordon wrote:

(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.

On 2011-11-07 13:54:29 +0000, Thilo Schulz wrote:

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.

On 2012-01-08 00:10:18 +0000, Sam Lantinga wrote:

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

On 2013-05-21 02:48:48 +0000, Sam Lantinga wrote:

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

On 2013-07-05 22:07:40 +0000, Ryan C. Gordon wrote:

(In reply to comment # 30)

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

Not yet, will do.

--ryan.

On 2013-07-11 23:15:50 +0000, Ryan C. Gordon wrote:

(In reply to comment # 31)

Not yet, will do.

This is now handled in SDL2 in hg changeset 698995795574.

--ryan.