Webp image loader and EXT_texture_webp#2722
Webp image loader and EXT_texture_webp#2722riccardobl wants to merge 5 commits intojMonkeyEngine:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces new image loaders based on the STB library and WebP decoder, aiming to replace older, platform-specific loaders like HDRLoader, TGALoader, and AWTLoader. It also adds support for the EXT_texture_webp glTF extension. My review highlights a critical thread-safety issue in StbImageLoader due to shared state, a potential loss of precision when loading 32-bit float HDR images, and a recommendation to improve memory efficiency by avoiding intermediate heap allocations during stream reading. Additionally, I suggest refining the exception handling in WebpImageLoader to avoid catching generic exceptions.
| import com.jme3.util.BufferUtils; | ||
|
|
||
| public class StbImageLoader implements AssetLoader { | ||
| private final StbImage stbImage = new StbImage(BufferUtils::createByteBuffer); |
There was a problem hiding this comment.
The stbImage instance is shared across all calls to load(). Since AssetLoader instances are typically singletons managed by the AssetManager, and stbImage is modified via setConvertIphonePngToRgb and setUnpremultiplyOnLoad inside the load() method (lines 38-39), this creates a race condition when multiple assets are loaded concurrently. To ensure thread safety, either move the configuration to the constructor if they are constant, or instantiate StbImage locally within the load() method.
| private final StbImage stbImage = new StbImage(BufferUtils::createByteBuffer); | |
| @Override | |
| public Object load(AssetInfo assetInfo) throws IOException { | |
| StbImage stbImage = new StbImage(BufferUtils::createByteBuffer); | |
| AssetKey<?> key = assetInfo.getKey(); |
| if (is16bit || isFloat) { | ||
| switch (channels) { | ||
| case 1: | ||
| jmeFormat = Image.Format.R16F; | ||
| desiredChannels = 1; | ||
| break; | ||
| case 2: | ||
| jmeFormat = Image.Format.RG16F; | ||
| desiredChannels = 2; | ||
| break; | ||
| case 3: | ||
| jmeFormat = Image.Format.RGB16F; | ||
| desiredChannels = 3; | ||
| break; | ||
| case 4: | ||
| jmeFormat = Image.Format.RGBA16F; | ||
| desiredChannels = 4; | ||
| break; | ||
| default: | ||
| throw new IOException("Unsupported number of channels: " + channels); | ||
|
|
||
| } |
There was a problem hiding this comment.
The current implementation downsamples 32-bit float images to 16-bit half-floats (Format.R16F, Format.RGB16F, etc.) regardless of whether the source is 16-bit or 32-bit float. This results in a loss of precision for HDR images that were previously supported at full 32-bit precision by HDRLoader. Consider distinguishing between is16bit and isFloat to select the appropriate 32F format when the source is a 32-bit float.
| }catch(Exception e){ | ||
| throw new IOException("Failed to load WebP image", e); | ||
| } |
There was a problem hiding this comment.
Catching a generic Exception is generally discouraged as it can mask unexpected runtime issues. It is better to catch specific exceptions like IOException or RuntimeException and handle them accordingly, or let them propagate if they cannot be handled locally.
} catch (IOException e) {
throw e;
} catch (Exception e) {
throw new IOException("Failed to load WebP image", e);
}| byte[] data = ByteUtils.getByteContent(is); | ||
| ByteBuffer buffer = BufferUtils.createByteBuffer(data); |
There was a problem hiding this comment.
This PR adds a Webp image loader for every platform (was only limited to android before this PR) using a java port of the rust crate image-webp's decoder.
Built on top of #2718
The PR also implements the
EXT_texture_webpfor the gltf loader.Closes #2616