Resolves#10069
Currently, the DDS header information for the width, height, and bytes per scan line
are read in and assumed to be correct. As these values are used for memory allocation
and reading, it would be good to verify they do not exceed the file size.
This patch adds a condition after the header is read in to verify those values. If they exceed
the file size (mins an offset), the file is not read in and an error message is shown.
GLib has a specific type for byte arrays: `GBytes` (and it's underlying
GType `G_TYPE_BYTES`).
By using this type, we can avoid having a `GimpUint8Array` which is a
bit cumbersome to use for both the C API, as well as bindings. By using
`GBytes`, we allow other languages to pass on byte arrays as they are
used to, while the bindings will make sure to do the right thing.
In the end, it makes the API a little bit simpler for everyone, and
reduces confusion for people who are used to working with byte arrays
in other C/GLib based code (and not having 2 different types to denote
the same thing).
Related: https://gitlab.gnome.org/GNOME/gimp/-/issues/5919
- This is unneeded in all import procedures. See previous commit. Note though
that this is not because of a change in previous commit. This was already
useless previously. The file set with this PDB function was overridden by the
core anyway (i.e. even before the previous commits).
In app/file/file-import.c:file_import_image(), the imported file is correctly
set (so there is no need to set it from plug-in, which anyway libgimp's
gimp_image_set_file() was not doing) and the XCF file is reset to NULL
(rendering the call to gimp_image_set_file() in a GimpLoadProcedure useless).
- Similarly, this is a useless call in export procedures because
app/file/file-save.c:file_save() overrides such call too. I could only see one
such case for JPEG export, which was quite useless.
- Finally in other types of plug-ins, setting a non-XCF file extension was
interfering with the save feature (similarly to commit e6e73e14c7). I only
fixed the screenshot implementations doing such a thing.
- I left a few usages which will have to be looked at more in details later.
Now that we bumped our meson requirement, meson is complaining about
several features now deprecated even in the minimum required meson
version:
s/meson.source_root/meson.project_source_root/ to fix:
> WARNING: Project targets '>=0.56.0' but uses feature deprecated since '0.56.0': meson.source_root. use meson.project_source_root() or meson.global_source_root() instead.
s/meson.build_root/meson.project_build_root/ to fix:
> WARNING: Project targets '>=0.56.0' but uses feature deprecated since '0.56.0': meson.build_root. use meson.project_build_root() or meson.global_build_root() instead.
Fixing using path() on xdg_email and python ExternalProgram variables:
> WARNING: Project targets '>=0.56.0' but uses feature deprecated since '0.55.0': ExternalProgram.path. use ExternalProgram.full_path() instead
s/get_pkgconfig_variable *(\([^)]*\))/get_variable(pkgconfig: \1)/ to
fix:
> WARNING: Project targets '>=0.56.0' but uses feature deprecated since '0.56.0': dependency.get_pkgconfig_variable. use dependency.get_variable(pkgconfig : ...) instead
This is the consequence of previous commit. Plug-ins' label and
documentation are now localized before sending these data to GIMP core.
In other words, we replace N_() macros with basic gettext calls.
Hence avoiding the stderr messages. These are going to be localized with
centrally installed catalogs "gimp*-std-plugins", "gimp*-script-fu" and
"gimp*-python".
We now handle core plug-in localizations differently and in particular,
with kind of a reverse logic:
- We don't consider "gimp*-std-plugins" to be the default catalog
anymore. It made sense in the old world where we would consider the
core plug-ins to be the most important and numerous ones. But we want
to push a world where people are even more encouraged to develop their
own plug-ins. These won't use the standard catalog anymore (because
there are nearly no reasons that the strings are the same, it's only a
confusing logic). So let's explicitly set the standard catalogs with
DEFINE_STD_SET_I18N macro (which maps to a different catalog for
script-fu plug-ins).
- Doing something similar for Python plug-ins which have again their own
catalog.
- Getting rid of the INIT_I18N macro since now all the locale domain
binding is done automatically by libgimp when using the set_i18n()
method infrastructure.
We have some examples of DDS images that have two 16-bit channels that
we could not load.
This commit adds checks to see if we need 16-bit channels, and conversion
of the input to a correct 16-bit red, green, blue or alpha channel.
To be able to use certain variables we use in our DDS loader earlier,
we move the computation of these variables up.
We also add checks to see if the mask needs to be for 8 or 16-bit, since
there are DDS images with 16 bits per sample.
This proved to be both an import and export issue.
Our import set expected format as RGB, causing garbled image output.
Our export for indexed images converted to grayscale first, although the
palette was correctly saved. This caused wrong palette indexes on import.
For indexed images, on import, we request the actual indexed format after
creating the layer with gimp_drawable_get_format, which gives us a correct
indexed Babl format.
Also added logic for indexed with alpha, although I have no sample images
to test this.
For indexed images on export we do the same: use gimp_drawable_get_format
to get an actual indexed Babl format.
While doing this cleanup, I found at least several other string leaks
in: file-compressor, file-gegl, file-pdf-save, file-raw-data, file-xwd,
jpeg-load, psd-save…
So it's quite worth it!
Note: in file-pdf-save, there is a global variable file_name which seems
to be happily leaked without caring (didn't look in details, but looks
so). I didn't fix this one which will require a bit more in-depth logics
care.
Freeing the path immediately could lead to a free-after-use error as we
used it for an error message when the file pointer failed to be created,
as reported by Massimo (thanks again!).
Using g_file_peek_path() has also other advantages, such as being less
error-prone, but also possibly more efficient. First looking at the
implementation, for local files, we already have the path around, so no
additional memory allocation even needs to happen. As for the generic
code path, it would still allocate a new string, yet cache it and reuse
it when needed later. This makes the _peek_ call much better for quick
peek-use-discard usage.
So far all dds images were loaded as 8 bit per channel which makes
sense for most but not all dds formats.
This commit implements loading in 16 bit for "L16" - a 16 bit
luminance channel.
In addition to that we improve security a bit by not assuming
that L16 is the only left over case but instead explicitly
checking the correct value of rmask for L16.
For other cases we now set an error with enough details
to identify the type of DDS image that needs extra
handling.
Looking at the documentation it is indeed red that should go
in the lowest bits and blue in the highest bits so just
reverse our code for red and blue.
We also update the version of our GIMP DDS plug-in,
this way we can catch and correct RGB10A2 images
written by older versions of our plug-in and correct
them.
This is the same as what we did for #5357
but in the reverse situation. The reason
for not doing the same at that time was
because I wasn't sure if that was valid
in all cases.
Looking at the documentation it does seem
to be the right solution and is working for
the supplied example image.
The gimp_drawable_type() is an issue though as gimp_drawable_get_type()
is already defined as a common GObject API.
Though I'm actually wondering if GimpImageType is well called. Rather
than Type, shouldn't we go with ColorModel?
sed -i 's/\<gimp_drawable_bpp\>/gimp_drawable_get_bpp/g' "$@"
sed -i 's/\<gimp_drawable_width\>/gimp_drawable_get_width/g' "$@"
sed -i 's/\<gimp_drawable_height\>/gimp_drawable_get_height/g' "$@"
sed -i 's/\<gimp_drawable_offsets\>/gimp_drawable_get_offsets/g' "$@"
s/gimp_image_base_type/gimp_image_get_base_type/
s/gimp_image_width/gimp_image_get_width/
s/gimp_image_height/gimp_image_get_height/
Sorry plug-in developers, more porting work! But really this seems like
the right thing to do in order not to get stuck with inconsistent naming
for many more years to come.
Information in issue #6200 revealed that 0 as default value for
the blue channel is a lot more common than 255 so let's use
that instead. The discussion and testing revealed no negative
effects for the other formats that use the same code to
initialize the memory to read blocks of image data.
Since older versions of our GIMP dds file exporter incorrectly
saved BC5 dds images with the red and green channels
swapped we should fix that. Since the exporter already
wrote the plug-ins version number and that it is written by
GIMP we can check for these incorrect images.
To enable that we increase the plug-ins revision in this
commit and swap red and green channels for images
that have an older version number and are of the
correct type.
Fatal errors should not be done with g_message(), but by setting the
returned GError appropriately. The main GError must therefore be passed
through functions.
Some non-fatal warnings may still be outputted with g_message() if we
want people to be aware of them (for instance if we managed to load some
data yet there might have been data loss or errors).
Finally when we are confident that we recovered from the format error,
it is still nice to output some error on standard error (not be totally
silent about it), but probably not need to bother people with popups.
See #5357.
Also clean up C++-style comments into C-style, to follow our coding
style.
Note: I only cleaned ddsread as a continuation of commit 81a3370e1d. I
expect ddswrite to have similar issues, but I did not look at it.
This commit just changes our saving API (i.e. the GimpSaveProcedure
class) to take an array of drawables as argument instead of a single
drawable.
It actually doesn't matter much for exporting as the whole API seems
more or less bogus there and all formats plug-ins mostly care only
whether they will merge/flatten all visible layers (the selected ones
don't really matter) or if the format supports layers of some sort. It
may be worth later strengthening a bit this whole logics, and maybe
allow partial exports for instance.
As for saving, it was not even looking at the passed GimpDrawable either
and was simply re-querying the active layer anyway.
Note that I don't implement the multi-selection saving in XCF yet in
this commit. I only updated the API. The reason is that the current
commit won't be backportable to gimp-2-10 because it is an API break. On
the other hand, the code to save multi-selection can still be backported
even though the save() API will only pass a single drawable (as I said
anyway, this argument was mostly bogus until now, hence it doesn't
matter much for 2.10 logics).
This gives a big cleanup in the meson.build files of the plug-ins.
It's also quite a bit more maintainable, since anything that changes in
libgimp's dependencies, linkage, ... doesn't have to be copy-pasted into
each plug-in.
There were 2 warnings of unused functions on this file, and masking
them, we were ending with a whole file #ifdef-ed 0. So let's just get
rid of the whole useless file.
They are semantically wrong anyway because they work on drawable,
assuming that the drawable is encoded in whatever non-RGB pixel (while
getting data in R'G'B'A format). This is just an internal trick of the
file-dds plug-in which converts DDS data internally instead of having
babl doing the job by making appropriate formats.
Anyway we should definitely not expose such procedure publicly. I am not
deleting the code directly, just hiding it away in `#if 0` for now.
and in an attack of madness, changes almost all file plug-in
code to use GFile instead of filenames, which means passing
the GFile down to the bottom and get its filename at the very
end where it's actually needed.
Alx Sa