[osg-users] minor change: move assumeSizedInternallFormat from Texture to Image

Discussion:

Julien Valentin

2018-08-15 22:07:42 UTC

Hi,

if I read all that correctly it seams assumeSizedInternallFormat is only used to create Texture from Image...I think an improvment would be to move assumeSizedInternallFormat from Texture to Image and change its name (getSizedTexInternallFormat).. It would lever some confusion and improve readabitlity of the Texture.cpp code ....

What od you think about that?

Thank you!

Cheers,
Julien

------------------------
Twirling twirling twirling toward freedom

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

Robert Osfield

2018-08-16 07:47:02 UTC

Permalink

Hi Julien,

On Wed, 15 Aug 2018 at 23:44, Julien Valentin

Post by Julien Valentin
if I read all that correctly it seams assumeSizedInternallFormat is only used to create Texture from Image...I think an improvment would be to move assumeSizedInternallFormat from Texture to Image and change its name (getSizedTexInternallFormat).. It would lever some confusion and improve readabitlity of the Texture.cpp code ....
What od you think about that?

This is what I was roughly thinking of. One can't move the function
from Texture to Image as not all textures have images. You could have
it in both places, or just have a function in the osg namespace and
provide all the input variables for it.

As a general guide, a Object::getMethod() typically gets a property
from an object, but if a method computes the value on the fly from
input variables I nornally opt for Object::computeMethod(). assume
is a bit wishy washy so probably isn't ideal - this was used in the
original glTexStorage PR but I probably should have suggested a
change.

We have to think about both master and the 3.6 branch here. The later
the aim is to maintain binary compatibility so our options are more
constrained.

Cheers,
Robert.

Julien Valentin

2018-08-16 15:28:32 UTC

Permalink

