OpenSceneGraph Forum Forum Index OpenSceneGraph Forum
Official forum which mirrors the existent OSG mailing lists. Messages posted here are forwarded to the mailing list and vice versa.
 
   FAQFAQ    SearchSearch    MemberlistMemberlist    RulesRules    UsergroupsUsergroups    RegisterRegister 
 Mail2Forum SettingsMail2Forum Settings  ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 
   AlbumAlbum  OpenSceneGraph IRC ChatOpenSceneGraph IRC Chat   SmartFeedSmartFeed 

BindImageTexture Crash


 
Post new topic   Reply to topic    OpenSceneGraph Forum Forum Index -> General
View previous topic :: View next topic  
Author Message
dcfennell
Newbie


Joined: 22 Feb 2013
Posts: 13

PostPosted: Thu Feb 01, 2018 4:19 pm    Post subject:
BindImageTexture Crash
Reply with quote

Hello,

The latest submission for BindImageTexture will crash. It's expecting a buffer object, which is new. The osgcomputeshaders example, which uses BindImageTexture, will also crash with this latest change.

Thank you,

DC
Back to top
View user's profile Send private message
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 396
Location: France

PostPosted: Thu Feb 01, 2018 5:33 pm    Post subject:
Re: BindImageTexture Crash
Reply with quote

Yes, I'm not surprised
my last pr was merged a bit too soon and may introduce a bug...
I mentionned it wasn't clean but Robert merge it anyway
My fault: I take pr more as a channel of communication than real candidates to merge

dcfennell wrote:
Hello,

The latest submission for BindImageTexture will crash. It's expecting a buffer object, which is new. The osgcomputeshaders example, which uses BindImageTexture, will also crash with this latest change.

Thank you,

DC
Back to top
View user's profile Send private message Visit poster's website
dcfennell
Newbie


Joined: 22 Feb 2013
Posts: 13

PostPosted: Thu Feb 01, 2018 5:37 pm    Post subject:
Reply with quote

OK. Thank you. I have a local workaround until a fix is in place.

DC
Back to top
View user's profile Send private message
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 396
Location: France

PostPosted: Thu Feb 01, 2018 7:08 pm    Post subject:
Re: BindImageTexture Crash
Reply with quote

This bug is indirectly related to how unrefaffterapply is implemented
@Robert
Do you think this feature could be modified to release only Image data and not the whole Image (make Image an "Imagedataproxy")?
This mod could lead to
-generalization BufferObject to TextureObject
-serialization homogenization (release only data and keep filename to load it if required)

Hope i'm clear...


mp3butcher wrote:
Yes, I'm not surprised
my last pr was merged a bit too soon and may introduce a bug...
I mentionned it wasn't clean but Robert merge it anyway
My fault: I take pr more as a channel of communication than real candidates to merge

dcfennell wrote:
Hello,

The latest submission for BindImageTexture will crash. It's expecting a buffer object, which is new. The osgcomputeshaders example, which uses BindImageTexture, will also crash with this latest change.

Thank you,

DC
Back to top
View user's profile Send private message Visit poster's website
dcfennell
Newbie


Joined: 22 Feb 2013
Posts: 13

PostPosted: Thu Feb 01, 2018 7:27 pm    Post subject:
Reply with quote

FYI, I did setUnRefImageDataAfterApply to false. The image data becomes valid, but the buffer object is NULL. One OSG example uses a textureBufferObject with BindTextureImage, which is OK. But if you use a texture, there is no buffer object.

Perhaps just a simple check for valid 'getBufferData' and then a check for 'getBufferObject'??

Or even better, if 'to' is NULL, shouldn't just the _target be applied?

Whether image data is NULL or not, the texture object should be valid after _target->apply if it's setup correctly and have a valid ID to bind (with glBindImageTexture).
Back to top
View user's profile Send private message
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 396
Location: France

PostPosted: Thu Feb 01, 2018 8:46 pm    Post subject:
Reply with quote

The bufferobject is null because of the design used for unRefImageDataAfterApply feature
Texture owns textureobjects in order to deref bufferdata

What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new class TextureObject inheriting BufferObject)->GraphicObject intact

