Return null from loadTexture when image fails to load instead of wrongly creating a VideoTexture (#5781)

* Return null from loadTexture when image fails to load intead of wrongly creating a VideoTexture

* Also add tests for video not found

* Still use checkIsImageFallback in case of 3xx redirects or server not supporting HEAD requests

* We can assume server not supporting HEAD request that it's not a video, revert the previous logic

* Revert "Also add tests for video not found"

This reverts commit b7867d0.

* Remove third param

* Update comments

Author: vincentfretin

src/systems/material.js

@@ -30,10 +30,14 @@ export var System = registerSystem('material', {
    *
    * @param {string|Element} src - URL or element
    * @param {object} data - Relevant texture properties
-   * @param {function} cb - Callback to pass texture to
+   * @param {function} cb - Callback that receives the texture, or null if image loading failed
    */
   loadTexture: function (src, data, cb) {
     this.loadTextureSource(src, function sourceLoaded (source) {
+      if (source === null) {
+        cb(null);
+        return;
+      }
       var texture = createCompatibleTexture(source);
       setTextureProperties(texture, data);
       cb(texture);
@@ -44,15 +48,15 @@ export var System = registerSystem('material', {
    * Determine whether `src` is an image or video. Then try to load the asset, then call back.
    *
    * @param {string|Element} src - URL or element.
-   * @param {function} cb - Callback to pass texture source to.
+   * @param {function} cb - Callback that receives the texture source, or null if image loading failed.
    */
   loadTextureSource: function (src, cb) {
     var self = this;
     var sourceCache = this.sourceCache;
 
     var hash = this.hash(src);
     if (sourceCache[hash]) {
-      sourceCache[hash].then(cb);
+      sourceCache[hash].then(cb, function () { cb(null); });
       return;
     }
 
@@ -71,7 +75,7 @@ export var System = registerSystem('material', {
 
     function sourceLoaded (sourcePromise) {
       sourceCache[hash] = Promise.resolve(sourcePromise);
-      sourceCache[hash].then(cb);
+      sourceCache[hash].then(cb, function () { cb(null); });
     }
   },
 
@@ -199,8 +203,9 @@ function loadImageUrl (src) {
       resolveSource,
       function () { /* no-op */ },
       function (xhr) {
-        error('`$s` could not be fetched (Error code: %s; Response: %s)', xhr.status,
+        error('`%s` could not be fetched (Error code: %s; Response: %s)', src, xhr.status,
               xhr.statusText);
+        reject(new Error('Failed to load image: ' + src));
       }
     );
 

src/utils/src-loader.js

@@ -116,6 +116,17 @@ export function parseUrl (src) {
   return parsedSrc[1];
 }
 
+var IMAGE_EXTENSIONS = ['jpg', 'jpeg', 'png', 'webp', 'avif'];
+
+/**
+ * Get file extension from URL (without query string or hash).
+ */
+function getExtension (src) {
+  var pathname = src.split('?')[0].split('#')[0];
+  var ext = pathname.split('.').pop().toLowerCase();
+  return ext;
+}
+
 /**
  * Call back whether `src` is an image.
  *
@@ -124,11 +135,20 @@ export function parseUrl (src) {
  */
 function checkIsImage (src, onResult) {
   var request;
+  var ext;
 
   if (src.tagName) {
     onResult(src.tagName === 'IMG');
     return;
   }
+
+  // Check file extension first to avoid HEAD request for common image extensions.
+  ext = getExtension(src);
+  if (IMAGE_EXTENSIONS.indexOf(ext) !== -1) {
+    onResult(true);
+    return;
+  }
+
   request = new XMLHttpRequest();
 
   // Try to send HEAD request to check if image first.
@@ -139,21 +159,31 @@ function checkIsImage (src, onResult) {
       contentType = request.getResponseHeader('Content-Type');
       if (contentType == null) {
         checkIsImageFallback(src, onResult);
+      } else if (contentType.startsWith('image')) {
+        onResult(true);
       } else {
-        if (contentType.startsWith('image')) {
-          onResult(true);
-        } else {
-          onResult(false);
-        }
+        onResult(false);
       }
     } else {
+      // Non-success status (3xx redirects, 404, 405, etc.) - try loading via Image tag
+      // as it handles redirects and the server might not support HEAD requests.
       checkIsImageFallback(src, onResult);
     }
     request.abort();
   });
+  request.addEventListener('error', function () {
+    // Network error (CORS, etc.) - try loading via Image tag.
+    checkIsImageFallback(src, onResult);
+  });
   request.send();
 }
 
+/**
+ * Try loading src as an image to determine if it's an image.
+ *
+ * @param {string} src - URL to test.
+ * @param {function} onResult - Callback with result.
+ */
 function checkIsImageFallback (src, onResult) {
   var tester = new Image();
   tester.addEventListener('load', onLoad);

tests/systems/material.test.js

@@ -3,6 +3,7 @@ import { entityFactory } from '../helpers.js';
 
 var IMAGE1 = 'base/tests/assets/test.png';
 var IMAGE2 = 'base/tests/assets/test2.png';
+var IMAGE_FAIL = 'base/tests/assets/nonexistent.png';
 var VIDEO1 = 'base/tests/assets/test.mp4';
 var VIDEO2 = 'base/tests/assets/test2.mp4';
 
@@ -122,6 +123,26 @@ suite('material system', function () {
           done();
         });
       });
+
+      test('returns null when image fails to load', function (done) {
+        var system = this.system;
+
+        system.loadTextureSource(IMAGE_FAIL, function (source) {
+          assert.equal(source, null);
+          done();
+        });
+      });
+    });
+
+    suite('loadTexture', function () {
+      test('returns null when image fails to load', function (done) {
+        var system = this.system;
+
+        system.loadTexture(IMAGE_FAIL, {}, function (texture) {
+          assert.equal(texture, null);
+          done();
+        });
+      });
     });
 
     suite('loadVideo', function () {