[GIL] Adding base_channel_type metafunction
All, Several months ago I submitted a patch to prevent warnings when performing certain channel conversions [1]. After some discussion with Christian a part of the patch was removed in favor of using the unsigned_integral_max_value metafunction. After tweaking my specializations of that metafunction my code compiled fine. Recently I modified my code to use GIL more extensively. I'm pleased with the reduced code footprint (a 5000 line file was reduced to ~150 lines), but unfortunately there are more warnings due to unsafe conversions with my now-more-extensive testing. Attached is a file which adds a base_channel_type metafunction which correctly determines the type which the channel constructor is expecting. I believe this is more correct than using unsigned_integral_max_value as the value_type of *max_value is not the same as the channel type for, e.g., 8-bit unsigned channels. Something like this metafunction has also been requested recently [2]. The following demonstrates its use for the built-in channel type models I'm aware of: ///////////BEGIN CODE//////////////////// #include <boost/cstdint.hpp> #include <boost/gil/gil_all.hpp> #include <boost/static_assert.hpp> #include <boost/type_traits/is_same.hpp> namespace gil = boost::gil; typedef gil::packed_image3_type<boost::uint16_t, 7,7,2, gil::bgr_layout_t>::type bgr772_image_t; typedef gil::kth_element_type<bgr772_image_t::value_type,0>::type b7_element_type; typedef gil::base_channel_type<b7_element_type>::type b7_type; BOOST_STATIC_ASSERT((boost::is_same<b7_type, boost::uint8_t>::value)); typedef gil::base_channel_type<gil::channel_type<gil::gray32f_pixel_t>::type>::type gray32f_type; BOOST_STATIC_ASSERT((boost::is_same<gray32f_type, float>::value)); typedef gil::base_channel_type<gil::channel_type<gil::rgb8_pixel_t>::type>::type rgb8_type; BOOST_STATIC_ASSERT((boost::is_same<rgb8_type, boost::uint8_t>::value)); int main() {} /////////END CODE///////////////// Adding the metafunction and using it to cast values prior to channel construction has removed the warnings from my testing. The diff file also contains changes to channel_algorithm.hpp to suppress warnings using the supplied metafunction. I'd be happy to write additional test cases, trac tickets, etc. The changes have been tested with g++3.4, g++4.3 and VC8 (VS2005). Thanks, Nate [1] <http://lists.boost.org/boost-users/2010/06/60066.php> [2] <http://lists.boost.org/Archives/boost/2010/07/169159.php>
Hi Nate, good to hear from you! Two things: 1. What version of boost are you using? There have some changes in the current trunk and release branch which should appear in boost 1.45. 2. Could you send me a small test code that exhibits your problem? Regards, Christian On Tue, Nov 2, 2010 at 2:31 AM, Nathan Crookston <nathan.crookston@gmail.com> wrote:
All,
Several months ago I submitted a patch to prevent warnings when performing certain channel conversions [1]. After some discussion with Christian a part of the patch was removed in favor of using the unsigned_integral_max_value metafunction. After tweaking my specializations of that metafunction my code compiled fine.
Recently I modified my code to use GIL more extensively. I'm pleased with the reduced code footprint (a 5000 line file was reduced to ~150 lines), but unfortunately there are more warnings due to unsafe conversions with my now-more-extensive testing.
Attached is a file which adds a base_channel_type metafunction which correctly determines the type which the channel constructor is expecting. I believe this is more correct than using unsigned_integral_max_value as the value_type of *max_value is not the same as the channel type for, e.g., 8-bit unsigned channels. Something like this metafunction has also been requested recently [2].
The following demonstrates its use for the built-in channel type models I'm aware of: ///////////BEGIN CODE//////////////////// #include <boost/cstdint.hpp> #include <boost/gil/gil_all.hpp> #include <boost/static_assert.hpp> #include <boost/type_traits/is_same.hpp>
namespace gil = boost::gil;
typedef gil::packed_image3_type<boost::uint16_t, 7,7,2, gil::bgr_layout_t>::type bgr772_image_t;
typedef gil::kth_element_type<bgr772_image_t::value_type,0>::type b7_element_type;
typedef gil::base_channel_type<b7_element_type>::type b7_type;
BOOST_STATIC_ASSERT((boost::is_same<b7_type, boost::uint8_t>::value));
typedef gil::base_channel_type<gil::channel_type<gil::gray32f_pixel_t>::type>::type gray32f_type;
BOOST_STATIC_ASSERT((boost::is_same<gray32f_type, float>::value)); typedef gil::base_channel_type<gil::channel_type<gil::rgb8_pixel_t>::type>::type rgb8_type; BOOST_STATIC_ASSERT((boost::is_same<rgb8_type, boost::uint8_t>::value));
int main() {} /////////END CODE/////////////////
Adding the metafunction and using it to cast values prior to channel construction has removed the warnings from my testing. The diff file also contains changes to channel_algorithm.hpp to suppress warnings using the supplied metafunction.
I'd be happy to write additional test cases, trac tickets, etc. The changes have been tested with g++3.4, g++4.3 and VC8 (VS2005).
Thanks, Nate
[1] <http://lists.boost.org/boost-users/2010/06/60066.php> [2] <http://lists.boost.org/Archives/boost/2010/07/169159.php>
_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
Hi Christian, On Tue, Nov 2, 2010 at 9:38 AM, Christian Henning <chhenning@gmail.com> wrote:
Hi Nate, good to hear from you! Likewise.
Two things:
1. What version of boost are you using? There have some changes in the current trunk and release branch which should appear in boost 1.45.
I've reproduced the issues with both boost 1.44 and the current trunk using VC8 (VS2005). Interestingly, the issues don't appear with VC10 nor with g++.
2. Could you send me a small test code that exhibits your problem?
I should have done this initially. The following code gives me warnings when compiled with cl.exe /EHsc /W4 <filename> /I<path-to-boost> /////////BEGIN CODE//////////// #include <boost/gil/gil_all.hpp> #include <iostream> namespace boost { namespace gil { ////////////////////////////////////////////////////////////////////////// // Byte-aligned 12 bit grayscale ////////////////////////////////////////////////////////////////////////// const uint16_t bits12_max_val = 0xFFF; struct bits12_min { static uint16_t apply() { return 0; } }; struct bits12_max { static uint16_t apply() { return bits12_max_val; } }; typedef scoped_channel_value<uint16_t, bits12_min, bits12_max> bits12; namespace detail { template <> struct unsigned_integral_max_value<bits12> : public mpl::integral_c<uint32_t, bits12_max_val> {}; template <> struct unsigned_integral_num_bits<bits12> : public mpl::int_<12> {}; }//end detail GIL_DEFINE_BASE_TYPEDEFS(12, gray) } }//end boost::gil namespace gil = boost::gil; int main() { gil::gray12_pixel_t g14(100); gil::rgb8_pixel_t rgb8; color_convert(g14, rgb8); color_convert(rgb8, g14); return 0; } /////////END CODE//////////// The problem appears to that the scoped_channel_value constructor expects either a scoped_channel_value object or an object of type base_channel_t. While the channel conversion code ensures the output will be valid (assuming valid input), it doesn't cast it to the type the channel is expecting. A small warning snippet: c:\cygwin\home\ncrookston\boost_trunk\boost\gil\channel_algorithm.hpp(230) : warning C4244: 'argument' : conversion from 'integer_t' to 'boost::uint16_t', possible loss of data NOTE: The patch I sent earlier has an error -- in copying my changes from 1.44 to the trunk I failed to replace value_type with type in a couple places. The patch attached to this email corrects that. Thanks, Nate
Hi Nathan, sorry for the late reply.
1. What version of boost are you using? There have some changes in the current trunk and release branch which should appear in boost 1.45.
I've reproduced the issues with both boost 1.44 and the current trunk using VC8 (VS2005). Interestingly, the issues don't appear with VC10 nor with g++.
I have tried with VC10, VC9, and VC8. I could only reproduce the problem with VC8 but only when turning on warning level W4. I have done some testing to figure out if the code produced by the various compilers differ. But as far as I can see they produce the same code. What I did was to insert some local variables to check for their data type based on the template parameters. I don't really understand why the newer compilers don't emit warnings. Do you have some ideas?
NOTE: The patch I sent earlier has an error -- in copying my changes from 1.44 to the trunk I failed to replace value_type with type in a couple places. The patch attached to this email corrects that.
I'll apply your patch to the boost trunk and will update the release branch once the boost 1.45 is out. I like the convenience of base_channel_type and I think it makes the code more readable. One change I did do is to remove the typedef for integer_t and the cast to integer_t in channel_converter_unsigned_integral_nondivisible<..>. I don't think that's needed anymore. Thanks Nathan for putting this through. Regards, Christian
Nathan, I found one more error in the patch. We cannot exchange typedef typename detail::unsigned_integral_max_value< SrcChannelV
::value_type src_integer_t; typedef typename detail::unsigned_integral_max_value< DstChannelV ::value_type dst_integer_t;
with typedef typename base_channel_type< SrcChannelV >::type src_integer_t; typedef typename base_channel_type< DstChannelV >::type dst_integer_t; since unsigned_integral_max_value<...> will upgrade the base_channel_t to either uint32_t or uintmax_t. Luckily I caught that error when running gil's unit tests. Regards, Christian
On Sat, Nov 6, 2010 at 11:35 AM, Christian Henning <chhenning@gmail.com> wrote:
I have done some testing to figure out if the code produced by the various compilers differ. But as far as I can see they produce the same code. What I did was to insert some local variables to check for their data type based on the template parameters.
I don't really understand why the newer compilers don't emit warnings. Do you have some ideas? I first ran into this problem several months ago, and my testing indicated that even VC8 was usually okay with narrowing during explicit conversion to a built-in type. The problem seemed the explicit conversion to the non-default type. Perhaps MSVC is more self-consistent in that way now?
NOTE: The patch I sent earlier has an error -- in copying my changes from 1.44 to the trunk I failed to replace value_type with type in a couple places. The patch attached to this email corrects that.
I'll apply your patch to the boost trunk and will update the release branch once the boost 1.45 is out.
Thanks!
I like the convenience of base_channel_type and I think it makes the code more readable. One change I did do is to remove the typedef for integer_t and the cast to integer_t in channel_converter_unsigned_integral_nondivisible<..>. I don't think that's needed anymore.
Yes, I wondered about that when writing the patch. I worried about removing it, but I was confused by its use in the original code and again in my patch. It seemed to me that it could cause integer overflow. The following sample demonstrates the problem in the trunk and (if I correctly understood your above comment) with your proposed change. In it I simply create a 30-bit channel type and convert it to 8-bit. ///BEGIN CODE demonstrating integer_t necessity & problem/// #include <boost/gil/gil_all.hpp> #include <iostream> namespace boost { namespace gil { ////////////////////////////////////////////////////////////////////////// // Byte-aligned 30 bit grayscale ////////////////////////////////////////////////////////////////////////// const uint32_t bits30_max_val = 1073741823; struct bits30_min { static uint32_t apply() { return 0; } }; struct bits30_max { static uint32_t apply() { return bits30_max_val; } }; typedef scoped_channel_value<uint32_t, bits30_min, bits30_max> bits30; namespace detail { template <> struct unsigned_integral_max_value<bits30> : public mpl::integral_c<uint32_t, bits30_max_val> {}; template <> struct unsigned_integral_num_bits<bits30> : public mpl::int_<30> {}; }//end detail GIL_DEFINE_BASE_TYPEDEFS(30, gray) } }//end boost::gil namespace gil = boost::gil; int main() { gil::gray30_pixel_t g30; gil::gray8_pixel_t g8(255); color_convert(g8, g30); std::cout << "Value: " << g30[0] << std::endl; std::cout << "Should be: " << gil::bits30_max_val << std::endl; return 0; } ///END CODE/// Output with trunk: Value: 12632255 Should be: 1073741823 I have an updated patch which fixes the above problem and also prevents a few more warnings. Since I dislike large amounts of code in a single message I'll send another shortly, describing how to reproduce the warning and with another patch which should correct it and everything else discussed thus far. Thanks for looking over my submission, Nate
Christan, Thanks for looking over my patch. On Sun, Nov 7, 2010 at 12:39 AM, Nathan Crookston
I have an updated patch which fixes the above problem and also prevents a few more warnings.
Since I dislike large amounts of code in a single message I'll send another shortly, describing how to reproduce the warning and with another patch which should correct it and everything else discussed thus far.
The following code reveals another place where we GIL can emit warnings (at an admittedly high warning level): ///BEGIN CODE showing channel conversion problems for large integers/// #include <boost/gil/gil_all.hpp> #include <iostream> namespace boost { namespace gil { ////////////////////////////////////////////////////////////////////////// // Byte-aligned 30 bit grayscale ////////////////////////////////////////////////////////////////////////// const uint64_t bits60_max_val = 1152921504606846976; struct bits60_min { static uint64_t apply() { return 0; } }; struct bits60_max { static uint64_t apply() { return bits60_max_val; } }; typedef scoped_channel_value<uint64_t, bits60_min, bits60_max> bits60; namespace detail { template <> struct unsigned_integral_max_value<bits60> : public mpl::integral_c<uint64_t, bits60_max_val> {}; template <> struct unsigned_integral_num_bits<bits60> : public mpl::int_<60> {}; }//end detail GIL_DEFINE_BASE_TYPEDEFS(60, gray) } }//end boost::gil namespace gil = boost::gil; int main() { gil::gray60_pixel_t g60; gil::gray8_pixel_t g8(255); color_convert(g8, g60); std::cout << "Value: " << g60[0] << std::endl; std::cout << "Should be: " << gil::bits60_max_val << std::endl; return 0; } ///END CODE/// The fix is the same as it's been for the other warnings. The attached patch corrects this, the original conversion problems and an actual bug as noted in my previous message. It also fixes the issue with my original patch (use type, not value_type -- thanks for catching that, I incorrectly merged my changes from 1.44 to trunk before generating the patch). Please let me know if there are any problems, thanks again! Nate
Hi Nathan, thanks for finding this.
The fix is the same as it's been for the other warnings.
The attached patch corrects this, the original conversion problems and an actual bug as noted in my previous message. It also fixes the issue with my original patch (use type, not value_type -- thanks for catching that, I incorrectly merged my changes from 1.44 to trunk before generating the patch).
Please let me know if there are any problems, thanks again!
No problems here. I'll apply your patch once you have looked at my last patch I send you a couple of minutes before. Christian
Hi Christian, On Sun, Nov 7, 2010 at 3:05 PM, Christian Henning <chhenning@gmail.com> wrote:
No problems here. I'll apply your patch once you have looked at my last patch I send you a couple of minutes before.
It looks good to me -- I like the enhanced readability. Thanks again, Nate
Hi Nathan,
Output with trunk: Value: 12632255 Should be: 1073741823
There was already a bug in the code before I patched it. Basically the static_cast to integer_t should have been applied to src only. Please review my patch. The function is more verbose now but I thing it's more clear. Index: channel_algorithm.hpp =================================================================== --- channel_algorithm.hpp (revision 66416) +++ channel_algorithm.hpp (working copy) @@ -226,8 +226,20 @@ template <typename SrcChannelV, typename DstChannelV> struct channel_converter_unsigned_integral_nondivisible<SrcChannelV,DstChannelV,true,false> { DstChannelV operator()(SrcChannelV src) const { - typedef typename base_channel_type<DstChannelV>::type dest_t; - return DstChannelV(static_cast<dest_t>( src * unsigned_integral_max_value<DstChannelV>::value) / unsigned_integral_max_value<SrcChannelV>::value); + + // make sure the operation below doesn't overflow dst's integeral data type. + // e.g. dst is 30bit and src is 8 bit can result in 38bit value + typedef typename detail::min_fast_uint< unsigned_integral_num_bits< SrcChannelV >::value + + unsigned_integral_num_bits< DstChannelV >::value + >::type integer_t; + + typedef typename base_channel_type< DstChannelV >::type dest_t; + + integer_t src_ = static_cast< integer_t >( src ); + integer_t dst_max = unsigned_integral_max_value<DstChannelV>::value; + integer_t src_max = unsigned_integral_max_value<SrcChannelV>::value; + + return static_cast< dest_t >( src_ * dst_max / src_max ); } }; Regards, Christian
participants (2)
-
Christian Henning -
Nathan Crookston