# Some pieces of SDL 1.3 win32 video code that look wrong to me

**URL:** <https://discourse.libsdl.org/t/some-pieces-of-sdl-1-3-win32-video-code-that-look-wrong-to-me/15542>\
**Category:** SDL Development\
**Created:** [August 2, 2008, 1:01pm UTC](https://discourse.libsdl.org/t/some-pieces-of-sdl-1-3-win32-video-code-that-look-wrong-to-me/15542 "2008-08-02T13:01:32Z")\
**Posts on this page:** 1\
**Page:** 1

<div class="post-metadata">

**Author:** ![Martin2](https://discourse.libsdl.org/letter_avatar_proxy/v4/letter/m/9de053/32.png) [@Martin2](https://discourse.libsdl.org/u/Martin2)\
**Post date:** [August 2, 2008, 1:01pm UTC](https://discourse.libsdl.org/t/some-pieces-of-sdl-1-3-win32-video-code-that-look-wrong-to-me/15542/1 "2008-08-02T13:01:32Z")

</div>

Hi,

when tracking down a few problems (including bug #605) I had with current SDL  
1.3 (last commit revision 3675), a number of details caught my attention.  
However I’m not sure any or all of them are bugs, so I would like to discuss  
them here.

SDL\_win32opengl.c, WIN\_GL\_SetupWindow:======================================  
\*iAttr++ = WGL\_ACCELERATION\_ARB;  
\*iAttr++ = WGL\_FULL\_ACCELERATION\_ARB;  
…  
if (\_this-\>gl\_config.accelerated \>= 0) {  
\*iAttr++ = WGL\_ACCELERATION\_ARB;  
\*iAttr++ =  
(\_this-\>gl\_config.  
accelerated ? WGL\_GENERIC\_ACCELERATION\_ARB :  
WGL\_NO\_ACCELERATION\_ARB);  
}

Looks like the attribute WGL\_ACCELERATION\_ARB might be inserted twice, with  
contradictory values.

# SDL\_win32modes.c, WIN\_GetDisplayMode:

```
     GetDIBits(hdc, hbm, 0, 1, NULL, bmi, DIB_RGB_COLORS);
     GetDIBits(hdc, hbm, 0, 1, NULL, bmi, DIB_RGB_COLORS);

```

The line appears twice (as shown :)). If that’s intended, please add a comment.

# SDL\_win32window.c, WIN\_CreateWindow:

```
 if (window->flags & SDL_WINDOW_FULLSCREEN) {
     top = HWND_TOPMOST;
 } else {
     top = HWND_NOTOPMOST;
 }

```

The variable top does not seem to be used afterwards. The same few lines appear  
in other functions, so I guess it’s a leftover of some copy and paste action.

# SDL\_win32window.c, SetupWindowData:

```
 /* Set up the window proc function */
 data->wndproc = (WNDPROC) GetWindowLongPtr(hwnd, GWLP_WNDPROC);
 if (data->wndproc == NULL) {
     data->wndproc = DefWindowProc;
 } else {
     SetWindowLongPtr(hwnd, GWLP_WNDPROC, (LONG_PTR) WIN_WindowProc);
 }

```

So, WIN\_WindowProc will replace any window procedure, but only if the window  
really had one. Otherwise, the window will continue to have no window procedure.  
I guess that WIN\_WindowProc should always be set, no matter what the previous  
WNDPROC was. The test for NULL still makes sense, because WIN\_WindowProc will  
occasionally call data-\>wndproc.

# SDL\_win32events.c, SDL\_RegisterApp:

```
 if (!name && !SDL_Appname) {
     name = "SDL_app";
     SDL_Appstyle = (CS_BYTEALIGNCLIENT | CS_OWNDC);
     SDL_Instance = hInst ? hInst : GetModuleHandle(NULL);
 }

 if (name) {
     SDL_Appname = WIN_UTF8ToString(name);
     SDL_Appstyle = style;
     SDL_Instance = hInst ? hInst : GetModuleHandle(NULL);
 }

```

Assigning name inside the first if-block causes the second if-block to be  
entered, too. Hence the assignment of SDL\_AppStyle/SDL\_Instance in the first  
block is useless, because the second block will inevitably replace/repeat it.  
That is, the code has the same result as:  
if (!name && !SDL\_Appname) {  
name = “SDL\_app”;  
}

```
 if (name) {
     SDL_Appname = WIN_UTF8ToString(name);
     SDL_Appstyle = style;
     SDL_Instance = hInst ? hInst : GetModuleHandle(NULL);
 }

```

But I don’t think that was intended.  
I suggest:  
if (name) {  
SDL\_Appname = WIN\_UTF8ToString(name);  
} else if (!SDL\_Appname) {  
SDL\_Appname = WIN\_UTF8ToString(“SDL\_app”);  
}

```
 if (style) {
     SDL_Appstyle = style;
 } else if (!SDL_Appstyle) {
     SDL_Appstyle = (CS_BYTEALIGNCLIENT | CS_OWNDC);
 }

 if (hInst) {
     SDL_Instance = hInst;
 } else if (!SDL_Instance) {
     SDL_Instance = GetModuleHandle(NULL);
 }

```

# SDL\_win32opengl.h, struct SDL\_GLDriverData:

SDL caches some GL function addresses in the SDL\_GLDriverData (part of the video  
driver object). The driver seems to be a global object, so there is only one set  
of addresses being cached.

I’m not sure if it matters, but according to MSDN, wglGetProcAddress may return  
different addresses for equal function names in different GL contexts (more  
specifically, contexts with different pixel formats).  
What if an SDL program tries to render to windows with different pixel formats?

By accident, I already prove that it is possible to activate different OpenGL  
implementations by choosing different pixel formats - see the next case.

# SDL\_win32opengl.c, WIN\_GL\_ChoosePixelFormatARB:

```
 int pixel_format = 0;

```

…  
SetPixelFormat(hdc, ChoosePixelFormat(hdc, &pfd), &pfd);  
…  
\_this-\>gl\_data-\>wglChoosePixelFormatARB(hdc, iAttribs, fAttribs,  
1, &pixel\_format,  
&matching);  
…  
return pixel\_format;

My program told me that glDrawRangeElements was unsupported. This is what I  
found to be the reason:

I had set SDL\_GL\_DEPTH\_SIZE to 32, but the hardware ICD only supports 24 depth  
bits (which kind of suprised me, but it still seems sufficient).  
ChoosePixelFormat picked a format supported by the hardware ICD, reducing the  
depth size as required.

wglChoosePixelFormatARB was less tolerant and found no match (matching==0,  
pixel\_format unchanged). So WIN\_GL\_ChoosePixelFormatARB returned 0.

SDL then fell back to looking through all formats on its own  
(WIN\_GL\_ChoosePixelFormat). The result was a perfect match for all the  
attributes, but supported only by MS’ “GDI Generic” software OpenGL  
implementation. Which does not provide glDrawRangeElements, apart from not using  
my hardware.

I solved the problem by adjusting SDL\_GL\_DEPTH\_SIZE to 24. But I wonder what  
will happen if some hardware only supports 16 bits. How could one tell SDL to  
use a smaller depth buffer rather than falling back to software OpenGL? (And can  
SDL find out whether a pixel format is supported by the hardware ICD?)

During my “investigation”, I tried this little change in the code above:  
SetPixelFormat(hdc, pixel\_format = ChoosePixelFormat(hdc, &pfd), &pfd);

This way, ChoosePixelFormat’s “hardware-oriented” choice is accepted if  
wglChoosePixelFormatARB does not find a (better?) match. What do you think about it?

Regards  
Martin