Re: AddGadgetItem image bug, all OS
Posted: Fri Feb 10, 2017 6:48 am
This thread has gotten long and as I've exhausted all avenues and believe I've proven beyond doubt that this is a PB problem, but still have doubters, please allow me to conclude my bug report, and what i've learned since encountering this problem 4-5 days ago and having to get stuck into it.
THE PROBLEM: AddGadgetItem(), which internally calls ImageList_Add (using Windows for this example but problem is also consistent on Linux and Mac), fails to call it if it detects the handle has been used before, instead incorrectly linking to that previous handle's instance of the image, which is the wrong image. (I believe this is a well-meaningfully intended cache-style feature, but unfortunately has turned out nothing but problematic).
How I believe it can be fixed: Simply always get AddGadgetItem to call ImageList_Add. (Or something like a #PB_NoImageCache parameter).
I can only guess, but I'd imagine it would be quick and trivial to fix.
Positives: Fixes every issue in this thread.
Negatives: None - I cannot see how it could have any negative effect or 'break' any existing PB code, because all it would be doing is drawing the intended image instead of the one from cache. The intended purpose of reusing existing images is no longer there, but this issue suggests that if that is required it should and must be done by the programmer, not the compiler.
Who is affected: Everyone using a List/Combo gadget with icons added by PB's AddGadgetItem() (but not native ie winapi). Win+Linux+Mac. Seemingly all PB versions.
There are two types of use, and both are affected but in slightly different ways:
1) those who use CreateImage() with every icon/AddGadgetItem, but keep the image in memory (no FreeImage). (Max correct images = <10000, but then big problems).
Problem 1: You cannot create more than 10000 images in Windows due to the handle limit (although you won't experience handle collisions), even though ImageList supports hundreds of thousands+ images.
Problem 2: Because the ImageList only uses its own independent bitmap those images do nothing but waste memory and handles - if you have 10000 images open you can't even open a file let alone another image, yet because ImageList's bitmap is independent there is no reason to keep the handles open and images wasting memory.
Problem 3: Speed. Because you have to call CreateImage for every icon its a lot slower, especially when adding 10000+ images. When using dynamically-created images 1 image is usually enough, and that's a lot faster.
2) those who use CreateImage() with every icon/AddGadgetItem, but then FreeImage() after adding to the list. (Max correct images <256)
Problem: You'll experience handle collisions in Windows because of this AddGadgetItem() bug/feature/limitation. If using 256 images in your list you'll experience this approximately every 2nd-3rd execution with typically 1-10 collisions. If using more than 512 images you'll almost always experience this problem, and it's tricky to notice because it's still using valid (but wrong/old) icons. (Tested XP32 and Win7-64, handle collisions the same on both)
Both of these approaches have restrictions imposed on them by AddGadgetItem(), even though the OS imposes no such restrictions and supports 1000000+ images, yet in neither case can you correctly add 10,000 icons to your list, and in the 2nd/FreeImage approach you can rarely even add 256 icons before encountering the handle collision problem. (And please don't say "well you shouldn't be using 256 or 10000 images" (freak suggested that i'm a special case), because the OS is perfectly fine with it, and every app is different with its own needs as to what gadgets work best etc for the specific task at hand - Windows Explorer for example has no problem with a folder having tens of thousands of files each with their own icon in its SysListview, and if there was a better way to display it im sure MS would've done so - and Explorer clearly doesn't have thousands of image handles open).
Are there any significant benefits to this apparent feature?
All I can think of, is it was well-meaningfully intended to save memory by using the existing version of an image if it detects a previously-used handle. (The LV_ITEM structure for example lets you link to any index in the imagelist ie. to re-use images if needed, but because AddGadgetItem doesn't have a parameter for this/doesn't expose the ImageList itself i'm guessing Fred with his heart in the right place added his own implementation of that to support previous images - sounds good on paper!). However that logic is flawed as 1) handles can be the same even when FreeImage'd and CreateImage'd and different location in memory (that's how i first found the problem), and 2) existing images can be overdrawn with new content, which is perfectly valid, yet this feature doesn't recognise them as new images. Not only does it impose these problems, it also means we can't obtain maximum speed due to having to CreateImage with every icon, when in the case of dynamically-generated images 1 is enough (and i've timed it, it's a lot faster). It's only 16x16 anyway (an ImageList with 256 images = a bitmap of only 16x4192 pixels = 201216 bytes 24bit RGB), so there's not much saving anyway, and this (previous handles) is something the programmer can easily detect with a simple integer array + IsImage() if they wanted to implement their own handle-reuse system.
I believe I have proven beyond reasonable doubt:
1: PB's AddGadgetItem() internally calls ImageList_Add, but not when it detects a handle has previously been used. That's the problem.
2: ImageList_Add does however correctly add the new image even if it has a previously-used handle (just not when AddGadgetItem() doesn't call it!)
3: ImageList's bitmap is independent of the images it is fed. When you ImageList_Add it grows its internal bitmap in size by 16 pixels height, and paints your image into its bitmap.
4: CreateImage()'d images that are added can be FreeImage'd immediately after being added to the ImageList, because they're now drawn onto the ImageList's internal bitmap. Clearly one of the reasons why it has its own internal bitmap is so that you don't need to keep thousands of handles open, as also supported by the fact that it supports hundreds-of-thousands+ of images, and the fact apps like Windows Explorer with tens of thousands of icons in their lists don't have tens of thousands of handles open.
5: ImageList as i've tested supports hundreds of thousands of images (im guessing resource-limited or 32bit ceiling).
6: My version of AddGadgetItem shows it can easily be done, and not by doing anything special as we're basically doing exactly the same thing. There should be no need to reinvent the wheel or create my own custom Canvasgadget item. The only real difference between AddGadgetItem and MyAddGadgetItem is that mine always called ImageList_Add, whereas PB's AddGadgetItem() only calls it if it hasn't seen the handle id before.
7: While well-meaning, this feature doesn't really offer much anyway (just a tiny memory saving), yet creates all these problems.
8: This is a PB limitation/bug, one that seemingly doesn't need to or shouldn't be there, yet the OS does not impose any such restrictions/problems.
Thankyou for your time and giving me a chance.
THE PROBLEM: AddGadgetItem(), which internally calls ImageList_Add (using Windows for this example but problem is also consistent on Linux and Mac), fails to call it if it detects the handle has been used before, instead incorrectly linking to that previous handle's instance of the image, which is the wrong image. (I believe this is a well-meaningfully intended cache-style feature, but unfortunately has turned out nothing but problematic).
How I believe it can be fixed: Simply always get AddGadgetItem to call ImageList_Add. (Or something like a #PB_NoImageCache parameter).
I can only guess, but I'd imagine it would be quick and trivial to fix.
Positives: Fixes every issue in this thread.
Negatives: None - I cannot see how it could have any negative effect or 'break' any existing PB code, because all it would be doing is drawing the intended image instead of the one from cache. The intended purpose of reusing existing images is no longer there, but this issue suggests that if that is required it should and must be done by the programmer, not the compiler.
Who is affected: Everyone using a List/Combo gadget with icons added by PB's AddGadgetItem() (but not native ie winapi). Win+Linux+Mac. Seemingly all PB versions.
There are two types of use, and both are affected but in slightly different ways:
1) those who use CreateImage() with every icon/AddGadgetItem, but keep the image in memory (no FreeImage). (Max correct images = <10000, but then big problems).
Problem 1: You cannot create more than 10000 images in Windows due to the handle limit (although you won't experience handle collisions), even though ImageList supports hundreds of thousands+ images.
Problem 2: Because the ImageList only uses its own independent bitmap those images do nothing but waste memory and handles - if you have 10000 images open you can't even open a file let alone another image, yet because ImageList's bitmap is independent there is no reason to keep the handles open and images wasting memory.
Problem 3: Speed. Because you have to call CreateImage for every icon its a lot slower, especially when adding 10000+ images. When using dynamically-created images 1 image is usually enough, and that's a lot faster.
2) those who use CreateImage() with every icon/AddGadgetItem, but then FreeImage() after adding to the list. (Max correct images <256)
Problem: You'll experience handle collisions in Windows because of this AddGadgetItem() bug/feature/limitation. If using 256 images in your list you'll experience this approximately every 2nd-3rd execution with typically 1-10 collisions. If using more than 512 images you'll almost always experience this problem, and it's tricky to notice because it's still using valid (but wrong/old) icons. (Tested XP32 and Win7-64, handle collisions the same on both)
Both of these approaches have restrictions imposed on them by AddGadgetItem(), even though the OS imposes no such restrictions and supports 1000000+ images, yet in neither case can you correctly add 10,000 icons to your list, and in the 2nd/FreeImage approach you can rarely even add 256 icons before encountering the handle collision problem. (And please don't say "well you shouldn't be using 256 or 10000 images" (freak suggested that i'm a special case), because the OS is perfectly fine with it, and every app is different with its own needs as to what gadgets work best etc for the specific task at hand - Windows Explorer for example has no problem with a folder having tens of thousands of files each with their own icon in its SysListview, and if there was a better way to display it im sure MS would've done so - and Explorer clearly doesn't have thousands of image handles open).
Are there any significant benefits to this apparent feature?
All I can think of, is it was well-meaningfully intended to save memory by using the existing version of an image if it detects a previously-used handle. (The LV_ITEM structure for example lets you link to any index in the imagelist ie. to re-use images if needed, but because AddGadgetItem doesn't have a parameter for this/doesn't expose the ImageList itself i'm guessing Fred with his heart in the right place added his own implementation of that to support previous images - sounds good on paper!). However that logic is flawed as 1) handles can be the same even when FreeImage'd and CreateImage'd and different location in memory (that's how i first found the problem), and 2) existing images can be overdrawn with new content, which is perfectly valid, yet this feature doesn't recognise them as new images. Not only does it impose these problems, it also means we can't obtain maximum speed due to having to CreateImage with every icon, when in the case of dynamically-generated images 1 is enough (and i've timed it, it's a lot faster). It's only 16x16 anyway (an ImageList with 256 images = a bitmap of only 16x4192 pixels = 201216 bytes 24bit RGB), so there's not much saving anyway, and this (previous handles) is something the programmer can easily detect with a simple integer array + IsImage() if they wanted to implement their own handle-reuse system.
I believe I have proven beyond reasonable doubt:
1: PB's AddGadgetItem() internally calls ImageList_Add, but not when it detects a handle has previously been used. That's the problem.
2: ImageList_Add does however correctly add the new image even if it has a previously-used handle (just not when AddGadgetItem() doesn't call it!)
3: ImageList's bitmap is independent of the images it is fed. When you ImageList_Add it grows its internal bitmap in size by 16 pixels height, and paints your image into its bitmap.
4: CreateImage()'d images that are added can be FreeImage'd immediately after being added to the ImageList, because they're now drawn onto the ImageList's internal bitmap. Clearly one of the reasons why it has its own internal bitmap is so that you don't need to keep thousands of handles open, as also supported by the fact that it supports hundreds-of-thousands+ of images, and the fact apps like Windows Explorer with tens of thousands of icons in their lists don't have tens of thousands of handles open.
5: ImageList as i've tested supports hundreds of thousands of images (im guessing resource-limited or 32bit ceiling).
6: My version of AddGadgetItem shows it can easily be done, and not by doing anything special as we're basically doing exactly the same thing. There should be no need to reinvent the wheel or create my own custom Canvasgadget item. The only real difference between AddGadgetItem and MyAddGadgetItem is that mine always called ImageList_Add, whereas PB's AddGadgetItem() only calls it if it hasn't seen the handle id before.
7: While well-meaning, this feature doesn't really offer much anyway (just a tiny memory saving), yet creates all these problems.
8: This is a PB limitation/bug, one that seemingly doesn't need to or shouldn't be there, yet the OS does not impose any such restrictions/problems.
Thankyou for your time and giving me a chance.