Merge pull request #42504 from akien-mga/glTF-fix-image-loading

glTF: Fix parsing image data with `mimeType` undefined

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;
 }