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.

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

QgsLayoutExporter: avoid to print the "ERROR 6: The PNG driver does not support update access to existing datasets." GDAL error message #60208

Conversation

agiudiceandrea
Copy link
Contributor

@agiudiceandrea agiudiceandrea commented Jan 22, 2025

Description

When QgsLayoutExporter.exportToImage is executed, the "ERROR 6: The PNG driver does not support update access to existing datasets." error message may be thrown by the GDAL/OGR library, and displayed in the console window on some systems, because QgsLayoutExporter.exportToImage tries to open the already exported image file in "update mode" in order to add the georeferencing information inside it, while it is not possible for many image formats (e.g. PNG).

The error doesn't actually affects the exporting functionality.

Fixes #51947.

When QgsLayoutExporter.exportToImage is executed, the "ERROR 6: The PNG driver does not support update access to existing datasets." error message may be thrown by the GDAL/OGR library because QgsLayoutExporter.exportToImage tries to open in "update mode" the already exported image file in order to add the georeferencing information inside it, while it is not possible for many image formats (e.g. PNG).
The error doesn't actually affects the exporting functionality.
@github-actions github-actions bot added this to the 3.42.0 milestone Jan 22, 2025
@agiudiceandrea agiudiceandrea changed the title QgsLayoutExporter: avoid to print the ""ERROR 6: The PNG driver does not support update access to existing datasets." GDAL error message QgsLayoutExporter: avoid to print the "ERROR 6: The PNG driver does not support update access to existing datasets." GDAL error message Jan 22, 2025
Copy link

github-actions bot commented Jan 22, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 74b0f7b)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 74b0f7b)

Copy link
Contributor

@troopa81 troopa81 left a comment

Choose a reason for hiding this comment

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

Wouldn't be better to avoid adding georeferencing information to format that don't support it instead of silencing potential useful errors ?

@rouault
Copy link
Contributor

rouault commented Jan 22, 2025

Wouldn't be better to avoid adding georeferencing information to format that don't support it instead of silencing potential useful errors ?

