Re: [PATCH RFC 5/5] dsound: convert from fixed to floating point
Alexander E. Patrakov patra...@gmail.com wrote: Alexander E. Patrakov patra...@gmail.com wrote: Andrew Eikum converted the buffer position calculation code from fixed point to floating point, because floating-point code is more readable. I agree with him. I am not sending further patches out, because it is not yet clear which of the two proposed patchsets for bug 14717 will be the final one. Anyway, both of the approaches depend on all the patches that I sent today. Please ignore this patch for now, it has at least one bug. Please ignore the ignore, I need some coffee. -- Alexander E. Patrakov
Re: [PATCH RFC 5/5] dsound: convert from fixed to floating point
Alexander E. Patrakov patra...@gmail.com wrote: Andrew Eikum converted the buffer position calculation code from fixed point to floating point, because floating-point code is more readable. I agree with him. I am not sending further patches out, because it is not yet clear which of the two proposed patchsets for bug 14717 will be the final one. Anyway, both of the approaches depend on all the patches that I sent today. Please ignore this patch for now, it has at least one bug. -freqAcc += adj; -adv = (freqAcc DSOUND_FREQSHIFT); -freqAcc = (1 DSOUND_FREQSHIFT) - 1; -ipos += adv * istride; +freqAcc += dsb-freqAdjust; +ipos += ((DWORD)freqAcc) * istride; +freqAcc -= truncf(freqAcc); The last line of this hunk is wrong, should be =, not -=. -- Alexander E. Patrakov
Re: DIB crash with gdb
On Fri, Dec 23, 2011 at 11:56:35PM -0600, Ken Thomases wrote: On Dec 23, 2011, at 3:10 PM, Michael Ost wrote: This all makes sense, and pulls the code together for me. Thanks! You're welcome. I assume this is a recent development, because I was successfully using gdb with our last wine version - 1.1.7. Nope. This is how it has worked for a long, long time. Don't know what else has changed. Maybe some of the other work on the DIB engine has changed whether/when the DIB accesses cause access violations that you're seeing. Yes, the DIB engine should no longer let this happen with the last 1.3. releases since around September or so. Otherwise, in gdb you can type pass to pass the signal on to the program. Ciao, Marcus
Re: [PATCH RFC 5/5] dsound: convert from fixed to floating point
On Saturday, December 24, 2011 9:20:38 PM Alexander E. Patrakov wrote: Andrew Eikum converted the buffer position calculation code from fixed point to floating point, because floating-point code is more readable. I agree with him. FWIW, I'm not sure using floating point for the position index is that good of an idea since it needs to be converted to integer all the time. Float-to-int conversions are very slow because it involves reading the FPU control word, setting a new control word with the proper rounding mode (which takes several cycles), storing the float value as an integer, then setting the old control word back (which again takes several cycles). It does this every time a float gets converted to an int, and does something similar when you truncate/floor the float value. In addition, you have potential rounding errors caused by precision loss as the exponent gets larger. Fixed-point doesn't have this issue since the precision is fixed, and the result will always be consistent. E.g., val + step*3 == val + step + step + step is true with fixed-point, but may not be true with floating-point and can result in a value that's larger or smaller than you expect. In some circumstances, it could even cause you to read past the end of the buffer. If you're concerned about cleanliness, perhaps some simple inline functions could be used to hide the bit-manipulation.
Re: kernel32/tests: test that GetModuleFileName returns an absolute path
Thomas Faber thfa...@gmx.de wrote: diff --git a/dlls/kernel32/tests/Makefile.in b/dlls/kernel32/tests/Makefile.in index dce27db..2ce3ec6 100644 --- a/dlls/kernel32/tests/Makefile.in +++ b/dlls/kernel32/tests/Makefile.in @@ -1,5 +1,5 @@ TESTDLL = kernel32.dll -IMPORTS = user32 advapi32 +IMPORTS = user32 advapi32 shlwapi Adding a shlwapi dependency to the kernel32 test suite is not a good idea, please find a way to avoid it. -- Dmitry.
Re: kernel32/tests: test that GetModuleFileName returns an absolute path
On 2011-12-25 17:12, Dmitry Timoshkov wrote: Thomas Faber thfa...@gmx.de wrote: diff --git a/dlls/kernel32/tests/Makefile.in b/dlls/kernel32/tests/Makefile.in index dce27db..2ce3ec6 100644 --- a/dlls/kernel32/tests/Makefile.in +++ b/dlls/kernel32/tests/Makefile.in @@ -1,5 +1,5 @@ TESTDLL = kernel32.dll -IMPORTS = user32 advapi32 +IMPORTS = user32 advapi32 shlwapi Adding a shlwapi dependency to the kernel32 test suite is not a good idea, please find a way to avoid it. Hmm okay. I'll just use strchr(path, '\\') as a substitute for PathIsRelative then, though that's not as accurate of course. Thanks. -Tom
Re: [PATCH 2/7] wined3d: Move srgb checks away from d3dfmt_get_conv.
Hi,
I beleive this patch introduced some kind of regression. While testing
Fallout the following message was printed to the screen:
trace:d3d_surface:surface_allocate_surface (0x1a25f0) : Creating surface
(target 0xde1) level 0, d3d format WINED3DFMT_P8_UINT, internal format 0x80e5,
width 1024, height 512, gl format 0x1908, gl type=0x1401
err:d3d_surface:surface_allocate_surface GL_INVALID_VALUE
(0x501) from glTexImage2D @ surface.c / 2571
Internal format 0x80e5 (GL_COLOR_INDEX8_EXT) is not valid for the format
0x1908 (GL_RGBA).
Initially, the surface's format is 0x1900 (GL_COLOR_INDEX) which is
compatible with the internal format above.
On Thu, Apr 08, 2010 at 10:49:46PM +0200, Roderick Colenbrander wrote:
[...]
@@ -1553,7 +1581,7 @@ void surface_prepare_texture(IWineD3DSurfaceImpl
*surface, const struct wined3d_
if (surface-Flags alloc_flag) return;
-d3dfmt_get_conv(surface, TRUE, TRUE, desc, convert, srgb);
+d3dfmt_get_conv(surface, TRUE, TRUE, desc, convert);
if(convert != NO_CONVERSION) surface-Flags |= SFLAG_CONVERTED;
else surface-Flags = ~SFLAG_CONVERTED;
@@ -1569,7 +1597,7 @@ void surface_prepare_texture(IWineD3DSurfaceImpl
*surface, const struct wined3d_
}
surface_bind_and_dirtify(surface, srgb);
-surface_allocate_surface(surface, gl_info, desc, width, height);
+surface_allocate_surface(surface, gl_info, desc, srgb, width, height);
surface-Flags |= alloc_flag;
}
However before surface_allocate_surface is called d3dfmt_get_conv which
translates the format to GL_RGBA. But the internal format remains in an
incompatible value due to a reordering of the instructions which was
introduced by the patch.
On Thu, Apr 08, 2010 at 10:49:46PM +0200, Roderick Colenbrander wrote:
[...]
@@ -2126,18 +2153,6 @@ HRESULT d3dfmt_get_conv(IWineD3DSurfaceImpl *This,
BOOL need_alpha_ck, BOOL use_
*desc = *This-resource.format_desc;
*convert = NO_CONVERSION;
-/* TODO: the caller should perform this check */
-if(srgb_mode) {
-desc-glInternal = glDesc-glGammaInternal;
-}
-else if (This-resource.usage WINED3DUSAGE_RENDERTARGET
- surface_is_offscreen((IWineD3DSurface *) This))
-{
-desc-glInternal = glDesc-rtInternal;
-} else {
-desc-glInternal = glDesc-glInternal;
-}
-
/* Ok, now look if we have to do any conversion */
switch(This-resource.format_desc-format)
{
Before applying the changes the processing of srgb_mode was done inside
d3dfmt_get_conv. Depending on the value of this variable one of
glGammaInternal, rtInternal or glInternal was used.
But those values were overriden because the conversion was applied
afterwards.
On Thu, Apr 08, 2010 at 10:49:46PM +0200, Roderick Colenbrander wrote:
[...]
@@ -783,11 +797,25 @@ static void surface_upload_data(IWineD3DSurfaceImpl
*This, const struct wined3d_
* the correct texture. */
/* Context activation is done by the caller. */
static void surface_allocate_surface(IWineD3DSurfaceImpl *This, const struct
wined3d_gl_info *gl_info,
-const struct wined3d_format_desc *format_desc, GLsizei width,
GLsizei height)
+const struct wined3d_format_desc *format_desc, BOOL srgb, GLsizei
width, GLsizei height)
{
BOOL enable_client_storage = FALSE;
const BYTE *mem = NULL;
-GLenum internal = format_desc-glInternal;
+GLenum internal;
+
+if (srgb)
+{
+internal = format_desc-glGammaInternal;
+}
+else if (This-resource.usage WINED3DUSAGE_RENDERTARGET
+ surface_is_offscreen((IWineD3DSurface *)This))
+{
+internal = format_desc-rtInternal;
+}
+else
+{
+internal = format_desc-glInternal;
+}
if (format_desc-heightscale != 1.0f format_desc-heightscale !=
0.0f) height *= format_desc-heightscale;
After the change was applied the result of the conversion done inside
d3dfmt_get_conv only takes effect in the else branch of srgb processing.
The following code is the conversion involved in this particular test
case.
format-glFormat = GL_RGBA;
format-glInternal = GL_RGBA;
format-glType = GL_UNSIGNED_BYTE;
format-conv_byte_count = 4;
It's located in d3dfmt_get_conv under the WINED3DFMT_P8_UINT case. For
some reason only glInternal is updated by the conversion. In any case it
didn't matter before the patch as none of the internal values were
actually used in case a conversion was applied.
Should the three internal values be updated by the conversion now that
any of them could be used? Even if it would work I'm not sure how
correct would be to do that.
Could some wined3d developer review the patch with all this in mind?
Thanks!
