Re: [osg-users] [osgPlugins] DDS Texture vanish with LINEAR_MIPMAP_LINEAR

[Lukasz Izdebski]

Hi,
i probably have solution for this problem, i have found a bug in dds plugin
ReaderWriterDDS.cpp
Line 633
unsigned numMipmaps = osg::Image::computeNumberOfMipmapLevels( s, t, r );

when compute numMipmaps returns wrong number. this number is less then number of mipmaps in dds file( ddsd.dwMipMapCount ) . This bug makes that when dds mipmaps are loaded to opengl last mipmap (4x4) isn't loaded. and with combination with LINEAR_MIPMAP_LINEAR make this bug.


the numMipmaps should be taken form ddsd.dwMipMapCount


in attachment i send a corrected version of file.
...

Thank you!

Cheers,
Lukasz

------------------
Read this topic online here:
http://forum.openscenegraph.org/viewtopic.php?p=51653#51653




Attachments:
http://forum.openscenegraph.org//files/readerwriterdds_204.cpp


_______________________________________________
osg-users mailing list
osg-...@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

[Christian Schulte]

Hi all,

I have investigated a little deeper the problem... Indeed, on Windows platform, the number of mipmaps returned by osg::Image::computeNumberOfMipmapLevels( s, t, r ) is wrong, but it is correct on Linux platforms for the same dds file...
Here is attached a little test program that explains why (it is more or less a computeNumberOfMipmapLevels with s=t=1024 and r=1), and it is not linked to OSG. Compile it on Windows 32 bits (works on Win7 and WinXP) (g++ main.cpp -o main.exe) You will see the following result :

logf(wf)                   = 6.93147182464599609375
log(wd)                    = 6.93147180559945308431
logf(2.0f)                 = 0.69314718246459960938
log(2.0)                   = 0.69314718055994528623
logf(wf)/logf(2.0f)        = 10.00000000000000000000
log(wd)/log(2.0)           = 10.00000000000000000000
floor(logf(wf)/logf(2.0f)) = 9.00000000000000000000 -> Here is the error, it should be 10 too...
floor(log(wd)/log(2.0))    = 10.00000000000000000000
floor(testf)               = 10.00000000000000000000
floor(testd)               = 10.00000000000000000000

Replacing the include of math.h by cmath results in :

logf(wf)                   = 6.93147182464599609375
log(wd)                    = 6.93147180559945308431
logf(2.0f)                 = 0.69314718246459960938
log(2.0)                   = 0.69314718055994528623
logf(wf)/logf(2.0f)        = 10.00000000000000000000
log(wd)/log(2.0)           = 10.00000000000000000000
floor(logf(wf)/logf(2.0f)) = 10.00000000000000000000
floor(log(wd)/log(2.0))    = 10.00000000000000000000
floor(testf)               = 10.00000000000000000000
floor(testd)               = 10.00000000000000000000

Under Linux both solutions give the second results.
The problem is that osg/Math includes math.h and not cmath. I don't know which would be the best solution :

• Replace math.h by cmath in include/osg/Math
• Store the logf division (float testf = logf(wf)/logf(2.0f)) of osg::Image::computeNumberOfMipmapLevels( s, t, r ) in a float before computing the floor.

Up to the list to give the answer...

PS : this is also the solution to the discussion "osgPlugins : dds problem on windows platform" I launched 07/03/2011

Cheers,

Christian Schulte

[Robert Osfield]

Hi Christian,

Does this mean there is a bug in the MS version of math.h and the
floor function it provides?

Previously we haven't used cmath as some platforms didn't support it
properly, I recall IRIX being a problem, but am not sure if it extends
further than this. IRIX support has long been deprecated so won't be
a constraint these days. So... using #include<cmath> could well be a
viable solution,

It does still concern me that the MS version of math.h is giving
different results to cmath.

Robert.

[Christian Schulte]

Hi Robert,

