Re: [boost] OpenCL discrepancies (RE: [compute] review)

Subject: Re: [boost] OpenCL discrepancies (RE: [compute] review)
From: Kyle Lutz (kyle.r.lutz_at_[hidden])
Date: 2014-12-28 20:45:08

On Sun, Dec 28, 2014 at 5:36 PM, Gruenke,Matt <mgruenke_at_[hidden]> wrote:
> -----Original Message-----
> From: Boost [mailto:boost-bounces_at_[hidden]] On Behalf Of Kyle Lutz
> Sent: Sunday, December 28, 2014 14:42
> To: boost_at_[hidden] List
> Subject: Re: [boost] [compute] review
>
>> On Sun, Dec 28, 2014 at 1:54 AM, Gruenke,Matt wrote:
>
>>> Finally, based on the docs, it seems there's a bug in
>>> boost::compute::command_queue::enqueue_copy_image_to_buffer() and
>>> boost::compute::command_queue::enqueue_copy_buffer_to_image().
>>> The region and origin parameters should be arrays, in the 2D and 3D cases.
>>> If this discrepancy is intentional, it should be noted.  I haven't
>>> exhaustively reviewed the reference docs, so there might be other
>>> issues of this sort.
>>
>> Can you be more specific as to what you consider to be a "bug" here?
>> These APIs in the command_queue class are very "thin" wrappers on top of
>> the clEnqueue* functions from the OpenCL C API. If you could provide a
>> small test-case which demonstrates the issue and submit it to the
>> bug-tracker [1] that would be great.
>
> I can submit bugs, but this is really simple (and blatant).  Now let's look at boost::compute::command_queue::enqueue_read_image().  It has the following overloads:
>
> void enqueue_read_image(const image2d & image, const size_t origin,
>                         const size_t region, size_t row_pitch,
>                         void * host_ptr,
>                         const wait_list & events = wait_list());
>
> void enqueue_read_image(const image3d & image, const size_t origin,
>                         const size_t region, size_t row_pitch,
>                         size_t slice_pitch, void * host_ptr,
>                         const wait_list & events = wait_list());
>
> See how origin and region are size_t?  Well, here's the OpenCL function they claim to wrap:
>
> cl_int clEnqueueReadImage(cl_command_queue  command_queue,
>                           cl_mem image, cl_bool blocking_read,
>                           const size_t *origin, const size_t *region,
>                           size_t row_pitch, size_t slice_pitch,
>                           void *ptr, cl_uint num_events_in_wait_list,
>                           const cl_event *event_wait_list, cl_event *event );
>
>
> See how origin and region are const size_t *?  The reason is:
>
> origin - Defines the (x, y, z) offset in pixels in the 1D, 2D, or 3D image, the (x, y) offset and the image index in the image array or the (x) offset and the image index in the 1D image array. If image is a 2D image object, origin[2] must be 0. If image is a 1D image or 1D image buffer object, origin[1] and origin[2] must be 0. If image is a 1D image array object, origin[2] must be 0. If image is a 1D image array object, origin[1] describes the image index in the 1D image array. If image is a 2D image array object, origin[2] describes the image index in the 2D image array.

> region - Defines the (width, height, depth) in pixels of the 1D, 2D or 3D rectangle, the (width, height) in pixels of the 2D rectangle and the number of images of a 2D image array or the (width) in pixels of the 1D rectangle and the number of images of a 1D image array. If image is a 2D image object, region[2] must be 1. If image is a 1D image or 1D image buffer object, region[1] and region[2] must be 1. If image is a 1D image array object, region[2] must be 1.
>
>
> I have seen several such examples, in boost::compute::command_queue, and I didn't even look very hard.  But it would be good to sort out all of these problems before Boost.Compute is released as part of boost, since fixing some may involve incompatible API changes.  I guess you could add overloads, in these cases, but it'd be nice if Boost.Compute didn't already have cruft right at the start.

Thanks for bringing this to my attention. This looks to be a bug in
generating the Boost documentation from the doxygen comments.

For example, the enqueue_read_image() method takes the origin argument
as "const size_t origin[2]" but the generated documentation lists this
as merely "const size_t origin". I'd invite you, for now until the
documentation bug is fixed, to look directly at the header file [1].

If anyone more knowledgable with the doxygen->quickbook toolchain
could comment on this or knows of a work-around that would be a great
help.

-kyle

[1] https://github.com/kylelutz/compute/blob/master/include/boost/compute/command_queue.hpp#L678