GNOME Bugzilla – Bug 639558
Pixbuf new_from_data() vs overload
Last modified: 2011-08-17 22:13:33 UTC
Created attachment 178349 [details] [review] patch and test case Is there a reason Gtk2::Gdk::Pixbuf->new_from_data() demands SvPOK of its data input? I thought it might SvPV to stringize in the usual way. Doing so would allow overloads and magic to work there the same way as they do elsewhere. (It normally seems like a bad idea to enquire into the nature of a scalar. For a string ask for a string and let perl worry how to get it :-) Possible simplification below, copying out the SvPV data into a malloced block for the pixbuf data area rather than an newSVsv. (I plan also to check the length of the input, to see the given width*height and rowstride won't overrun the data area. That would be with or without this change. Maybe SvPVbytes too if it unmangles any 0x80 to 0xFF wide chars, and/or guards against chars 0x100 up.)
(In reply to comment #0) > Is there a reason Gtk2::Gdk::Pixbuf->new_from_data() demands SvPOK of its data > input? I thought it might SvPV to stringize in the usual way. Doing so would > allow overloads and magic to work there the same way as they do elsewhere. Sounds good to me. > Possible simplification below, copying out the SvPV data into a malloced block > for the pixbuf data area rather than an newSVsv. Also good. But any particular reason you use perl's memory-handling macros? Nearly all the other code in Glib and Gtk2 uses glib's things (g_new, g_memdup).
I can't remember why I thought the perl malloc was a good idea, only maybe to use perl mallocs from within the perl level. Both die horribly when out of memory if I'm not mistaken, though no doubt in subtly different ways. Do the perl ones attempt exit/cleanups better? Otherwise I don't suppose it makes a difference.
Yeah, I guess you're right. I thought that with g_new and friends, there might be a possibility of some memory pooling happening, but that can also be said about New and friends. Patch committed with slight adjustments. Thanks.