Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Convenience overload to make it easy to read entire image #157

Merged
merged 9 commits into from
Mar 2, 2017
Merged

Convenience overload to make it easy to read entire image #157

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
61536d5
Convenience overload to make it easy to read entire image
JonathanMeans Feb 20, 2017
f0cfc29
Add Doxygen
JonathanMeans Feb 20, 2017
3a3bbc8
Added some more Doxygen comments. I think the new interleaved() over…
asylvest Feb 20, 2017
40f0db8
Actually, the new interleaved() overloading can just go in the base c…
asylvest Feb 20, 2017
c9d646a
Add safer interleaved overload
JonathanMeans Feb 21, 2017
9e6951e
Fix inheritance
JonathanMeans Feb 22, 2017
0100db0
Clean up test_geotiff
JonathanMeans Mar 1, 2017
a421ce7
Update ReadControl comments
JonathanMeans Mar 1, 2017
8556f0f
Fix problem in test
JonathanMeans Mar 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions six/modules/c++/six.sidd/include/six/sidd/GeoTIFFReadControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class GeoTIFFReadControl : public ReadControl
virtual void load(const std::string& fromFile,
const std::vector<std::string>& schemaPaths);

using ReadControl::interleaved;
virtual UByte* interleaved(Region& region, size_t imageNumber);

virtual std::string getFileType() const
Expand Down
15 changes: 3 additions & 12 deletions six/modules/c++/six.sidd/tests/test_byte_swap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,22 +118,13 @@ void write(const sys::Int16_T* data, bool useStream, bool byteSwap)
}
}

void read(const std::string& filename, sys::Int16_T* data)
void read(const std::string& filename, mem::ScopedArray<sys::Int16_T>& data)
{
six::NITFReadControl reader;
reader.load(filename);
mem::SharedPtr<const six::Container> container = reader.getContainer();
const six::Data* const inData = container->getData(0);

const size_t numRows = inData->getNumRows();
const size_t numCols = inData->getNumCols();
six::Region region;
region.setStartRow(0);
region.setStartCol(0);
region.setNumRows(numRows);
region.setNumCols(numCols);
region.setBuffer(reinterpret_cast<six::UByte*>(data));
reader.interleaved(region, 0);
reader.interleaved(region, 0, data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a little hung up on that we're not re-using the memory now... to me, just adding one more line (the region.setBuffer() line from before) and passing in a raw pointer here rather than the ScopedArray really is pretty legible code, but it is the case that we're talking a trivial amount of memory and this is a test program, so if you think this does improve readability, I'm good with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'd lost track of this one! I've pushed up the fixes.

}

bool run(bool useStream = false, bool byteswap = false)
Expand All @@ -151,7 +142,7 @@ bool run(bool useStream = false, bool byteswap = false)
BYTES_PER_PIXEL, DATA_LENGTH);
}
write(testData.get(), useStream, byteswap);
read(OUTPUT_NAME, testData.get());
read(OUTPUT_NAME, testData);

if (memcmp(testData.get(), imageData.get(), DATA_SIZE_IN_BYTES) == 0)
{
Expand Down
22 changes: 7 additions & 15 deletions six/modules/c++/six.sidd/tests/test_geotiff.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,33 +102,25 @@ void write(const sys::Int16_T* data)
writer.save(reinterpret_cast<const six::UByte*>(data), OUTPUT_NAME);
}

void read(const std::string& filename, sys::Int16_T* data)
void read(const std::string& filename, mem::ScopedArray<sys::Int16_T>& data)
{
six::sidd::GeoTIFFReadControl reader;
six::NITFReadControl reader;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test program really get run? This can't be right... it should still be a GeoTIFFReadControl right??

reader.load(filename);
mem::SharedPtr<const six::Container> container = reader.getContainer();
const six::Data* const inData = container->getData(0);

const size_t numRows = inData->getNumRows();
const size_t numCols = inData->getNumCols();
six::Region region;
region.setStartRow(0);
region.setStartCol(0);
region.setNumRows(numRows);
region.setNumCols(numCols);
region.setBuffer(reinterpret_cast<six::UByte*>(data));
reader.interleaved(region, 0);
reader.interleaved(region, 0, data);
}

