Adding the min(dim) call is unarguably a good idea. We need to sit down and
make sure this stuff is consistent across all the classes we have that
represent blocks of data.

I do not like xmin/ymin but top/bottom/left/right are fine with me. (Though
mostly because tlbr is nice and comfy due to my history as opposed to a
fundamental argument of correctness :-))


On Mon, Dec 16, 2013 at 10:14 AM, Andrew Adams <notifications@github.com>wrote:

> In src/Buffer.h:
>
> > @@ -180,6 +180,15 @@ class Buffer {
> >          return contents.ptr->buf.extent[dim];
> >      }
> >
> > +    /** Set the extent of this buffer. */
> > +    void set_extent(int e0, int e1 = 0, int e2 = 0, int e3 = 0) {
> > +        assert(defined());
> > +        contents.ptr->buf.extent[0] = e0;
> > +        contents.ptr->buf.extent[1] = e1;
> > +        contents.ptr->buf.extent[2] = e2;
> > +        contents.ptr->buf.extent[3] = e3;
> > +    }
>
> Being able to query the min valid coordinate of the image actually passed
> in is a missing useful feature though, and is as correct as querying the
> extent when not processing in tiles. I'm merging that part of the pull
> request. Instead of "xmin" and "ymin" I'm going with "left" and "top", to
> avoid confusion when people use variables other than x and y to index their
> images. I'm also adding "right" and "bottom" to represent the maximum legal
> coordinate to load from. - Andrew
> … <#142fc9e1e13df119_>
> On Mon, Dec 16, 2013 at 10:09 AM, Zalman Stern <notifications@github.com>wrote:
> In src/Buffer.h: > @@ -180,6 +180,15 @@ class Buffer { > return
> contents.ptr->buf.extent[dim]; > } > > + /** Set the extent of this buffer.
> */ > + void set_extent(int e0, int e1 = 0, int e2 = 0, int e3 = 0) { > +
> assert(defined()); > + contents.ptr->buf.extent[0] = e0; > +
> contents.ptr->buf.extent[1] = e1; > + contents.ptr->buf.extent[2] = e2; > +
> contents.ptr->buf.extent[3] = e3; > + } This is a longer discussion, but
> the issue is that Halide::clamp(x, 0, in.width()-1) establishes an edge
> replication boundary condition on x. (Being very clear for others who may
> be reading that in.width() means in.extent(0) and we are assuming we're
> working with dimension 0 of in here.) When processing tiles, one does not
> want edge replication at the tile boundaries but rather one wishes to
> require the input tiles have enough padding to satisfy the requested output
> area. (And if that area extends off the edge of the image, then apply the
> boundary condition at the image edge.) It should be an error in most cases
> if tiled processing reads outside of the input tiles at coordinates that
> are not at the edge of the of the entire image. Now we can say that images,
> by convention, always have a 0 origin and that makes the 0 boundary
> generally the correct place to apply the boundary condition. However the
> extent (width if talking about dimension 0) is only correct for applying
> the boundary condition if we pass in the entire image. I started writing up
> a lot of stuff on this a long time ago and Andrew and Jonathan and I have
> talked about it more than once. With Halide as it stands now, the best
> solution is likely to pass the total image area (extent and possibly
> origin) as extra parameters to the filter. One could possibly also make it
> an implicit global using user_context or some such, though I'm not sure
> that wins anything other than allowing filter signatures to stay the same
> if adding this functionality. It is however clear that we should consider
> extending Halide to handle this cleanly. — Reply to this email directly or
> view it on GitHub<https://github.com/halide/Halide/pull/215/files#r8375279>
> .
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/halide/Halide/pull/215/files#r8375487>
> .
>