Re: [osg-users] Image::readPixels (3.0.1)
Hi Andrew, Thanks for the callback code. Having a look at how this uses osg::Image and how readPixels() calls allocateImage() it looks to me the problems stems from the the later call not passing in the _packing value so this defaults to 0. Your suggested fix of passing in the _packing value to allocateImage() addresses this. Looking at the API of the allocateImage() and readPixels() I can't help but feel that readPixels() should match allocateImage() in having the packing passed in as well, so avoid the need for you to call setPacking(4). I have made this change and checked it into svn/trunk. This will leave the original default behaviour in place but allow users like yourself to set a specific packing value. Robert. On 29 March 2012 01:20, Andrew Cunningham wrote: > Just imagine this scenario of a DrawCallBack > > struct SnapImage : public osg::Camera::DrawCallback > { > SnapImage(unsigned int format): > _snapImage(false),_format(format) > { > _image = new osg::Image; > _image->setPacking(4); > } > > ~SnapImage(){}; > virtual void operator () (osg::RenderInfo& renderInfo) const > { > > if (!_snapImage) return; > > osg::notify(osg::NOTICE)<<"Camera callback"< > osg::Camera* camera = renderInfo.getCurrentCamera(); > osg::Viewport* viewport = camera ? camera->getViewport() : 0; > > > if (viewport && _image.valid()) > { > > _image->readPixels(int(viewport->x()),int(viewport->y()),int(viewport->width()),int(viewport->height()), > _format, > GL_UNSIGNED_BYTE); > > osg::notify(osg::NOTICE)<<"Taken screenshot.." < } > > _snapImage = false; > } > > mutable bool _snapImage; > mutable unsigned int _format; > mutable osg::ref_ptr _image; > }; > > > > This will likely crash when width is not a multiple of 4. I not calling > setPacking() after the image has been allocated. > I wanted a packing of 4 as this matches the "packing" of BMP > > -- > Read this topic online here: > http://forum.openscenegraph.org/viewtopic.php?p=46659#46659 > > > > > > ___ > osg-users mailing list > osg-users@lists.openscenegraph.org > http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org ___ osg-users mailing list osg-users@lists.openscenegraph.org http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
Re: [osg-users] Image::readPixels (3.0.1)
Just imagine this scenario of a DrawCallBack struct SnapImage : public osg::Camera::DrawCallback { SnapImage(unsigned int format): _snapImage(false),_format(format) { _image = new osg::Image; _image->setPacking(4); } ~SnapImage(){}; virtual void operator () (osg::RenderInfo& renderInfo) const { if (!_snapImage) return; osg::notify(osg::NOTICE)<<"Camera callback"readPixels(int(viewport->x()),int(viewport->y()),int(viewport->width()),int(viewport->height()), _format, GL_UNSIGNED_BYTE); osg::notify(osg::NOTICE)<<"Taken screenshot.." <_image; }; This will likely crash when width is not a multiple of 4. I not calling setPacking() after the image has been allocated. I wanted a packing of 4 as this matches the "packing" of BMP -- Read this topic online here: http://forum.openscenegraph.org/viewtopic.php?p=46659#46659 ___ osg-users mailing list osg-users@lists.openscenegraph.org http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
Re: [osg-users] Image::readPixels (3.0.1)
Hi Andrew, Calling setPacking after you've called setData or allocateImage will break the packing of the image and is not something you should do. There will circumstances that it would be safe, but in general it won't be. What to do about this flexibility of osg::Image that provides the setPacking method without reallocating the image data is a question we should ask. One could just remove the set method completely to avoid the issue, or perhaps just add a comment that setPacking should only be done with the data already allocated and assigned to the osg::Image is compatible with this change. The later change would not change the API, but also wouldn't close the potential usage error. The OSG is an advanced API so I'm more inclined to retain the flexibility that power users can take advantage of rather than the trying to catch all the possible usage errors that end users could come up with. Robert. On 27 March 2012 18:53, Andrew Cunningham wrote: > I think there is an issue in Image::readPixels that I noted in 2.8.x > > Assume you have set the Image "packing" is set to say 4 , via > image->setPacking(4) before this call. > glReadPixels might try and store 'too much' data into _data ( overwriting > memory) because glReadPixels will be expecting the pixel data is using a > packing of 4 but allocateImage was passed a "packing" of 1 via the default > parameter. > > It certainly cause my code to crash depending on the variations of width and > height. > > void Image::readPixels(int x,int y,int width,int height, > GLenum format,GLenum type) > { > allocateImage(width,height,1,format,type); > > glPixelStorei(GL_PACK_ALIGNMENT,_packing); > > glReadPixels(x,y,width,height,format,type,_data); > } > > changing code to > > allocateImage(width,height,1,format,type,_packing); > > seems to fix the issue. > It's pretty subtle, and may only affect some people who need a particular > format for the image data, and maybe calling setPacking is an abuse of the > API so this can be seen as informational only rather than a 'bug' report. > > -- > Read this topic online here: > http://forum.openscenegraph.org/viewtopic.php?p=46637#46637 > > > > > > ___ > osg-users mailing list > osg-users@lists.openscenegraph.org > http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org ___ osg-users mailing list osg-users@lists.openscenegraph.org http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
[osg-users] Image::readPixels (3.0.1)
I think there is an issue in Image::readPixels that I noted in 2.8.x Assume you have set the Image "packing" is set to say 4 , via image->setPacking(4) before this call. glReadPixels might try and store 'too much' data into _data ( overwriting memory) because glReadPixels will be expecting the pixel data is using a packing of 4 but allocateImage was passed a "packing" of 1 via the default parameter. It certainly cause my code to crash depending on the variations of width and height. void Image::readPixels(int x,int y,int width,int height, GLenum format,GLenum type) { allocateImage(width,height,1,format,type); glPixelStorei(GL_PACK_ALIGNMENT,_packing); glReadPixels(x,y,width,height,format,type,_data); } changing code to allocateImage(width,height,1,format,type,_packing); seems to fix the issue. It's pretty subtle, and may only affect some people who need a particular format for the image data, and maybe calling setPacking is an abuse of the API so this can be seen as informational only rather than a 'bug' report. -- Read this topic online here: http://forum.openscenegraph.org/viewtopic.php?p=46637#46637 ___ osg-users mailing list osg-users@lists.openscenegraph.org http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org