dcfennell wrote:
FYI, I did setUnRefImageDataAfterApply to false. The image data becomes valid, but the buffer object is NULL. One OSG example uses a textureBufferObject with BindTextureImage, which is OK. But if you use a texture, there is no buffer object.

Perhaps just a simple check for valid 'getBufferData' and then a check for 'getBufferObject'??

Or even better, if 'to' is NULL, shouldn't just the _target be applied?

Whether image data is NULL or not, the texture object should be valid after _target->apply if it's setup correctly and have a valid ID to bind (with glBindImageTexture).
Back to top
View user's profile Send private message Visit poster's website
robertosfield
OSG Project Lead


Joined: 18 Mar 2009
Posts: 11360

PostPosted: Fri Feb 02, 2018 9:21 am    Post subject:
BindImageTexture Crash
Reply with quote

On 1 February 2018 at 20:46, Julien Valentin <> wrote:
Quote:
The bufferobject is null because of the design used for unRefImageDataAfterApply feature
Texture owns textureobjects in order to deref bufferdata

What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new classTextureObject)->GraphicObject intact

Umm.... lets change something that's been working fine for well over a
decade to fix an issue recently introduce by an ill-considered
submission.

I will need to look deeper into the issue and decide what is best to
do. I don't have the time to do this right away. Might have time
next week.

Robert.


------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
robertosfield
OSG Project Lead


Joined: 18 Mar 2009
Posts: 11360

PostPosted: Fri Feb 02, 2018 10:08 am    Post subject:
BindImageTexture Crash
Reply with quote

I have reverted the PR, this resolved the osgcomputeshaders bug.

On 2 February 2018 at 09:17, Robert Osfield <> wrote:
Quote:
On 1 February 2018 at 20:46, Julien Valentin <> wrote:
Quote:
The bufferobject is null because of the design used for unRefImageDataAfterApply feature
Texture owns textureobjects in order to deref bufferdata

What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new classTextureObject)->GraphicObject intact

Umm.... lets change something that's been working fine for well over a
decade to fix an issue recently introduce by an ill-considered
submission.

I will need to look deeper into the issue and decide what is best to
do. I don't have the time to do this right away. Might have time
next week.

Robert.


------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 396
Location: France

PostPosted: Fri Feb 02, 2018 3:50 pm    Post subject:
Re: BindImageTexture Crash
Reply with quote

https://github.com/openscenegraph/OpenSceneGraph/pull/465

Here's what I'm thinking of....sorry for the huge commit
ChangeLog (in absence of commit history)

    1) introduce a TextureGraphicObject ( buffered_object PCTOs )
    2) inject a base class PerContextGraphicObject (base of BufferObject, TextureGraphicObject (bufferedPCTOs), ProgramsObject ..)
    3) deprecate Image less Texture by adding an Image with NULL data and make unrefimageafterapply just deallocate Image (may require further data==0 tests to ensure retrocomp)
    4) remove buffered_object from Texture to use the TextureGraphicObject of its Image instead


I'll try to clean the PR if community is happy with it


robertosfield wrote:
I have reverted the PR, this resolved the osgcomputeshaders bug.

On 2 February 2018 at 09:17, Robert Osfield <> wrote:
Quote:
On 1 February 2018 at 20:46, Julien Valentin <> wrote:
Quote:
The bufferobject is null because of the design used for unRefImageDataAfterApply feature
Texture owns textureobjects in order to deref bufferdata

What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new classTextureObject)->GraphicObject intact

Umm.... lets change something that's been working fine for well over a
decade to fix an issue recently introduce by an ill-considered
submission.

I will need to look deeper into the issue and decide what is best to
do. I don't have the time to do this right away. Might have time
next week.

Robert.