bool run()
{
mem::ScopedArray<sys::Int16_T> imageData(new sys::Int16_T[DATA_LENGTH]);
generateData(imageData.get());

mem::ScopedArray<sys::Int16_T> testData(new sys::Int16_T[DATA_LENGTH]);
//mem::ScopedArray<sys::Int16_T> testData(new sys::Int16_T[DATA_LENGTH]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need this line anymore


write(imageData.get());
read(OUTPUT_NAME, testData.get());

mem::ScopedArray<sys::Int16_T> testData;
read(OUTPUT_NAME, testData);

if (memcmp(testData.get(), imageData.get(), DATA_SIZE_IN_BYTES) == 0)
{
Expand Down
18 changes: 18 additions & 0 deletions six/modules/c++/six/include/six/NITFReadControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#define __SIX_NITF_READ_CONTROL_H__

#include <map>

#include "six/NITFImageInfo.h"
#include "six/ReadControl.h"
#include "six/ReadControlFactory.h"
Expand Down Expand Up @@ -134,6 +135,23 @@ class NITFReadControl : public ReadControl
void load(mem::SharedPtr<nitf::IOInterface> ioInterface,
const std::vector<std::string>& schemaPaths);


using ReadControl::interleaved;
/*!
* Read section of image data specified by region
*
* \param region Rows and columns of the image to read. If the number
* of rows and/or number of columns is set to -1, this indicates to read
* the entirety of the image in that dimension. In this case, this
* parameter will be updated with the actual number of rows and/or
* columns that were read.
* \param imageNumber Index of the image to read
*
* \return Buffer of image data. This is simply a pointer to the buffer
* that is held by 'region'. If it is NULL in the incoming region, the
* memory is allocated and the region's buffer is updated. In this case
* it is up to the caller to delete the memory.
*/
virtual UByte* interleaved(Region& region, size_t imageNumber);

virtual std::string getFileType() const
Expand Down
32 changes: 32 additions & 0 deletions six/modules/c++/six/include/six/ReadControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "six/Container.h"
#include "six/Options.h"
#include "six/XMLControlFactory.h"
#include <mem/ScopedArray.h>
#include <import/logging.h>

namespace six
Expand Down Expand Up @@ -108,11 +109,42 @@ class ReadControl
* This function reads in the image area specified by the region.
* If you want us to use a work buffer, you can set it through region.
*
* To simply read the entire image, pass a default-constructed
* Region as the first parameter
*
* Once read, the image buffer is set in both the region pointer,
* and in the return value, for convenience
*
* For safety, prefer the overload below.
*/
virtual UByte* interleaved(Region& region, size_t imageNumber) = 0;

/*!
* This function reads in the image area specified by the region.
* If you want us to use a work buffer, you can set it through region.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since buffer takes ownership of this, spell out in the comments here what happens if you do in fact set that work buffer through region (i.e. you don't own the memory after this call - the ScopedArray does)

*
* To simply read the entire image, pass a default-constructed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add a comment somewhere in here that there's no casting that happens... for example, if it's an integer SICD on disk and you pass in a ScopedArray<std::complex<float>>, we're not going to cast the data to complex float for you; it'll just do the wrong thing.

* Region as the first parameter.
*
* Once read, the image buffer is set in the passed buffer, and
* also the region pointer and the return value for convenience.
*
* \param region Region stores row and cols to be read
* \param imageNumber Index of the image to read
* \param buffer Scoped array that holds the memory for the read-in image.
* This will be allocated by this function.
*
* \return Buffer of image data. This is simply equal to buffer.get() and
* is provided as a convenience.
*/
template<typename T>
T* interleaved(Region& region, size_t imageNumber,
mem::ScopedArray<T>& buffer)
{
buffer.reset(reinterpret_cast<T*>(interleaved(region, imageNumber)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... if they pass in a region with a non-NULL buffer, they will probably think they still own the memory and double delete it. I wish the region held a smart pointer but it doesn't right now. Any thoughs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this templated - might as well make the return type T* for convenience too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not overfond of the Region holding any sort of pointer at all, as it makes it tricky to use the same Region for multiple reads. If we didn't have to worry about breaking anything, I'd suggest removing it altogether.

I'm trying to think through passing a Region that already has a buffer. I would hope that the smart pointer parameter and the documentation would make it clear that that's what owns the memory at the end. Presumably the caller would already have it stored as a ScopedArray and not just a smart pointer, so that's what they'd end up passing as the third argument, and nothing would change.

Or if we change Region to hold a smart pointer, we don't even need the new overload, and there's one less pointer to the same data getting passed around.

return buffer.get();
}

/*!
* Get the file type. For SICD, this will only include "NITF", but
* for SIDD, there will be subclassing for "NITF" and "GeoTIFF"
Expand Down