I think the problem is linked indeed to a MS math.h problem, but in my
opinion it is not linked directly to the floor function but could affect
even other functions. I don't know what would be the consequences on the
global OSG behaviour but I agree with you that replacing the math.h
include would be the best solution.
Maybe someone could try to compile my little test code with a Microsoft
compiler to see if it is gcc linked or Windows linked.

Cheers,

Christian

[Jason Daly]

I was about to ask why we aren't just using the log2 function here, but
apparently Microsoft doesn't consider this C99-standard function to be
important (ie: it's not included in Microsoft's math.h).

Seems like cmath is indeed the best solution.

--"J"

[Christian Schulte]

Hi,

I just tried the compilation of my test code on Microsoft Visual C++,
and the problem does not appear so it seems gcc linked... I will try to
recompile OSG on my different platforms using cmath in osg/Math, hoping
we don't see any other bugs.
Will keep you informed later today.

Christian

[Sebastian Messerschmidt]

Hi,
I can confirm that it is working correctly on MS Visual C++ 2005 and
2010. Platform is Win7

cheers
Sebastian

[Christian Schulte]

Robert,

I just tried another solution, which works and is maybe, let's say, more
correct from a coding point of view. Indeed, we want to floor a division
of two floats, so we should logically use floorf instead of floor,
because in math.h floor is only double. That's also the reason we don't
have the error with cmath because we have a float overload of the double
floor(double).

What do you think about this solution ? (I will still try to see which
are the consequences of replacing math.h by cmath, but it will be a
little longer :-) )

Cheers,

Christian

[Robert Osfield]

Hi Christian,

On 10 January 2013 09:31, Christian Schulte <Christia...@onera.fr> wrote:
> I just tried another solution, which works and is maybe, let's say, more
> correct from a coding point of view. Indeed, we want to floor a division of
> two floats, so we should logically use floorf instead of floor, because in
> math.h floor is only double. That's also the reason we don't have the error
> with cmath because we have a float overload of the double floor(double).
>
> What do you think about this solution ? (I will still try to see which are
> the consequences of replacing math.h by cmath, but it will be a little
> longer :-) )

Using floorf would be appropriate if the function returns a float and
the parameters internally will be float.

If we do properly embrace cmath then we can probably take the
opportunity to clean up the more than just this particular function in
include/osg/Math.

Robert.

[Lukasz Izdebski]

Hi,

I have a question. Why while reading dds file, to set number of mipmaps it is using function osg::Image::computeNumberOfMipmapLevels( s, t, r ) but not what is written in dds file header ?
I think that dds file knows better how many mipmaps it has inside.

Thank you!

Cheers,
Lukasz

------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=51911#51911

[Christian Schulte]

Hi,

I think it is used as cross-checking method. Indeed, if the calculated
number (using osg::Image) is less than the number of mipmaps specified
in the dds file, the calculated value is kept. On the other hand if the
theoretical number of possible mipmaps is higher than the number of
mipmaps specified by the dds file, the number used is the one of the dds
file. In my opinion, it seems to be possible to have dds files with
wrong mipmap informations...

Cheers,

Christian

[Lukasz Izdebski]

Hi,

But for example we have 512x512 dds texture compressed with DXT1 and in header we have 4 mipmaps.
In this example is meaningless to compute number of mipmaps, because OpenGL ,with my knowledge, does not generate mipmaps for compressed textures. So setting bigger number of mipmaps is wrong.

I didn't find a program which generate dds and write wrong number of mipmaps. If it does change the program :).

Thank you!

Cheers,
Lukasz

------------------
Read this topic online here:

http://forum.openscenegraph.org/viewtopic.php?p=51914#51914

[Christian Schulte]

Hi,

I didn't either find such a program being able to give a wrong number of
mipmaps, I only said that it may exist... Maybe it is better if Wojciech
explains us why he made this calculations. If we can indeed don't make
it it is ok for me, but it does not change anything to the fact that the
floor calculation is wrong on gcc under windows using math.h.
And by the way, I checked, it is not the only place in OSG code where
floor is used. So maybe it is still better to correct this behaviour.
I just recompiled OSG on win xp, win7 and linux and in all cases no
problems appeared by replacing math.h by cmath in include/osg/Math.
However, I saw some other places where math.h has been included, maybe
it will be necessary to change all math.h to cmath and not only the one
in include/osg/Math.

