Skip to content

Commit

Permalink
Mrf: Fixes for Create() (OSGeo#2923)
Browse files Browse the repository at this point in the history 
* Finish create call when closing dataset

The Create call does write the files on disk if no data is every written before closing the newly created dataset.

* Fix the Create logic

Test main file creation early in Create(), report errors.
Invoke Crystalize in case Create gets used and no data is written.
Report errors from Crystalize instead of throwing.
  • Loading branch information
@lucianpls
lucianpls authored Sep 5, 2020
1 parent f33eb67 commit c493027
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 21 deletions.
4 changes: 3 additions & 1 deletion gdal/frmts/mrf/marfa.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,6 @@ class MRFDataset final: public GDALPamDataset {

static GDALDataset *Open(GDALOpenInfo *);
static int Identify(GDALOpenInfo *);
void Crystalize();

static GDALDataset *CreateCopy(const char *pszFilename, GDALDataset *poSrcDS,
int bStrict, char **papszOptions, GDALProgressFunc pfnProgress,
Expand Down Expand Up @@ -380,6 +379,9 @@ class MRFDataset final: public GDALPamDataset {
}

protected:
// False if it failed
int Crystalize();

CPLErr LevelInit(const int l);

// Reads the XML metadata and returns the XML
Expand Down
69 changes: 50 additions & 19 deletions gdal/frmts/mrf/marfa_dataset.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,13 @@ int MRFDataset::CloseDependentDatasets()
MRFDataset::~MRFDataset()

{ // Make sure everything gets written
MRFDataset::FlushCache();
if (eAccess != GA_ReadOnly && !bCrystalized)
if (!Crystalize()) {
// Can't return error code from a destructor, just emit the error
CPLError(CE_Failure, CPLE_FileIO, "Error creating files");
}

MRFDataset::FlushCache();
MRFDataset::CloseDependentDatasets();

if (ifp.FP)
Expand Down Expand Up @@ -207,6 +212,12 @@ CPLErr MRFDataset::IRasterIO(GDALRWFlag eRWFlag, int nXOff, int nYOff, int nXSiz
static_cast<int>(nPixelSpace), static_cast<int>(nLineSpace),
static_cast<int>(nBandSpace));

if (eRWFlag == GF_Write && !bCrystalized)
if (!Crystalize()) {
CPLError(CE_Failure, CPLE_FileIO, "MRF: Error creating files");
return CE_Failure;
}

//
// Call the parent implementation, which splits it into bands and calls their IRasterIO
//
Expand Down Expand Up @@ -786,7 +797,7 @@ static CPLErr Init_Raster(ILImage &image, MRFDataset *ds, CPLXMLNode *defimage)
image.pagesize.y < 1 ||
image.pagesize.c <= 0 )
{
CPLError(CE_Failure, CPLE_AppDefined, "Invalid PageSize");
CPLError(CE_Failure, CPLE_IllegalArg, "Invalid PageSize");
return CE_Failure;
}
}
Expand All @@ -802,7 +813,7 @@ static CPLErr Init_Raster(ILImage &image, MRFDataset *ds, CPLXMLNode *defimage)
image.comp = CompToken(CPLGetXMLValue(defimage, "Compression", "PNG"));

if (image.comp == IL_ERR_COMP) {
CPLError(CE_Failure, CPLE_AppDefined,
CPLError(CE_Failure, CPLE_IllegalArg,
"GDAL MRF: Compression %s is unknown",
CPLGetXMLValue(defimage, "Compression", nullptr));
return CE_Failure;
Expand Down Expand Up @@ -843,7 +854,7 @@ static CPLErr Init_Raster(ILImage &image, MRFDataset *ds, CPLXMLNode *defimage)
ce_start = GetXMLColorEntry(p);
int start_idx = static_cast<int>(getXMLNum(p, "idx", 0));
if (start_idx < 0) {
CPLError(CE_Failure, CPLE_AppDefined,
CPLError(CE_Failure, CPLE_IllegalArg,
"GDAL MRF: Palette index %d not allowed", start_idx);
delete poColorTable;
return CE_Failure;
Expand All @@ -854,7 +865,7 @@ static CPLErr Init_Raster(ILImage &image, MRFDataset *ds, CPLXMLNode *defimage)
ce_end = GetXMLColorEntry(p);
int end_idx = static_cast<int>(getXMLNum(p, "idx", start_idx + 1));
if ((end_idx <= start_idx) || (start_idx >= entries)) {
CPLError(CE_Failure, CPLE_AppDefined,
CPLError(CE_Failure, CPLE_IllegalArg,
"GDAL MRF: Index Error at index %d", end_idx);
delete poColorTable;
return CE_Failure;
Expand All @@ -868,7 +879,7 @@ static CPLErr Init_Raster(ILImage &image, MRFDataset *ds, CPLXMLNode *defimage)
ds->SetColorTable(poColorTable);
}
else {
CPLError(CE_Failure, CPLE_AppDefined, "GDAL MRF: Palette definition error");
CPLError(CE_Failure, CPLE_IllegalArg, "GDAL MRF: Palette definition error");
return CE_Failure;
}
}
Expand Down Expand Up @@ -1039,7 +1050,7 @@ VSILFILE *MRFDataset::IdxFP() {

if (nullptr != ifp.FP) {
if (!bCrystalized && !CheckFileSize(current.idxfname, expected_size, GA_Update)) {
CPLError(CE_Failure, CPLE_AppDefined, "Can't extend the cache index file %s",
CPLError(CE_Failure, CPLE_FileIO, "MRF: Can't extend the cache index file %s",
current.idxfname.c_str());
return nullptr;
}
Expand Down Expand Up @@ -1617,7 +1628,7 @@ GDALDataset *MRFDataset::CreateCopy(const char *pszFilename,
Create(pszFilename, x, y, nBands, dt, options));

if (poDS == nullptr || poDS->bCrystalized)
throw CPLString().Printf("Can't create %s",pszFilename);
throw CPLString().Printf("MRF: Can't create %s",pszFilename);

img = poDS->current; // Deal with the current one here

Expand Down Expand Up @@ -1663,7 +1674,8 @@ GDALDataset *MRFDataset::CreateCopy(const char *pszFilename,
poDS->SetColorTable(poSrcBand1->GetColorTable()->Clone());

// Finally write the XML in the right file name
poDS->Crystalize();
if (!poDS->Crystalize())
throw CPLString("MRF: Error creating files");
}
catch (const CPLString& e) {
if (poDS)
Expand Down Expand Up @@ -1978,8 +1990,7 @@ GDALDataset *
MRFDataset::Create(const char * pszName,
int nXSize, int nYSize, int nBands,
GDALDataType eType, char ** papszOptions)

{ // Pending create
{
if( nBands == 0 )
{
CPLError(CE_Failure, CPLE_NotSupported, "nBands == 0 not supported");
Expand All @@ -2005,6 +2016,21 @@ MRFDataset::Create(const char * pszName,
poDS->fname.resize(pos); // Cut the ornamentations
}

// Try creating the mrf file early, to avoid failing on Crystalize later
if (!STARTS_WITH(poDS->fname.c_str(), "<MRF_META>")) {
// Try opening it first, even though we still clobber it later
VSILFILE* mainfile = VSIFOpenL(poDS->fname.c_str(), "r+b");
if (!mainfile) { // Then try creating it
mainfile = VSIFOpenL(poDS->fname.c_str(), "w+b");
if (!mainfile) {
CPLError(CE_Failure, CPLE_OpenFailed, "MRF: Can't open %s for writing", poDS->fname.c_str());
delete poDS;
return nullptr;
}
}
VSIFCloseL(mainfile);
}

// Use the full, set some initial parameters
ILImage &img = poDS->full;
img.size = ILSize(nXSize, nYSize, 1, nBands);
Expand Down Expand Up @@ -2071,24 +2097,29 @@ MRFDataset::Create(const char * pszName,
return poDS;
}

void MRFDataset::Crystalize()

int MRFDataset::Crystalize()
{
if (bCrystalized || eAccess != GA_Update)
return;
if (bCrystalized || eAccess != GA_Update) {
bCrystalized = TRUE;
return TRUE;
}

// No need to write to disk if there is no filename. This is a
// memory only dataset.
if (strlen(GetDescription()) == 0
|| EQUALN(GetDescription(), "<MRF_META>", 10))
return;
|| EQUALN(GetDescription(), "<MRF_META>", 10)) {
bCrystalized = TRUE;
return TRUE;
}

CPLXMLNode *config = BuildConfig();
WriteConfig(config);
if (!WriteConfig(config))
return FALSE;
CPLDestroyXMLNode(config);
if (!nocopy && (!IdxFP() || !DataFP()))
throw CPLString().Printf("MRF: %s", strerror(errno));
return FALSE;
bCrystalized = TRUE;
return TRUE;
}

// Copy the first index at the end of the file and bump the version count
Expand Down
5 changes: 4 additions & 1 deletion gdal/frmts/mrf/mrf_band.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,7 +867,10 @@ CPLErr MRFRasterBand::IWriteBlock(int xblk, int yblk, void *buffer)

// Finish the Create call
if (!poDS->bCrystalized)
poDS->Crystalize();
if (!poDS->Crystalize()) {
CPLError(CE_Failure, CPLE_AppDefined, "MRF: Error creating files");
return CE_Failure;
}

if (1 == cstride) { // Separate bands, we can write it as is
// Empty page skip
Expand Down

0 comments on commit c493027

Please sign in to comment.