diff --git a/include/caffe/layers/crop_layer.hpp b/include/caffe/layers/crop_layer.hpp index 5c605b2ae9e..c4fda1220c3 100644 --- a/include/caffe/layers/crop_layer.hpp +++ b/include/caffe/layers/crop_layer.hpp @@ -44,6 +44,7 @@ class CropLayer : public Layer { vector offsets; private: + // Recursive copy function. void crop_copy(const vector*>& bottom, const vector*>& top, const vector& offsets, @@ -53,6 +54,14 @@ class CropLayer : public Layer { Dtype* dest_data, bool is_forward); + // Recursive copy function: this is similar to crop_copy() but loops over all + // but the last two dimensions to allow for ND cropping while still relying on + // a CUDA kernel for the innermost two dimensions for performance reasons. An + // alterantive implementation could rely on the kernel more by passing + // offsets, but this is problematic because of its variable length. + // Since in the standard (N,C,W,H) case N,C are usually not cropped a speedup + // could be achieved by not looping the application of the copy_kernel around + // these dimensions. void crop_copy_gpu(const vector*>& bottom, const vector*>& top, const vector& offsets, diff --git a/src/caffe/layers/crop_layer.cpp b/src/caffe/layers/crop_layer.cpp index e81bdd732f3..aecdcd63194 100644 --- a/src/caffe/layers/crop_layer.cpp +++ b/src/caffe/layers/crop_layer.cpp @@ -15,8 +15,7 @@ namespace caffe { template void CropLayer::LayerSetUp(const vector*>& bottom, const vector*>& top) { - // All logic that depends only on the number of dimensions is here, - // the rest is in Reshape because it depends on Blob size. + // LayerSetup() handles the number of dimensions; Reshape() handles the sizes. // bottom[0] supplies the data // bottom[1] supplies the size const CropParameter& param = this->layer_param_.crop_param(); @@ -40,41 +39,35 @@ void CropLayer::Reshape(const vector*>& bottom, int input_dim = bottom[0]->num_axes(); const int start_axis = bottom[0]->CanonicalAxisIndex(param.axis()); - // initialize all offsets to 0 + // Initialize offsets to 0 and the new shape to the current shape of the data. offsets = vector(input_dim, 0); - // initialize new shape to bottom[0] vector new_shape(bottom[0]->shape()); - // apply crops + // Determine crop offsets and the new shape post-crop. for (int i = 0; i < input_dim; ++i) { int crop_offset = 0; - int new_size = bottom[0]->shape(i); + int new_size = bottom[0]->shape(i); if (i >= start_axis) { new_size = bottom[1]->shape(i); - if (param.offset_size() == 1) { - // if only one crop value is supplied, crop all dimensions after axis - // by this crop value + // If only one offset is given, all crops have the same offset. crop_offset = param.offset(0); } else if (param.offset_size() > 1) { - // crop values specified must be equal to the number of dimensions - // following axis + // For several offsets, the number of offsets must be equal to the + // number of dimensions to crop, that is dimensions after the axis. crop_offset = param.offset(i - start_axis); } + // Check that the crop and offset are within the dimension's bounds. + CHECK_GE(bottom[0]->shape(i) - crop_offset, bottom[1]->shape(i)) + << "the crop for dimension " << i << " is out-of-bounds with " + << "size " << bottom[1]->shape(i) << " and offset " << crop_offset; } - // Check that the image we are cropping minus the margin is bigger - // than the destination image. - CHECK_GE(bottom[0]->shape(i) - crop_offset, - bottom[1]->shape(i)) - << "invalid crop parameters in dimension: " << i; - // Now set new size and offsets new_shape[i] = new_size; offsets[i] = crop_offset; } top[0]->Reshape(new_shape); } -// recursive copy function template void CropLayer::crop_copy(const vector*>& bottom, const vector*>& top, diff --git a/src/caffe/layers/crop_layer.cu b/src/caffe/layers/crop_layer.cu index 9ed8f7cce57..f78cecbbeee 100644 --- a/src/caffe/layers/crop_layer.cu +++ b/src/caffe/layers/crop_layer.cu @@ -22,15 +22,6 @@ __global__ void copy_kernel(const int n, const int height, const int width, } } -// recursive copy function, this function is similar to crop_copy but loops -// over all but the last two dimensions. It is implemented this way to allow -// for ND cropping while still relying on a CUDA kernel for the innermost -// two dimensions for performance reasons. -// An alternative way to implement ND cropping relying more on the kernel -// would require passing offsets to the kernel, which is a bit problematic -// because it is of variable length. Since in the standard (N,C,W,H) case -// N,C are usually not cropped a speedup could be achieved by not looping -// the application of the copy_kernel around these dimensions. template void CropLayer::crop_copy_gpu(const vector*>& bottom, const vector*>& top, diff --git a/src/caffe/test/test_crop_layer.cpp b/src/caffe/test/test_crop_layer.cpp index 45f24e2ee8d..ce2c736f644 100644 --- a/src/caffe/test/test_crop_layer.cpp +++ b/src/caffe/test/test_crop_layer.cpp @@ -91,6 +91,24 @@ TYPED_TEST(CropLayerTest, TestSetupShapeNegativeIndexing) { } } +TYPED_TEST(CropLayerTest, TestDimensionsCheck) { + typedef typename TypeParam::Dtype Dtype; + LayerParameter layer_param; + // Reshape size blob to have incompatible sizes for uncropped dimensions: + // the size blob has more channels than the data blob, but this is fine + // since the channels dimension is not cropped in this configuration. + this->blob_bottom_1_->Reshape(2, 5, 4, 2); + CropLayer layer(layer_param); + layer.SetUp(this->blob_bottom_vec_, this->blob_top_vec_); + for (int i = 0; i < this->blob_top_->num_axes(); ++i) { + if (i < 2) { + EXPECT_EQ(this->blob_bottom_0_->shape(i), this->blob_top_->shape(i)); + } else { + EXPECT_EQ(this->blob_bottom_1_->shape(i), this->blob_top_->shape(i)); + } + } +} + TYPED_TEST(CropLayerTest, TestCropAll) { typedef typename TypeParam::Dtype Dtype; LayerParameter layer_param;