------------------
Post generated by Mail2Forum
[list=][/list]
    Back to top
    View user's profile Send private message Visit poster's website
    mp3butcher (Julien Valentin)
    Appreciator


    Joined: 17 Feb 2010
    Posts: 396
    Location: France

    PostPosted: Wed Feb 07, 2018 6:59 pm    Post subject:
    Re: BindImageTexture Crash
    Reply with quote

    Okay,
    I've gone totally mad with this PR...(chainsaw night fever)
    Mea Culpa

    So to sum up the problem:
    BindImageTexture stateattribute is not a textureattribute (_imageunit != _textureunit) but it sometimes have to bind texture object on a textureunit in case data of the associated texture has gone dirty

    The current implementation relies on user to setup BindImageTexture::_targettex as texture attribute of "the same stateset" (and then relies on this texatt to upload dirtied data if required)
    It works well but involves a systematic texture->apply (texobj.bind) overcost at each use...

    to make BindImageTexture "autonomous", We then need:
    - a way to know if texture have to be applied :
    - a textureunit on which to applied

    What I've proposed :
    - clarify semantic given to textures::_modifiedcount to be the textureobjectmodifiedcount. So, as texobjs are owned by Texture, i putted modifiedcount in Texture and removed it from daughter classes.
    - in LayeredTextures (cubemap, texture2Darray) i changed modifiedcount to layermodifiedcount these flags doesn't have the same purpose as the textureobjectmodifiedcount as it's just a inner mechanism not to upload unchanged layer image (not related to the textureobject state).

    and Then I added a textureunit prop in BindImageTexture, this tu is used to applytexattribute(_targettex) if required.
    (Note it's user charge to set this prop correctely according other uses of the _targettex as texattribute in the rest of your scenegraph)

    https://github.com/openscenegraph/OpenSceneGraph/pull/467
    My pr has been rejected:
    Quote:
    Making _modifiedCount part of the base class is broken because you end up having to duplicate the data structure for the array version of texture.

    Also I can grasp how 12 changed files could be at all a minimal change set. You've merged three set of changes into one commit.

    Adding the texture unit as into BindImageTexture just feels like this class is getting more a more tightly coupled state that breaks the orthogonal design that the OSG and OpenGL try to maintain, the fact you are having to do this tight coupling suggests that approach is far from idea and may need revising at a fundamental level.

    I'm closing this PR as it just contains too many different problems.


    so i'm now asking community reviews and insights because I can't see myself a better compromise to finish BindImageTexture.

    Thanks in advance

    mp3butcher wrote:
    https://github.com/openscenegraph/OpenSceneGraph/pull/465

    Here's what I'm thinking of....sorry for the huge commit
    ChangeLog (in absence of commit history)

      1) introduce a TextureGraphicObject ( buffered_object PCTOs )
      2) inject a base class PerContextGraphicObject (base of BufferObject, TextureGraphicObject (bufferedPCTOs), ProgramsObject ..)
      3) deprecate Image less Texture by adding an Image with NULL data and make unrefimageafterapply just deallocate Image (may require further data==0 tests to ensure retrocomp)
      4) remove buffered_object from Texture to use the TextureGraphicObject of its Image instead


    I'll try to clean the PR if community is happy with it


    robertosfield wrote:
    I have reverted the PR, this resolved the osgcomputeshaders bug.

    On 2 February 2018 at 09:17, Robert Osfield <> wrote:
    Quote:
    On 1 February 2018 at 20:46, Julien Valentin <> wrote:
    Quote:
    The bufferobject is null because of the design used for unRefImageDataAfterApply feature
    Texture owns textureobjects in order to deref bufferdata

    What I propose/ask to Robert is to release only data of the Image data keeping the chain Texture->BufferData(Image)->BufferObject(a new classTextureObject)->GraphicObject intact

    Umm.... lets change something that's been working fine for well over a
    decade to fix an issue recently introduce by an ill-considered
    submission.

    I will need to look deeper into the issue and decide what is best to
    do. I don't have the time to do this right away. Might have time
    next week.

    Robert.


    ------------------
    Post generated by Mail2Forum
    [list=][/list]
      Back to top
      View user's profile Send private message Visit poster's website
      robertosfield
      OSG Project Lead


      Joined: 18 Mar 2009
      Posts: 11360

      PostPosted: Thu Feb 08, 2018 10:12 am    Post subject:
      BindImageTexture Crash
      Reply with quote

      Hi Juilien,

      On 7 February 2018 at 18:59, Julien Valentin <> wrote:
      Quote:
      So to sum up the problem:
      - BindImageTexture stateattribute is not a textureattribute (_imageunit != _textureunit) but it sometimes have to bind texture object on a textureunit in case data of the associated texture has gone dirty

      It is a form of texture attribute, it relates directly to texture, the
      only thing that decouples BindImageTexture from being a traditional
      OSG style texture attribute is that you don't always need to apply the
      texture unit when applying it as the interface provides this. This
      duality of concepts/implementation is down to OpenGL 4.x introducing
      APIs that don't work like previous OpenGL APIs worked.

      The only reason I can see for BindImageTexture attribute needing to
      bind a particular texture unit being applying the texture would be to
      avoid osg::State lazy state updating for not apply a texture for a
      texture unit.

      In normal OSG usage There are several things going on when you apply a
      Texture::apply(), the create a texture object is not already created,
      any data is uploaded to this texture object if required and the
      texture object is made current to currently set up texture unit
      (osg::State handles this part). Once a texture is created doing a
      Texture::apply() only really does the binding of the texture object to
      the texture unit.

      For BindImageTexture my guess is that if you do need to respond to the
      dirty Texture by calling apply() is to just apply it on the current
      texture unit and then may sure osg::State's lazy state updating knows
      about it. The osg::State::applyTextureAttribute(unsigned int unit,
      const StateAttribute* attribute) method is the tool for the job. The
      actual texture unit is probably irrelevant as the OSG should
      automatically reset if required so you could using unit=0 just fine,
      or the value of osg::State::getActiveTextureUnit() if you want to
      avoid changing as much state as possible,


      Quote:
      We then need a way to know if texture have to be applied:

      What I've propose :
      - clarify semantic given to textures::_modifiedcount to be the textureobjectmodifiedcount. Son as tos belong to Texture i putted modifiedcount in Texture and removed it from daughter classes

      That's a dreadful idea given it directly conflicts with the
      array/cubmemap version of texture subclasses, that alone should have
      raised a red flag with your implementation not being appropriate.

      Perhaps a virtual bool Texture::isDirty(const unsigned int contextID)
      would be the way to implement a generic mechanism. This is the
      standard object orientation way of decoupling interface from
      implementation.


      Quote:
      -in LayeredTextures (cubemap, texture2Darray) i changed modifiedcount to layermodifiedcount these flags doesn't have the same purpose as the textureobjectmodifiedcount as it's just a inner mecanism not t upload unchanged layer image (not related to the to)

      modifiedCount implementation has the same function in all Texture
      subclasses, of you tracking whether image data and associated texture
      objects are in sync or not.


      ------------------
      Post generated by Mail2Forum
      Back to top
      View user's profile Send private message
      Display posts from previous:   
      Post new topic   Reply to topic    OpenSceneGraph Forum Forum Index -> General All times are GMT
      Page 1 of 1

       
      Jump to:  
      You cannot post new topics in this forum
      You cannot reply to topics in this forum
      You cannot edit your posts in this forum
      You cannot delete your posts in this forum
      You cannot vote in polls in this forum
      You cannot attach files in this forum
      You cannot download files in this forum

      Similar Topics
      Topic Author Forum Replies Posted
      No new posts [Toward BindImageTexture completness] mp3butcher General 1 Wed Feb 07, 2018 8:56 pm View latest post
      No new posts Crash in ReaderWriterCURL::readFile w... hartwigw General 3 Sun Dec 17, 2017 11:46 pm View latest post
      No new posts osg::Text::setBackdropType crash with... Carlo Lanzotti - DynaD... General 7 Wed Sep 20, 2017 2:40 pm View latest post
      No new posts Fix for osg::Program crash Colin McDonald Submission 5 Mon Sep 11, 2017 4:43 pm View latest post
      No new posts Crash with VBO and shared context jumaroch General 0 Tue Jul 25, 2017 1:24 pm View latest post


      Board Security Anti Bot Question MOD - phpBB MOD against Spam Bots
      Powered by phpBB © 2001, 2005 phpBB Group
      Protected by Anti-Spam ACP