Ho I haven't realized the root of existence of this function was the previous contribution to the implementation of glTexStorage.
So Now I can see where the contributor (sure it's Pawel Ksiezopolski) wanted to do.

I don't know why there's so many comments and misses in sizedInternalFormats but it's same to add them at the end

So my proposal is a 2 way process:
-add missing sizedInternalFormats (ex:I found internalformat GL_RGBA16 never mentionned)
-Fix the image less glTexStorage use case with

Code:
if( useTexStorrage)
{
//ensure _internalFormat is sized
GLenum sized_internalFormat = _internalFormat;
if(!isSizedInternalFormat( sized_internalFormat))
sized_internalFormat = assumeSizedInternalFormat( sized_internalFormat, _sourceType ? _sourceType : GL_UNSIGNED_BYTE);
if(!sized_internalFormat)
OSG_FATAL<<"Texture2D : can't generate TextureStorage setup fails: "<<std::endl;
extensions->glTexStorage2D( GL_TEXTURE_2D, (_numMipmapLevels >0)?_numMipmapLevels:1, sized_internalFormat, _textureWidth, _textureHeight);
};

and

Post by Robert Osfield
Hi Julien,
On Wed, 15 Aug 2018 at 23:44, Julien Valentin

This is what I was roughly thinking of. One can't move the function
from Texture to Image as not all textures have images. You could have
it in both places, or just have a function in the osg namespace and
provide all the input variables for it.
As a general guide, a Object::getMethod() typically gets a property
from an object, but if a method computes the value on the fly from
input variables I nornally opt for Object::computeMethod(). assume
is a bit wishy washy so probably isn't ideal - this was used in the
original glTexStorage PR but I probably should have suggested a
change.
We have to think about both master and the 3.6 branch here. The later
the aim is to maintain binary compatibility so our options are more
constrained.
Cheers,
Robert.
_______________________________________________
osg-users mailing list
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
------------------
Post generated by Mail2Forum

------------------------
Twirling twirling twirling toward freedom

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

Robert Osfield

2018-08-16 17:22:09 UTC

Permalink

Hi julien,

On Thu, 16 Aug 2018 at 17:45, Julien Valentin

Post by Julien Valentin
Ho I haven't realized the root of existence of this function was the previous contribution to the implementation of glTexStorage.
So Now I can see where the contributor (sure it's Pawel Ksiezopolski) wanted to do.
I don't know why there's so many comments and misses in sizedInternalFormats but it's same to add them at the end
-add missing sizedInternalFormats (ex:I found internalformat GL_RGBA16 never mentionned)
-Fix the image less glTexStorage use case with
if( useTexStorrage)
{
//ensure _internalFormat is sized
GLenum sized_internalFormat = _internalFormat;
if(!isSizedInternalFormat( sized_internalFormat))
sized_internalFormat = assumeSizedInternalFormat( sized_internalFormat, _sourceType ? _sourceType : GL_UNSIGNED_BYTE);
if(!sized_internalFormat)
OSG_FATAL<<"Texture2D : can't generate TextureStorage setup fails: "<<std::endl;
extensions->glTexStorage2D( GL_TEXTURE_2D, (_numMipmapLevels >0)?_numMipmapLevels:1, sized_internalFormat, _textureWidth, _textureHeight);
};

I did a quick look through Texture.cpp, and there is this entry:

if(useTexStorrage)
{
if(extensions->isTexStorage2DSupported() && _borderWidth == 0)
{
//calculate sized internal format
if(!compressed_image)
{
if(isSizedInternalFormat(_internalFormat))
{
sizedInternalFormat = _internalFormat;
}
else
{
sizedInternalFormat =
assumeSizedInternalFormat((GLenum)image->getInternalTextureFormat(),
(GLenum)image->getDataType());
}
}
else
{

if(isCompressedInternalFormatSupportedByTexStorrage(_internalFormat))
{
sizedInternalFormat = _internalFormat;
}
}
}

useTexStorrage &= sizedInternalFormat != 0;
}

if(useTexStorrage)
{

It's handling the case of having an osg::Image but does illustrate
some of previous PR's introduced (I'm not the author of this
particular set of code so I'm learning by reading...) What it does
have is attempt to select an appropriate sizeInternalFormat, but if
this fails then switch off use of glTexSorage.

It would be good to avoid having different code paths having different
combinations of ways of setting up the sizeInternalFormat and
different ways of falling back when it's not supported, My
inclination would be to have a selectSizeInernalFormat(Image*) method
that can take an optional image pointer, then have this have it's own
with Image and witout image internal logical to do the mapping, and
then have the code be something like:

GLenum useTexStorageWithWithSizedInternalFormat =
extensions->isTextureStorageEnabled ?
selectSizedInternalFormat(optional_image) : 0;
if (useTexStorageWithWithSizedInternalFormat)
{
// glTexStorage code path
}
else
{
// glTexImage code path
}

I'm not 100% sure conflating the bool and internal format in this way
is perfect coding practice but suggest it as just having lots of code
and different variables in play just complicates the code. The code
we are talking about is already overloaded with different code paths
so it's really important to avoid this code becoming even more
spaghetti like, opportunities to untangle the existing spaghetti is
worth a few compromises like reusing an enum as the bool and the size.

Robert.

Julien Valentin

2018-08-16 17:53:25 UTC

Permalink

testing enum as bool is sure bad practice but the first error was to set 0 as default for _sourceFormat...

Post by Robert Osfield
Hi julien,
On Thu, 16 Aug 2018 at 17:45, Julien Valentin

if(useTexStorrage)
{
if(extensions->isTexStorage2DSupported() && _borderWidth == 0)
{
//calculate sized internal format
if(!compressed_image)
{
if(isSizedInternalFormat(_internalFormat))
{
sizedInternalFormat = _internalFormat;
}
else
{
sizedInternalFormat =
assumeSizedInternalFormat((GLenum)image->getInternalTextureFormat(),
(GLenum)image->getDataType());
}
}
else
{
if(isCompressedInternalFormatSupportedByTexStorrage(_internalFormat))
{
sizedInternalFormat = _internalFormat;
}
}
}
useTexStorrage &= sizedInternalFormat != 0;
}
if(useTexStorrage)
{
It's handling the case of having an osg::Image but does illustrate
some of previous PR's introduced (I'm not the author of this
particular set of code so I'm learning by reading...) What it does
have is attempt to select an appropriate sizeInternalFormat, but if
this fails then switch off use of glTexSorage.
It would be good to avoid having different code paths having different
combinations of ways of setting up the sizeInternalFormat and
different ways of falling back when it's not supported, My
inclination would be to have a selectSizeInernalFormat(Image*) method
that can take an optional image pointer, then have this have it's own
with Image and witout image internal logical to do the mapping, and
GLenum useTexStorageWithWithSizedInternalFormat =
extensions->isTextureStorageEnabled ?
selectSizedInternalFormat(optional_image) : 0;
if (useTexStorageWithWithSizedInternalFormat)
{
// glTexStorage code path
}
else
{
// glTexImage code path
}
I'm not 100% sure conflating the bool and internal format in this way
is perfect coding practice but suggest it as just having lots of code
and different variables in play just complicates the code. The code
we are talking about is already overloaded with different code paths
so it's really important to avoid this code becoming even more
spaghetti like, opportunities to untangle the existing spaghetti is
worth a few compromises like reusing an enum as the bool and the size.
Robert.
_______________________________________________
osg-users mailing list
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
------------------
Post generated by Mail2Forum

------------------------
Twirling twirling twirling toward freedom

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

Julien Valentin

2018-08-16 18:04:47 UTC

Permalink

..But my question was
If I'd make a pr with this fix for all Textures

Code:
if( useTexStorrage)
{
//ensure _internalFormat is sized
GLenum sized_internalFormat = _internalFormat;
if(!isSizedInternalFormat( sized_internalFormat))
sized_internalFormat = assumeSizedInternalFormat( sized_internalFormat, _sourceType==0 ? _sourceType : GL_UNSIGNED_BYTE);
if(!sized_internalFormat)
OSG_WARN<<"Texture2D : can't generate TextureStorage for internalFormat: "<<_internalFormat<<" and SourceType: "<<_sourceType==0 ? _sourceType : GL_UNSIGNED_BYTE<<std::endl;
extensions->glTexStorage2D( GL_TEXTURE_2D, (_numMipmapLevels >0)?_numMipmapLevels:1, sized_internalFormat, _textureWidth, _textureHeight);
};

Would you accept it?

Post by Julien Valentin
testing enum as bool is sure bad practice but the first error was to set 0 as default for _sourceFormat...

Post by Robert Osfield
Hi julien,
On Thu, 16 Aug 2018 at 17:45, Julien Valentin

Code:

------------------------
Twirling twirling twirling toward freedom

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

Julien Valentin

2018-08-16 18:08:03 UTC

Permalink

Forgot to mention:
The code posing problem with ImageLess Texture is not in Texture.cpp but it's in derivated TextureXXX.cpp

Post by Julien Valentin
..But my question was
If I'd make a pr with this fix for all Textures
if( useTexStorrage)
{
//ensure _internalFormat is sized
GLenum sized_internalFormat = _internalFormat;
if(!isSizedInternalFormat( sized_internalFormat))
sized_internalFormat = assumeSizedInternalFormat( sized_internalFormat, _sourceType==0 ? _sourceType : GL_UNSIGNED_BYTE);
if(!sized_internalFormat)
OSG_WARN<<"Texture2D : can't generate TextureStorage for internalFormat: "<<_internalFormat<<" and SourceType: "<<_sourceType==0 ? _sourceType : GL_UNSIGNED_BYTE<<std::endl;
extensions->glTexStorage2D( GL_TEXTURE_2D, (_numMipmapLevels >0)?_numMipmapLevels:1, sized_internalFormat, _textureWidth, _textureHeight);
};
Would you accept it?

Post by Julien Valentin
testing enum as bool is sure bad practice but the first error was to set 0 as default for _sourceFormat...

Post by Robert Osfield
Hi julien,
On Thu, 16 Aug 2018 at 17:45, Julien Valentin

------------------------
Twirling twirling twirling toward freedom

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

Robert Osfield

2018-08-17 09:02:41 UTC

Permalink

Hi Julien,

On Fri, 17 Aug 2018 at 03:19, Julien Valentin

This morning I'll do a full code review of the glTexStorage usage and
surround code and have a think how we might be handle the internal
format issue. I'll use the above as a guide as to one possible route.
I can't provide any guidance until I've done the full code review.

Cheers,
Robert.

Robert Osfield

2018-08-17 11:30:22 UTC

Permalink

Hi Julien et. al,

I have so far just focused on the 3.6 branch as this has quite well
constrained use of glTexStorage, so not so many code paths to look at.
To help clean up the sized internal format code up I have created a
GLenum Texture::selectSizedInternalFormat(const osg::Image* image)
const method in place of the original code, functionality it's very
similar, the important thing about it is that is pulls all the logical
for computing he sized internal format into a single method and cleans
up the code that requires it i.e. the glTexStorage code.

The commit to the 3.6 branch is:

https://github.com/openscenegraph/OpenSceneGraph/commit/58a51cbc41e0ad678fb6b818c460c00098f9b727

I've now cherry picked this and am about to test master, then I'll
start looking more widely at the glTexStorage usage in master and see
if there opportunities to use this new helper method and if so will
this require further refinement of the selectSizedInternalFormat(..)
method.

Robert.

Robert Osfield

2018-08-17 18:11:34 UTC

Permalink

I am just heading offline for the evening so a quick update. I have
rolled out the use of GLenum Texture::selectSizedInternalFormat(const
osg::Image* image) to the other glTexStorage cases in the OSG and also
when doing this spotted a bug in the allocation/reuse of GL texture
objects - to fix this issue I can to refactor the code to push the
generateAndAssignTextureObject(..) calls to after where the internal
format is finally decided upon:

https://github.com/openscenegraph/OpenSceneGraph/commit/d37b826a745b4638666b0d53c613033a17df4397

Could those who've see issue with OSG master have a look at master now
to see if it's working fine.

Cheers,
Robert.