GNOME Bugzilla – Bug 720336
plugins: implement GLTextureUploadMeta user data copy
Last modified: 2013-12-19 16:31:43 UTC
This patch makes the copies of a buffer reference their own GLTextureUploadMeta user data and prevent the original buffer accessing already freed memory if its copies has been released and freed.
Created attachment 264091 [details] [review] plugins: implement GLTextureUploadMeta user data copy
Why not make it a real copy, i.e. also referencing the newly created texture, instead of creating yet another?
Created attachment 264493 [details] [review] plugins: implement GLTextureUploadMeta user data copy Does this patch work for you?
(In reply to comment #2) > Why not make it a real copy, i.e. also referencing the newly created texture, > instead of creating yet another? There is no guarantee that the texture handle you will provide to the upload function will be the same, this is why I didn't try to copy the existing texture.
Do you mean there are cases when we would just recycle the texture id with other characteristics e.g. dimensions? BTW, that hurts performance. We'd need a way to notify the consumer of the upload() function to prefer a strategy where he would try to re-use the texture.
(In reply to comment #5) > Do you mean there are cases when we would just recycle the texture id with > other characteristics e.g. dimensions? > > BTW, that hurts performance. We'd need a way to notify the consumer of the > upload() function to prefer a strategy where he would try to re-use the > texture. Indeed. (In reply to comment #3) > Created an attachment (id=264493) [details] [review] > plugins: implement GLTextureUploadMeta user data copy > > Does this patch work for you? It works thanks :)
(In reply to comment #6) > (In reply to comment #5) > > Do you mean there are cases when we would just recycle the texture id with > > other characteristics e.g. dimensions? > > > > BTW, that hurts performance. We'd need a way to notify the consumer of the > > upload() function to prefer a strategy where he would try to re-use the > > texture. > > Indeed. So, the only case where it could fail is if the texture id is recycled, i.e. glDeleteTexture() + glGenTExtures() to get the same id again, and if the associated size is changed. We could additionally check for dimensions changes in _upload() and not just texture id. However, this causes the GL pipeline to block (glGet*). Therefore, an efficient way to solve that could be to let the caller notify upload, through extra arguments, of the relevant dimensions. This causes API/ABI changes. We could also mandate, i.e. define and document, that once we established the texture info, it should not change through subsequent upload() calls. > (In reply to comment #3) > > Created an attachment (id=264493) [details] [review] [details] [review] > > plugins: implement GLTextureUploadMeta user data copy > > > > Does this patch work for you? > > It works thanks :) At least, it works the same as before. :)
(In reply to comment #6) > > Created an attachment (id=264493) [details] [review] [details] [review] > > plugins: implement GLTextureUploadMeta user data copy > > > > Does this patch work for you? > > It works thanks :) Pushed it to git master branch. Thanks.
(In reply to comment #7) > So, the only case where it could fail is if the texture id is recycled, i.e. > glDeleteTexture() + glGenTExtures() to get the same id again, and if the > associated size is changed. > > We could additionally check for dimensions changes in _upload() and not just > texture id. However, this causes the GL pipeline to block (glGet*). Therefore, > an efficient way to solve that could be to let the caller notify upload, > through extra arguments, of the relevant dimensions. This causes API/ABI > changes. > > We could also mandate, i.e. define and document, that once we established the > texture info, it should not change through subsequent upload() calls. I opened bug #720728 to track the status of this case since I am going to close this present bug.
While we are at it, another optimization is also possible and tracked in bug #720727
commit bf8f244de3262c782f425252afe919dbad0f554b Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Thu Dec 12 17:01:29 2013 +0000 plugins: implement GLTextureUploadMeta user data copy. Makes the copies of a buffer reference their own GLTextureUploadMeta user data and prevent the original buffer accessing already freed memory if its copies has been released and freed. https://bugzilla.gnome.org/show_bug.cgi?id=720336 [Propagate the original meta texture to the copy too] Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>