glTF: Fix parsing image data with `mimeType` undefined

The glTF 2.0 spec only makes `mimeType` mandatory for `bufferView` image data,
so the previous logic to handle URIs with base64-encoded images could fail if
`mimeType` is undefined.

The logic was documented and refactored to better handle the spec, notably:

- `uri` and `bufferView` are now mutually exclusive, and only the latter fails
  if `mimeType` is undefined.
- `uri` with a file path will now respect the `mimeType` if defined, and thus
  attempt loading the file with the specified format (even if its extension is
  not the one expected for this format). So we can support bad extensions (PNG
  data with `.jpg` extension) or custom ones (PNG data in `.img` file for
  example).
- `uri` with base64 encoded data will infer MIME type from `data:image/png` or
  `data:image/jpeg` if it was not documented in `mimeType` initially.
- `uri` with base64 encoded data, no `mimeType` and `application/octet-stream`
  or `application/gltf-buffer` will fall back to trying both PNG and JPEG
  loaders.

Fully fixes #33796 (and fixes up #42501).

2 files changed, 85 insertions, 52 deletions

diff --git a/editor/import/editor_scene_importer_gltf.cpp b/editor/import/editor_scene_importer_gltf.cpp
index f4171eda32..0c67c769ef 100644
--- a/editor/import/editor_scene_importer_gltf.cpp
+++ b/editor/import/editor_scene_importer_gltf.cpp
@@ -381,19 +381,15 @@ Error EditorSceneImporterGLTF::_parse_buffers(GLTFState &state, const String &p_
 
 				if (uri.begins_with("data:")) { // Embedded data using base64.
 					// Validate data MIME types and throw an error if it's one we don't know/support.
-					// Could be an importer bug on our side or a broken glTF file.
-					// Ref: https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#file-extensions-and-mime-types
 					if (!uri.begins_with("data:application/octet-stream;base64") &&
-							!uri.begins_with("data:application/gltf-buffer;base64") &&
-							!uri.begins_with("data:image/jpeg;base64") &&
-							!uri.begins_with("data:image/png;base64")) {
-						ERR_PRINT("glTF file contains buffer with an unknown URI data type: " + uri);
+							!uri.begins_with("data:application/gltf-buffer;base64")) {
+						ERR_PRINT("glTF: Got buffer with an unknown URI data type: " + uri);
 					}
 					buffer_data = _parse_base64_uri(uri);
-				} else { // Should be a relative file path.
+				} else { // Relative path to an external image file.
 					uri = p_base_path.plus_file(uri).replace("\\", "/"); // Fix for Windows.
 					buffer_data = FileAccess::get_file_as_array(uri);
-					ERR_FAIL_COND_V_MSG(buffer.size() == 0, ERR_PARSE_ERROR, "Couldn't load binary file as an array: " + uri);
+					ERR_FAIL_COND_V_MSG(buffer.size() == 0, ERR_PARSE_ERROR, "glTF: Couldn't load binary file as an array: " + uri);
 				}
 
 				ERR_FAIL_COND_V(!buffer.has("byteLength"), ERR_PARSE_ERROR);
@@ -1269,12 +1265,28 @@ Error EditorSceneImporterGLTF::_parse_images(GLTFState &state, const String &p_b
 		return OK;
 	}
 
+	// Ref: https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#images
+
 	const Array &images = state.json["images"];
 	for (int i = 0; i < images.size(); i++) {
 		const Dictionary &d = images[i];
 
+		// glTF 2.0 supports PNG and JPEG types, which can be specified as (from spec):
+		// "- a URI to an external file in one of the supported images formats, or
+		//  - a URI with embedded base64-encoded data, or
+		//  - a reference to a bufferView; in that case mimeType must be defined."
+		// Since mimeType is optional for external files and base64 data, we'll have to
+		// fall back on letting Godot parse the data to figure out if it's PNG or JPEG.
+
+		// We'll assume that we use either URI or bufferView, so let's warn the user
+		// if their image somehow uses both. And fail if it has neither.
+		ERR_CONTINUE_MSG(!d.has("uri") && !d.has("bufferView"), "Invalid image definition in glTF file, it should specific an 'uri' or 'bufferView'.");
+		if (d.has("uri") && d.has("bufferView")) {
+			WARN_PRINT("Invalid image definition in glTF file using both 'uri' and 'bufferView'. 'bufferView' will take precedence.");
+		}
+
 		String mimetype;
-		if (d.has("mimeType")) {
+		if (d.has("mimeType")) { // Should be "image/png" or "image/jpeg".
 			mimetype = d["mimeType"];
 		}
 
@@ -1283,23 +1295,52 @@ Error EditorSceneImporterGLTF::_parse_images(GLTFState &state, const String &p_b
 		int data_size = 0;
 
 		if (d.has("uri")) {
+			// Handles the first two bullet points from the spec (embedded data, or external file).
 			String uri = d["uri"];
 
-			if (uri.findn("data:application/octet-stream;base64") == 0 ||
-					uri.findn("data:" + mimetype + ";base64") == 0) {
-				//embedded data
+			if (uri.begins_with("data:")) { // Embedded data using base64.
+				// Validate data MIME types and throw an error if it's one we don't know/support.
+				if (!uri.begins_with("data:application/octet-stream;base64") &&
+						!uri.begins_with("data:application/gltf-buffer;base64") &&
+						!uri.begins_with("data:image/png;base64") &&
+						!uri.begins_with("data:image/jpeg;base64")) {
+					ERR_PRINT("glTF: Got image data with an unknown URI data type: " + uri);
+				}
 				data = _parse_base64_uri(uri);
 				data_ptr = data.ptr();
 				data_size = data.size();
-			} else {
-				uri = p_base_path.plus_file(uri).replace("\\", "/"); //fix for windows
-				Ref<Texture2D> texture = ResourceLoader::load(uri);
-				state.images.push_back(texture);
-				continue;
+				// mimeType is optional, but if we have it defined in the URI, let's use it.
+				if (mimetype.empty()) {
+					if (uri.begins_with("data:image/png;base64")) {
+						mimetype = "image/png";
+					} else if (uri.begins_with("data:image/jpeg;base64")) {
+						mimetype = "image/jpeg";
+					}
+				}
+			} else { // Relative path to an external image file.
+				uri = p_base_path.plus_file(uri).replace("\\", "/"); // Fix for Windows.
+				// The spec says that if mimeType is defined, we should enforce it.
+				// So we should only rely on ResourceLoader::load if mimeType is not defined,
+				// otherwise we should use the same logic as for buffers.
+				if (mimetype == "image/png" || mimetype == "image/jpeg") {
+					// Load data buffer and rely on PNG and JPEG-specific logic below to load the image.
+					// This makes it possible to load a file with a wrong extension but correct MIME type,
+					// e.g. "foo.jpg" containing PNG data and with MIME type "image/png". ResourceLoader would fail.
+					data = FileAccess::get_file_as_array(uri);
+					ERR_FAIL_COND_V_MSG(data.size() == 0, ERR_PARSE_ERROR, "glTF: Couldn't load image file as an array: " + uri);
+					data_ptr = data.ptr();
+					data_size = data.size();
+				} else {
+					// Good old ResourceLoader will rely on file extension.
+					Ref<Texture2D> texture = ResourceLoader::load(uri);
+					state.images.push_back(texture);
+					continue;
+				}
 			}
-		}
+		} else if (d.has("bufferView")) {
+			// Handles the third bullet point from the spec (bufferView).
+			ERR_FAIL_COND_V_MSG(mimetype.empty(), ERR_FILE_CORRUPT, "glTF: Image specifies 'bufferView' but no 'mimeType', which is invalid.");
 
-		if (d.has("bufferView")) {
 			const GLTFBufferViewIndex bvi = d["bufferView"];
 
 			ERR_FAIL_INDEX_V(bvi, state.buffer_views.size(), ERR_PARAMETER_RANGE_ERROR);
@@ -1315,45 +1356,36 @@ Error EditorSceneImporterGLTF::_parse_images(GLTFState &state, const String &p_b
 			data_size = bv.byte_length;
 		}
 
-		ERR_FAIL_COND_V(mimetype == "", ERR_FILE_CORRUPT);
+		Ref<Image> img;
 
-		if (mimetype.findn("png") != -1) {
-			//is a png
+		if (mimetype == "image/png") { // Load buffer as PNG.
 			ERR_FAIL_COND_V(Image::_png_mem_loader_func == nullptr, ERR_UNAVAILABLE);
-
-			const Ref<Image> img = Image::_png_mem_loader_func(data_ptr, data_size);
-
-			ERR_FAIL_COND_V(img.is_null(), ERR_FILE_CORRUPT);
-
-			Ref<ImageTexture> t;
-			t.instance();
-			t->create_from_image(img);
-
-			state.images.push_back(t);
-			continue;
-		}
-
-		if (mimetype.findn("jpeg") != -1) {
-			//is a jpg
+			img = Image::_png_mem_loader_func(data_ptr, data_size);
+		} else if (mimetype == "image/jpeg") { // Loader buffer as JPEG.
 			ERR_FAIL_COND_V(Image::_jpg_mem_loader_func == nullptr, ERR_UNAVAILABLE);
+			img = Image::_jpg_mem_loader_func(data_ptr, data_size);
+		} else {
+			// We can land here if we got an URI with base64-encoded data with application/* MIME type,
+			// and the optional mimeType property was not defined to tell us how to handle this data (or was invalid).
+			// So let's try PNG first, then JPEG.
+			ERR_FAIL_COND_V(Image::_png_mem_loader_func == nullptr, ERR_UNAVAILABLE);
+			img = Image::_png_mem_loader_func(data_ptr, data_size);
+			if (img.is_null()) {
+				ERR_FAIL_COND_V(Image::_jpg_mem_loader_func == nullptr, ERR_UNAVAILABLE);
+				img = Image::_jpg_mem_loader_func(data_ptr, data_size);
+			}
+		}
 
-			const Ref<Image> img = Image::_jpg_mem_loader_func(data_ptr, data_size);
-
-			ERR_FAIL_COND_V(img.is_null(), ERR_FILE_CORRUPT);
-
-			Ref<ImageTexture> t;
-			t.instance();
-			t->create_from_image(img);
-
-			state.images.push_back(t);
+		ERR_FAIL_COND_V_MSG(img.is_null(), ERR_FILE_CORRUPT, "glTF: Couldn't load image with its given mimetype: " + mimetype);
 
-			continue;
-		}
+		Ref<ImageTexture> t;
+		t.instance();
+		t->create_from_image(img);
 
-		ERR_FAIL_V(ERR_FILE_CORRUPT);
+		state.images.push_back(t);
 	}
 
-	print_verbose("Total images: " + itos(state.images.size()));
+	print_verbose("glTF: Total images: " + itos(state.images.size()));
 
 	return OK;
 }
@@ -1513,7 +1545,7 @@ Error EditorSceneImporterGLTF::_parse_materials(GLTFState &state) {
 		state.materials.push_back(material);
 	}
 
-	print_verbose("Total materials: " + itos(state.materials.size()));
+	print_verbose("glTF: Total materials: " + itos(state.materials.size()));
 
 	return OK;
 }
@@ -3064,6 +3096,8 @@ Node3D *EditorSceneImporterGLTF::_generate_scene(GLTFState &state, const int p_b
 }
 
 Node *EditorSceneImporterGLTF::import_scene(const String &p_path, uint32_t p_flags, int p_bake_fps, List<String> *r_missing_deps, Error *r_err) {
+	print_verbose(vformat("glTF: Importing file %s as scene.", p_path));
+
 	GLTFState state;
 
 	if (p_path.to_lower().ends_with("glb")) {
diff --git a/scene/resources/mesh.cpp b/scene/resources/mesh.cpp
index 10f0a040d0..e9606e03e6 100644
--- a/scene/resources/mesh.cpp
+++ b/scene/resources/mesh.cpp
@@ -872,7 +872,6 @@ Array ArrayMesh::_get_surfaces() const {
 
 		ret.push_back(data);
 	}
-	print_line("Saving surfaces: " + itos(ret.size()));
 
 	return ret;
 }