indeed. The thing is that it is not so easy for QGIS to know which formats support update. GDAL advertizes for Create or CreateCopy support, but not for update (I'm considering working on that. not as immediately trivial as I would hope)

Looking closer at the code, it is weird that this georeferenceOutputPrivate() method returns true when GDALOpen() fails, or doesn't test the output of GDALSetGeoTransform() or GDALSetProjection() for the return code to return. That said, I don't know the downstream impacts of returning false.

But if we want to be clean, georeferenceOutputPrivate() should return an error on PNG.

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented Jan 22, 2025

Wouldn't be better to avoid adding georeferencing information to format that don't support it instead of silencing potential useful errors ?

While the error message is only displayed in some systems's terminal (so it is not displayed in general), and it is a little bit misleading (so it is not useful to be displayed), and the GDAL's error silencing is used in various other part of the code (so it shouldn't be be an issue to use it in this particular function), I also think, like you, that it would be better to find another approach.

I don't know a proper way to check if a GDAL driver is capable to open a file in GA_Update mode.

Assuming (but I'm not sure) that only GTiff and PDF drivers can handle to store georeference and metadata information inside the file (which QgsLayoutExporter::georeferenceOutputPrivate tries to add to the already exported image file), what do you think about adding the following check in georeferenceOutputPrivate instead of silencing the error?

  const char *const apszAllowedDrivers[] = { "GTiff", "PDF", nullptr };
  if ( !GDALIdentifyDriverEx( file.toUtf8().constData(), GDAL_OF_RASTER, apszAllowedDrivers, nullptr ) )
    return false;

@rouault
Copy link
Contributor

rouault commented Jan 22, 2025

I don't know a proper way to check if a GDAL driver is capable to open a file in GA_Update mode.

yes, what I was refereing above: there's none currently

what do you think about adding the following check in georeferenceOutputPrivate instead of silencing the error?

const char *const apszAllowedDrivers[] = { "GTiff", "PDF", nullptr };
if ( !GDALIdentifyDriverEx( file.toUtf8().constData(), GDAL_OF_RASTER, apszAllowedDrivers, nullptr ) )
return false;

hum, that's a bit restrictive, although I guess that would cover 90% of the update scenarios.
But why not going along my suggestions of at least returning false if GDALOpen() fails ?

Or maybe let that (somewhat minor) topic aside until GDAL offers something cleaner ?

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented Jan 22, 2025

yes, what I was refereing above: there's none currently

Oops... I was writing the comment before your comment get published, so I haven't read it before publishing my comment...

But why not going along my suggestions of at least returning false if GDALOpen() fails ?

I'm not sure to understand.

The issue is that when GDALOpen() in GA_Update mode fails (e.g. for PNG), then the "ERROR 6: The PNG driver does not support update access to existing datasets." error message in printed in some system's terminal: that seems to create confusion since the PNG file is actually correctly created even if GDALOpen() fails and the error message is misleading in such situation (see "Would anyone know why that is? ... However, there is no updating within this process but simply creation." in https://gis.stackexchange.com/questions/360254/pyqgis-exporting-print-layout-error-6-the-png-driver-does-not-support-update).

It looks like a similar approach (to restrict the opening in GA_Update mode only to exported files with PDF or TIF or TIFF format) is currently used in QgsMapRendererTask

if ( mFileFormat == QLatin1String( "PDF" ) )
{
if ( mForceRaster )
{
QPainter pp;
pp.begin( mPdfWriter.get() );
const QRectF rect( 0, 0, mImage.width(), mImage.height() );
pp.drawImage( rect, mImage, rect );
pp.end();
}
if ( mSaveWorldFile || mExportMetadata )
{
CPLSetThreadLocalConfigOption( "GDAL_PDF_DPI", QString::number( mMapSettings.outputDpi() ).toLocal8Bit().constData() );
const gdal::dataset_unique_ptr outputDS( GDALOpen( mFileName.toUtf8().constData(), GA_Update ) );

if ( outputSuffix.compare( QLatin1String( "TIF" ), Qt::CaseInsensitive ) == 0 || outputSuffix.compare( QLatin1String( "TIFF" ), Qt::CaseInsensitive ) == 0 )
{
const gdal::dataset_unique_ptr outputDS( GDALOpen( mFileName.toUtf8().constData(), GA_Update ) );

Moreover, I cannot find where in the code it is actually checked what georeferenceOutputPrivate and georeferenceOutput returns.

Or maybe let that (somewhat minor) topic aside until GDAL offers something cleaner ?

I agree with you that if the proposed approaches are not considered optimal, when can wait to fix this minor issue later.

@rouault
Copy link
Contributor

rouault commented Jan 22, 2025

since the PNG file is actually correctly created even if GDALOpen() fails

if you open that PNG, is it georeferenced ? (with the georeferencing in the .aux.xml side car file)

It looks like a similar approach (to only restrict the opening in GA_Update mode only to exported files with PDF or TIF or TIFF format) is currently used in QgsMapRendererTask

That code in QgsMapRenderedTask looks a bit like band-aid to a cleaner approach that would be needed, or just a down-to-earth approach trying to covering the most common use cases, which is also fine sometimes.

Moreover, I cannot find where in the code is actually checked what georeferenceOutputPrivate and georeferenceOutput returns.

indeed, so there might be something missing to report to users that not everything went right.

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented Jan 22, 2025

if you open that PNG, is it georeferenced ? (with the georeferencing in the .aux.xml side car file)

No. Using the GUI, it only has gereferencing info in an ESRI World File .pgw sidecar file if the "Generate world file" checkbox is checked:
it seems it is expected according to the docs "If you want to export your layout as a georeferenced image (e.g., to share with other projects), check the unchecked Generate world file option, and an ESRI World File with the same name as the exported image, but a different extension will be created when exporting." https://docs.qgis.org/testing/en/docs/user_manual/print_composer/create_output.html#export-as-image and according to the tests in testExportToImage and testExportWorldFile.

Anyway, I agree with you that if the proposed approaches are not considered optimal, whe can wait to fix this minor issue later.

@troopa81
Copy link
Contributor

Filter on format is fine by, but it could indeed lead to issues if other format supporting it are missing (or added later in gdal).

The issue is that when GDALOpen() in GA_Update mode fails (e.g. for PNG), then the "ERROR 6: The PNG driver does not support update access to existing datasets." error message in printed in some system's terminal: that seems to create confusion since the PNG file is actually correctly created even if GDALOpen() fails and the error message is misleading in such situation (see "Would anyone know why that is? ... However, there is no updating within this process but simply creation." in https://gis.stackexchange.com/questions/360254/pyqgis-exporting-print-layout-error-6-the-png-driver-does-not-support-update)

But shall we not instead:

  • make georeferenceOutputPrivate return false when update fails
  • if false, display a user message alike "Georeferencing has failed, maybe the format is not supporting it, please consider a format that support georefenrecing (PDF, GTiff)"
  • let the debug message being dispplayed in the console for debug purpose only

This way user is fully aware of what happen and has an hint on how to fix it!

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented Jan 23, 2025

My understanding is that, from a user point of view (I've failed to find issue reports about missing georef info) and according to the documentation and the tests, not having georeference info for PNG or JPEG files (or other non GeoTIFF formats) is expected unless the "Generate world file" checkbox is checked, so there is no need to either print the "ERROR 6: ..." message in the console or to display a dialog window in the GUI.

There is probably a not quite straightforward and clear logic in QgsLayoutExporter::exportToImage (and "Export as Image") and QgsLayoutExporter::georeferenceOutputPrivate (considering it is also called by QgsLayoutExporter::exportToPDF and it tries to do the same actions for both images and PDF) and it seems also inconsistent with the logic of QgsMapRendererTask (and "Export Map to Image")

image

So, maybe better to wait for a comprehensive reworking of the logic of both in a more coherent way.

@rouault
Copy link
Contributor

rouault commented Jan 23, 2025

See proposed related GDAL RFC: OSGeo/gdal#11708

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QgsLayoutExporter export png file failed.
3 participants