Cheers,

Christian

[Jason Daly]

On 01/11/2013 08:26 AM, Christian Schulte wrote:
> Hi,
>
> I think it is used as cross-checking method. Indeed, if the calculated
> number (using osg::Image) is less than the number of mipmaps specified
> in the dds file, the calculated value is kept. On the other hand if the
> theoretical number of possible mipmaps is higher than the number of
> mipmaps specified by the dds file, the number used is the one of the dds
> file. In my opinion, it seems to be possible to have dds files with
> wrong mipmap informations...

Right, OpenGL requires a certain number of mipmap levels depending on
the dimensions of the image. DDS likely doesn't enforce the same
restriction.

--"J"

[Jason Daly]

On 01/11/2013 09:00 AM, Lukasz Izdebski wrote:
> Hi,
>
> But for example we have 512x512 dds texture compressed with DXT1 and in header we have 4 mipmaps.
> In this example is meaningless to compute number of mipmaps, because OpenGL ,with my knowledge, does not generate mipmaps for compressed textures. So setting bigger number of mipmaps is wrong.

Never mind what I said before :-)

I think the check is to be sure that for some reason, the DDS file
doesn't have too many mipmaps. The loader will take the number of
mipmaps in the file, or the number of mipmaps that OpenGL can handle,
whichever is less.

osg::Image automatically sets the GL_TEXTURE_MAX_LEVEL parameter to the
number of mipmaps that are loaded.

--"J"

[Wojciech Lewandowski]

I was summoned so I respond. Version of DDS plugin before my additions was using this code to compute number of mipmaps (see revision 10369 of ReaderWriterDDS.cpp ):

        //debugging messages        
        float power2_s = logf((float)s)/logf((float)2);
        float power2_t = logf((float)t)/logf((float)2);
        osg::notify(osg::INFO) << "ReadDDSFile INFO : ddsd.dwMipMapCount = "<<ddsd.dwMipMapCount<<std::endl;
        osg::notify(osg::INFO) << "ReadDDSFile INFO : s = "<<s<<std::endl;
        osg::notify(osg::INFO) << "ReadDDSFile INFO : t = "<<t<<std::endl;
        osg::notify(osg::INFO) << "ReadDDSFile INFO : power2_s="<<power2_s<<std::endl;
        osg::notify(osg::INFO) << "ReadDDSFile INFO : power2_t="<<power2_t<<std::endl;
        mipmaps.resize((unsigned int)osg::maximum(power2_s,power2_t),0);

I replaced above with call to osg::Image::computeNumberOfMipmapLevels because this was the same math.  But I also encountered problems with memory access errors with DDS files which did not contain full mipmap chain (with 3D Volume textures mostly) and then I added line which updates numOfMipmaps if file have lower number than theoretical number.

Hope that explanation excludes me from the list of people to blame ;-). I agree that probably using number of mipmaps from file ignoring theoretical number could be the best idea. However, if log function does not work as it should, it has to be fixed as highest priority as its used in many other places in OSG. 

On a side note: I was always surprised by use of floating point math in mipmap number computation: 

logf((float)s)/logf((float)2);

I would rather use folowing fixed point code instead (which would avoid log problem entirely):

num_mipmaps = 1 + max( MostSignificantBit( width ), MostSignificantBit( height ) );

with 

int MostSignifcantBit( unsigned int i )

{

    if( i == 0 ) 

              return -1   

    int bit = 0;

    if( i >= 0x10000 ) { bit += 16; i >>= 16; }

    if( i >= 0x100 ) { bit += 8; i >>= 8; }

    if( i >= 0x10 ) { bit += 4; i >>= 4; }

    if( i >= 0x4 ) { bit += 2; i >>= 2; }

    if( i >= 0x2 ) { bit += 1; i >>= 1; }

    return bit;

}

Cheers,

Wojtek