app: fix a crash when converting to higher precision.
gimp_display_shell_render() writes to a GeglBuffer backed by allocated memory (shell->profile_data). Unfortunately while converting prevision in gimp_image_convert_precision(), we change the "precision" property (hence the source format) first, hence end up trying to write data in a too small buffer. This crash was hard to find as it was not showing up on my machine (though it did produce rendering artifacts!), unless I built both GIMP and babl with `b_sanitize=address`. Note that an alternate fix was to make sure that the profile_data buffer is big enough (by calling gimp_display_shell_profile_update() before rendering), but anyway the image is in an inconsistent state while conversion is in progress: whereas the `src_format` is the new one, the `src_profile` is still the old one (and cannot be changed before we finish converting). Moreover the render happen regularly on progress signals, once after each converted drawable. So each of these rendering step happens in an inconsistent state, with the wrong profile set, some of the drawables converted and others not yet. We could still render properly if each drawable's buffer used space-aware format (thus allowing different drawables to use different profiles/spaces), but it feels over-engineering the problem. It might be much better to ignore rendering steps while converting the image precision. Moreover it would obviously make a faster conversion. See discussions in #9136 for this crash, which didn't have dedicated report AFAIK.
This commit is contained in:
@ -135,6 +135,8 @@ gimp_image_convert_precision (GimpImage *image,
|
||||
old_profile = gimp_image_get_color_profile (image);
|
||||
old_format = gimp_image_get_layer_format (image, FALSE);
|
||||
|
||||
gimp_image_set_converting (image, TRUE);
|
||||
|
||||
/* Set the new precision */
|
||||
g_object_set (image, "precision", precision, NULL);
|
||||
|
||||
@ -227,6 +229,8 @@ gimp_image_convert_precision (GimpImage *image,
|
||||
g_object_unref (new_profile);
|
||||
}
|
||||
|
||||
gimp_image_set_converting (image, FALSE);
|
||||
|
||||
gimp_image_undo_group_end (image);
|
||||
|
||||
gimp_image_precision_changed (image);
|
||||
|
@ -64,6 +64,7 @@ struct _GimpImagePrivate
|
||||
|
||||
gboolean is_color_managed; /* is this image color managed */
|
||||
GimpColorProfile *color_profile; /* image's color profile */
|
||||
gboolean converting; /* color model or profile in middle of conversion? */
|
||||
|
||||
/* Cached color transforms: from layer to sRGB u8 and double, and back */
|
||||
gboolean color_transforms_created;
|
||||
|
@ -140,7 +140,8 @@ enum
|
||||
PROP_PRECISION,
|
||||
PROP_METADATA,
|
||||
PROP_BUFFER,
|
||||
PROP_SYMMETRY
|
||||
PROP_SYMMETRY,
|
||||
PROP_CONVERTING,
|
||||
};
|
||||
|
||||
|
||||
@ -692,6 +693,12 @@ gimp_image_class_init (GimpImageClass *klass)
|
||||
GIMP_TYPE_SYMMETRY,
|
||||
GIMP_PARAM_READWRITE |
|
||||
G_PARAM_CONSTRUCT));
|
||||
|
||||
g_object_class_install_property (object_class, PROP_CONVERTING,
|
||||
g_param_spec_boolean ("converting",
|
||||
NULL, NULL,
|
||||
FALSE,
|
||||
GIMP_PARAM_READWRITE));
|
||||
}
|
||||
|
||||
static void
|
||||
@ -1007,6 +1014,10 @@ gimp_image_set_property (GObject *object,
|
||||
}
|
||||
break;
|
||||
|
||||
case PROP_CONVERTING:
|
||||
private->converting = g_value_get_boolean (value);
|
||||
break;
|
||||
|
||||
case PROP_ID:
|
||||
case PROP_METADATA:
|
||||
case PROP_BUFFER:
|
||||
@ -1057,6 +1068,10 @@ gimp_image_get_property (GObject *object,
|
||||
G_TYPE_FROM_INSTANCE (private->active_symmetry) :
|
||||
G_TYPE_NONE);
|
||||
break;
|
||||
case PROP_CONVERTING:
|
||||
g_value_set_boolean (value, private->converting);
|
||||
break;
|
||||
|
||||
default:
|
||||
G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
|
||||
break;
|
||||
@ -5144,3 +5159,33 @@ gimp_image_invalidate_previews (GimpImage *image)
|
||||
gimp_item_stack_invalidate_previews (layers);
|
||||
gimp_item_stack_invalidate_previews (channels);
|
||||
}
|
||||
|
||||
/* Sets the image into a "converting" state, which is there to warn other code
|
||||
* (such as shell render code) that the image properties might be in an
|
||||
* inconsistent state. For instance when converting to another precision with
|
||||
* gimp_image_convert_precision(), the babl format may be updated first, and the
|
||||
* profile later, after all drawables are converted. Rendering the image
|
||||
* in-between would at best render broken previews (at worst, crash, e.g.
|
||||
* because we depend on allocated data which might have become too small).
|
||||
*/
|
||||
void
|
||||
gimp_image_set_converting (GimpImage *image,
|
||||
gboolean converting)
|
||||
{
|
||||
g_return_if_fail (GIMP_IS_IMAGE (image));
|
||||
|
||||
g_object_set (image,
|
||||
"converting", converting,
|
||||
NULL);
|
||||
}
|
||||
|
||||
gboolean
|
||||
gimp_image_get_converting (GimpImage *image)
|
||||
{
|
||||
GimpImagePrivate *private;
|
||||
g_return_val_if_fail (GIMP_IS_IMAGE (image), FALSE);
|
||||
|
||||
private = GIMP_IMAGE_GET_PRIVATE (image);
|
||||
|
||||
return private->converting;
|
||||
}
|
||||
|
@ -455,5 +455,9 @@ gboolean gimp_image_coords_in_active_pickable (GimpImage *image,
|
||||
|
||||
void gimp_image_invalidate_previews (GimpImage *image);
|
||||
|
||||
void gimp_image_set_converting (GimpImage *image,
|
||||
gboolean converting);
|
||||
gboolean gimp_image_get_converting (GimpImage *image);
|
||||
|
||||
|
||||
#endif /* __GIMP_IMAGE_H__ */
|
||||
|
@ -274,13 +274,17 @@ gimp_display_shell_canvas_expose (GtkWidget *widget,
|
||||
/* ignore events on overlays */
|
||||
if (eevent->window == gtk_widget_get_window (widget))
|
||||
{
|
||||
GimpImage *image = gimp_display_get_image (shell->display);
|
||||
cairo_t *cr;
|
||||
|
||||
cr = gdk_cairo_create (gtk_widget_get_window (shell->canvas));
|
||||
gdk_cairo_region (cr, eevent->region);
|
||||
cairo_clip (cr);
|
||||
|
||||
if (gimp_display_get_image (shell->display))
|
||||
/* If we are currently converting the image, it might be in inconsistent
|
||||
* state and should not be redrawn.
|
||||
*/
|
||||
if (image != NULL && ! gimp_image_get_converting (image))
|
||||
{
|
||||
gimp_display_shell_canvas_draw_image (shell, cr);
|
||||
}
|
||||
|
@ -75,6 +75,15 @@ gimp_display_shell_render (GimpDisplayShell *shell,
|
||||
g_return_if_fail (scale > 0.0);
|
||||
|
||||
image = gimp_display_get_image (shell->display);
|
||||
|
||||
/* While converting, the render can be wrong; but worse, we rely on allocated
|
||||
* data which might be the wrong size and this was a crash we had which was
|
||||
* hard to diagnose as it doesn't always crash immediately (see discussions in
|
||||
* #9136). This is why this assert is important. We want to make sure we never
|
||||
* call this when the shell's image is in the inconsistent "converting" state.
|
||||
*/
|
||||
g_return_if_fail (! gimp_image_get_converting (image));
|
||||
|
||||
buffer = gimp_pickable_get_buffer (
|
||||
gimp_display_shell_get_pickable (shell));
|
||||
#ifdef USE_NODE_BLIT
|
||||
|
Reference in New Issue
Block a user