Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Examples: Add webgl_loader_texture_hdrjpg #27183

Merged
merged 9 commits into from
Nov 19, 2023
Merged

Examples: Add webgl_loader_texture_hdrjpg #27183

merged 9 commits into from
Nov 19, 2023

Conversation

daniele-pelagatti
Copy link
Contributor

Related issue: #27171

Description

Adds an external example demonstrating the usage of the gainmap-js library with its loaders (see issue for a detailed description of what gain maps are and what they do)

@elalish
Copy link
Contributor

elalish commented Nov 13, 2023

From the previous thread:

I managed to get rid of the wasm for extracting the XMP Metadata end the gain map image.

The whole extraction process in pure js lasts ~158ms for a 16k Equirectangular JPEG image which is the maximum supported resolution of our library.

For comparison, the texSubImage2D call for uploading the same texture on the GPU lasts ~1176ms on my machine so the parsing speed is not that bad.

It looks like there's nothing stopping me from integrating this into <model-viewer> now. I'll have to see if I can manage that next week.

Regarding the failed test - how many platforms have you tested this on? Sometimes the CI fails because SwiftShader gets different results (CPU vs GPU rendering). It might be worth poking around a bit to see if your shader has any lines that hit popular driver bugs.

@daniele-pelagatti
Copy link
Contributor Author

Regarding the failed test - how many platforms have you tested this on? Sometimes the CI fails because SwiftShader gets different results (CPU vs GPU rendering). It might be worth poking around a bit to see if your shader has any lines that hit popular driver bugs.

The failed test is weird, I'm not sure if its caused by puppeteer or not because I've tried to run npm run make-screenshot in linux and I got a black torus knot (as if the envMap wasn't set), but in the same machine I can see a non-black torus knot when running the example in a normal chrome browser windows in linux.

I re-ran npm run make-screenshot in windows and I got the correct screenshot, so I've used that one in the PR.

Now it seems the same "black torus knot" is happening also in the ci env which uses macos AFAIK...but I'm really not sure why that happens, tomorrow I'll test in chrome + macos plus other browsers.

I've tested my example in

  1. windows + chrome
  2. windows + firefox
  3. linux + chrome
  4. linux + firefox
  5. Android chrome

Tomorrow I'll test in iOS and OSX.

The library uses createImageBitmap (which is not super widely supported) only in the encoder part, which is separated from the loaders. Nothing else unusual to report

@elalish
Copy link
Contributor

elalish commented Nov 13, 2023

Yeah, so the difference between running in puppeteer vs normal Chrome is the first uses SwiftShader (CPU rendering), while the second uses your GPU driver. Like all drivers, SwiftShader has its own set of bugs. Do you know what version of WebGL / extensions you're targeting?

@daniele-pelagatti
Copy link
Contributor Author

Yeah, so the difference between running in puppeteer vs normal Chrome is the first uses SwiftShader (CPU rendering), while the second uses your GPU driver. Like all drivers, SwiftShader has its own set of bugs. Do you know what version of WebGL / extensions you're targeting?

Got it.

I'm targeting Webgl2 with EXT_color_buffer_half_float (I believe that's required for half float render targets, which we use)

Given the fact that the torus is black, maybe the discrepancy is caused by my usage of a rendertarget as source for PMREMGenerator fromEquirectangular -> envMap?

This usage is maybe not very common? It worked in my tests but I'm not sure.

Also (maybe unrelated) there's a line that shouldn't be there in my code. During preliminary tests, Firefox didn't behave correctly when performing readRenderTargetPixels from half float render targets so I added an exception and forced Float RTs, that probably wasn't smart but... Today I tested again and Firefox worked fine even without that line...weird, in any case I'll do some more tests tomorrow and maybe publish a new version without that exception.

@daniele-pelagatti
Copy link
Contributor Author

daniele-pelagatti commented Nov 14, 2023

Okay, I did more tests, the example + the decoder in https://gainmap-creator.mono-grid.com (which uses the same library) both run fine on

  • MacOS Ventura 13.4 + Safari 16.5
  • MacOS Catalina 10.15.7 + Safari 15.6.1
  • MacOS Catalina 10.15.7 + Chrome 119.0
  • MacOS Catalina 10.15.7 + Firefox 119.0

but I was able to reproduce the "black torus" bug on iOS 17.1 + Safari.

The decoding part done by the library seems to work fine because I can see the scene.background so that means the decoding finished successfully.

It seems the part of the example involving PMREMGenerator is the culprit. I pass a renderTarget.texture as a source for fromEquirectangular and, from attaching the device to a debugger, I can't see any error being thrown but the torus is black. When switching to the normal HDR the Torus gets rendered correctly again.

EDIT: I tested again in the same iOS 17.1 + Safari hardware and the decoding on https://gainmap-creator.mono-grid.com runs fine, the problem has definitely something to do with the PMREMGenerator

…ompatibility problems in some browsers and platforms + adds progress handler for separate gainmap data
@daniele-pelagatti
Copy link
Contributor Author

Alright, it looks like my shader needed to clamp to half float values -65504 to 65504: there seems to be no loss of HDR range and all tests are passing

@mrdoob mrdoob added this to the r159 milestone Nov 15, 2023
@arpu
Copy link

arpu commented Nov 15, 2023

some infos found for gainmaps and ImageMagic ImageMagick/ImageMagick#6377

@mrdoob
Copy link
Owner

mrdoob commented Nov 16, 2023

One last thing...

What's the reasoning behind the JPEGRLoader naming?
And... Could it be HDRJPGLoader instead? 🤔

@daniele-pelagatti
Copy link
Contributor Author

daniele-pelagatti commented Nov 16, 2023

One last thing...

What's the reasoning behind the JPEGRLoader naming? And... Could it be HDRJPGLoader instead? 🤔

Good point, I'm conflicted on this because a JPEG file with an embedded gain map has not been given a established and commonly used name yet (at least AFAIK) , that only definition I found around is from the libultrahdr code which refers to these files with the naming JPEGR so I assumed they are going to push for that naming in the future, especially considering they are planning on supporting the technology in Android 14, which I, again, assume will be able to read/write/shoot gain map images natively.

They also define the whole format as Ultra HDR as opposed to the generic Gain Map from Adobe... sooooo... naming is a bit of an Issue right now 😕

I decided to stick with the gain map terminology in regards to the technology but settled on JPEGR because, simply put, no-one else has given it a different name to it yet 😄

an alternative could be UltraHDRJPEGLoader maybe? but then if google pushes for another naming it could be confusing, I'm not sure yet on the matter, i'm open to suggestions and maybe @elalish can shed light of google's future naming plans (if he knows them or can ask around)

EDIT: another point to consider is that gain maps are can be embedded in avif, heic and jpegxl files, each one with a different method for embedding the gain map image as an addition to the base sdr representation, our library may support them in the future but we will need a different loader for each one (assuming browser support for them), so each loader naming will need to be set accordingly

@mrdoob
Copy link
Owner

mrdoob commented Nov 16, 2023

I remember @elalish saying that Ultra HDR actually has less range than .hdr.

To me, Ultra HDR sounds like a higher range than what we already had, so to avoid that confusion I thought just calling it HDR JPG was more clear.

@elalish
Copy link
Contributor

elalish commented Nov 16, 2023

I remember @elalish saying that Ultra HDR actually has less range than .hdr.

To me, Ultra HDR sounds like a higher range than what we already had, so to avoid that confusion I thought just calling it HDR JPG was more clear.

Well... I wasn't quite right. UltraHDR was designed primarily for TV HDR, which has less range than .hdr, but the actual format they settled on (this gain map stuff) is actually generic enough to do a good job on arbitrarily large HDR data. But I refuse to care about naming enough to debate it 😄

@daniele-pelagatti
Copy link
Contributor Author

okay then HDRJPGLoader it is, expect an update with the renaming soon

@Mugen87 Mugen87 merged commit 0582b00 into mrdoob:dev Nov 19, 2023
11 checks passed
@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 20, 2023

@daniele-pelagatti would you mind double-checking that textures assigned to material.envMap, scene.background, scene.environment, or pmremGenerator.fromEquirectangular(<texture>) all have a color space? It looks like some of these use NoColorSpace now, but should be LinearSRGBColorSpace. Results will look the same either way in this example, but NoColorSpace for these assignments will not work in future wide gamut color workflows. See:

It might just require changes upstream in gainmap-js, I don't see any issues in the PR.

@daniele-pelagatti
Copy link
Contributor Author

Sure, I'll look into it. I indeed use NoColorSPace upstream, I'll change it now

@mrdoob
Copy link
Owner

mrdoob commented Nov 20, 2023

@mrdoob mrdoob changed the title Examples: Add webgl_loader_gainmap Examples: Add webgl_loader_texture_hdrjpg Nov 20, 2023
@daniele-pelagatti
Copy link
Contributor Author

Okay, a new version of the package has been published with the requested fixes, the example needs to be updated to using version 2.0.6, shall I open another PR for this or you can change the importmap version yourselves?

@mrdoob
Copy link
Owner

mrdoob commented Nov 20, 2023

Done! c15506e

@elalish
Copy link
Contributor

elalish commented Nov 21, 2023

@daniele-pelagatti I've started integrating your library into <model-viewer>, but I've run into a couple of issues:

  1. Setting scene.environment = result.renderTarget.texture (the result of HDRJPGLoader) works great, PMREM generation is automatic and lovely. However, setting scene.background = result.renderTarget.texture places the texture as a backdrop quad rather than a cubemap like it does for the result of TextureLoader, even though I've set texture.mapping = EquirectangularReflectionMapping; for both. That kind of feels like a three.js bug - what do you think @mrdoob?

  2. I notice in HDRJPGLoader docs that it says to make a background, you need to use result.toDataTexture(), but this uses readPixels, which will be an extra slow and ideally unnecessary trip from GPU -> CPU -> GPU. Is this some kind of work around?

  3. If I give a normal, non-HDR jpeg to HDRJPGLoader, it fails because it can't find a gain map. Is there any easy way to tell if a given jpg is HDR or not? It would be really convenient if it could just fall back to TextureLoader if the jpeg doesn't have HDR. This would probably necessitate HDRJPGLoader having a consistent API with TextureLoader and just return a texture, which would hopefully be adequate if we can solve 1 without the need for 2.

@elalish
Copy link
Contributor

elalish commented Nov 21, 2023

I've tracked the difference with 1 down to this line - @Mugen87 any idea why you don't want a renderTarget texture to be able to become a cube map? Feels like an arbitrary distinction...

@daniele-pelagatti
Copy link
Contributor Author

daniele-pelagatti commented Nov 21, 2023

@elalish yes, all you have said for point 1 and 2 is true and I've found is linked to the line you mentioned in three.js during the coding of the example, that is why I added those warnings in the docs.

I think it has something to do with how an Equirectangular texture is automatically transformed into a proper CubeMap but I'm not sure why the transformation does not accept a rendertarget.texture in input, there must be a good reason because the line you mentioned is explicitly forbidding this.

If I give a normal, non-HDR jpeg to HDRJPGLoader, it fails because it can't find a gain map. Is there any easy way to tell if a given jpg is HDR or not? It would be really convenient if it could just fall back to TextureLoader if the jpeg doesn't have HDR. This would probably necessitate HDRJPGLoader having a consistent API with TextureLoader and just return a texture, which would hopefully be adequate if we can solve 1 without the need for 2.

this is an interesting use case I didn't think about.

The reason why the library returns the whole "QuadRenderer" (and not the rendertarget.texture) is because this way the user is able to change one parameter of the hdr reconstruction rendering: maxDisplayBoost (see here and search for "Max display boost") allows to change the HDR "intensity" and is part of the gain map proper specification.

By returning the renderer an user is able to do

const result = await hdrJpgLoader.loadAsync('image.jpg')
// do something here with the result

// update the gain map data
result.material.maxDisplayBoost = 1 // this would effectively render the sdr representation
result.render()
// do something with the updated result.renderTarget.texture 

to be honest I'm not entirely sure yet how much that will be useful in the future (especially in the context of three.js) but

  • given this is part of what makes a gain map a proper compliant-to-specs gain map
  • given the library aims to implement a javascript solution for encoding/decoding gain maps
  • given the spec specifically mentions the potential variability of maxDisplayBoost over time

I've decided to let the user experiment and decide by themselves what this feature (re-render on-the-fly) can or cannot do for them.

Now, to answer your question: I'm not sure how to approach it, loader.load() needs to immediately return something before starting loading (a texture for normal loaders, my QuadRenderer in our case) so returning the renderTarget.texure would cut out the ability to re-render the gain map.

How about QuadRenderer was augmented with

  • success: boolean signals the success the the reconstruction
  • fallback: Texture contains a texture loaded with a normal TextureLoader in case the gain map is not found
    this way an user could do
let myTexture
const result = await hdrJpgLoader.loadAsync('image.jpg')
if (result.success) {
  myTexture = result.renderTarget.texture
} else {
  myTexture = result.fallback
}

this way they don't waste time re-loading a texture when it does not contain a gain map? let me know if this would solve your use case.
Thanks for the feedback by the way! You've been very helpful so far, I appreciate the input

EDIT: side note, as a result of my decision to keep the "re-rendering" of the gain map available to the user, you must call result.renderTarget.dispose() and (if I remember correctly) result.material.dispose() yourself if you want to free up the resources created by the decoding process, i've left the decision of when to do that as well, kinda like wat happens with PMREMGenerator. result.dispose() is probably going to be deleted (or renamed) in the future because its name is misleading and does not effectively dispose the used resources (is is mostly used internally in the library)

@elalish
Copy link
Contributor

elalish commented Nov 21, 2023

Interesting - I hadn't considered the idea of maxDisplayBoost being dynamic, but I can see how that drives the API design. As usual, this could be cleaned up considerably if it were built into Three.js, but @mrdoob may want to wait until this is more settled and has more usage to take that step.

Possible alternative: what if HDRJPGLoader owned the maxDisplayBoost parameter, so if you wanted an update you could do

hdrjpgLoader.maxDisplayBoost = 1;
const myTexture = await hdrJpgLoader.loadAsync('image.jpg');

And you just had the loader cache the last image internally so that repeated calls are efficient? I suppose you'd still need a .dispose() method, but at least it would be one instead of two.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 22, 2023

I've tracked the difference with 1 down to this line - @Mugen87 any idea why you don't want a renderTarget texture to be able to become a cube map? Feels like an arbitrary distinction...

Um, I'm not sure anymore why this check is in place by I think it was PMREM related. Looking at the code now, it should be safe to remove it. The next if statement explicitly checks for EquirectangularReflectionMapping and EquirectangularRefractionMapping so WebGLCubeMaps should not accidentally try to convert a PMREM to a normal cube map.

@daniele-pelagatti
Copy link
Contributor Author

Possible alternative: what if HDRJPGLoader owned the maxDisplayBoost parameter, so if you wanted an update you could do
hdrjpgLoader.maxDisplayBoost = 1;
const myTexture = await hdrJpgLoader.loadAsync('image.jpg');
And you just had the loader cache the last image internally so that repeated calls are efficient? I suppose you'd still need a >.dispose() method, but at least it would be one instead of two.

I'm not convinced by this solution because when doing something like:

const loader = new HDRJPGLoader();
loader.maxDisplayBoost = 0;
const tex1 = loader.load('01.jpg');
loader.maxDisplayBoost = 16;
const tex2 = loader.load('02.jpg');

the loader would use the last value of maxDisplayBoost for both images if the first image was NOT loaded before the second maxDisplayBoost is set to 16.

I feel this would unnecessarily complicate my code in order to manage it and the first solution that comes to mind is doing something like

const loader = new HDRJPGLoader();
loader.setMaxDisplayBoost('01.jpg', 0);
const tex1 = loader.load('01.jpg');
loader.setMaxDisplayBoost('02.jpg, 16);
const tex2 = loader.load('02.jpg');

and so it would also need something like loader.dispose('01.jpg') in order to distinguish between those internally managed QuarRenderer instances, all of this is because the same loader can be used to load many images at the same time.

I think the api I proposed (with the addition of a single proper dispose method) is more straightforward? I'm not sure, let me know

Um, I'm not sure anymore why this check is in place by I think it was PMREM related. Looking at the code now, it should be safe to remove it. The next if statement explicitly checks for EquirectangularReflectionMapping and EquirectangularRefractionMapping so WebGLCubeMaps should not accidentally try to convert a PMREM to a normal cube map.

be careful because I've just tried to force a bypass of that line by passing a renderTarget.texture with rednertarget.texture.isRenderTargetTexture = false; and i've got this result

three.module.js:23377 THREE.WebGLState: TypeError: Failed to execute 'texSubImage2D' on 'WebGL2RenderingContext': Overload resolution failed.
    at Object.texSubImage2D (three.module.js:23373:21)
    at uploadTexture (three.module.js:24804:13)
    at WebGLTextures.setTexture2D (three.module.js:24177:5)
    at SingleUniform.setValueT1 [as setValue] (three.module.js:18538:11)
    at WebGLUniforms.upload (three.module.js:19093:7)
    at setProgram (three.module.js:30196:19)
    at WebGLRenderer.renderBufferDirect (three.module.js:28942:20)
    at renderObject (three.module.js:29736:11)
    at renderObjects (three.module.js:29705:6)
    at renderScene (three.module.js:29566:36)

@daniele-pelagatti
Copy link
Contributor Author

daniele-pelagatti commented Nov 22, 2023

@elalish

nevermind my previous proposals and mindstorming, I've just realized any proposed fallback would eventually fail under this scenario

pitfall of my proposal

const loader = new HDRJPGLoader()
const result = loader.load('gainmap.jpeg') // notice the usage of load, not loadAsync

// at this point "result" must be usable somewhere, even if not loaded

let texture: Texture
if (!result.success && result.fallback) { // will not work because the texture is not loaded yet
  texture = result.fallback
} else {
  texture = result.renderTarget.texture
}

pitfall of your proposal

const loader = new HDRJPGLoader()
loader.maxDisplayBoost = 1;
// in the following line, **before** knowing if the texture is a real gainmap or not
// I must return a texture, but it needs to be either a isRenderTargetTexture = true or false
// which i don't know beforehand
const myTexture = loader.load('image.jpg'); 

also cutting off the ability to re-render the gain map on the fly will lead to the problem of the second scenario, I'm not sure if this is solvable or not at this point, to be honest

EDIT: I just realized I can render the base jpeg without adding the gainmap data ( I'll need to do some tests first).

I think I'll return same renderTarget as before with the base rendition rendered inside (plus a console warning) in case the gain map is not found. This appears to be the only solution to our use case that keeps the return type of load() synchronous calls consistent and does not require me to know the outcome beforehand.

@daniele-pelagatti
Copy link
Contributor Author

@elalish I have a solution ready for both your use case and the dispose situation

  1. HDRJPGLoader will not change its behavior but will render the SDR image with a console.warn in case it is passed a normal JPG
  2. The resulting "QuadRenderer" returned from const result = loader.loadAsync('image.jpg') can be disposed with a single result.dispose(true/false)

here's the documentation for this method

Will dispose of all assets used by this gain map renderer.

@param disposeRenderTarget will dispose of the renderTarget which will not be usable later
set this to true if you passed the renderTarget.texture to a PMREMGenerator
or are otherwise done with it.

@example

const loader = new HDRJPGLoader(renderer)
const result = await loader.loadAsync('gainmap.jpeg')
const mesh = new Mesh(geometry, new MeshBasicMaterial({ map: result.renderTarget.texture }))
// DO NOT dispose the renderTarget here,
// it is used directly in the material
result.dispose()

@example

const loader = new HDRJPGLoader(renderer)
const pmremGenerator = new PMREMGenerator( renderer );
const result = await loader.loadAsync('gainmap.jpeg')
const envMap = pmremGenerator.fromEquirectangular(result.renderTarget.texture)
const mesh = new Mesh(geometry, new MeshStandardMaterial({ envMap }))
// renderTarget can be disposed here
// because it was used to generate a PMREM texture
result.dispose(true)

also, side note: from now on maybe it could be better to open issues directly in the gain map repository to avoid polluting this thread further? issue reports are welcome there 😄

@elalish
Copy link
Contributor

elalish commented Nov 22, 2023

This sounds like a good solution, thanks for thinking through the various ramifications. I'll test this today if you have pushed an npm release and try changing that one-liner in three.js to get rid of toDataTexture. If that works, I'll push a PR for that.

@elalish
Copy link
Contributor

elalish commented Nov 22, 2023

Okay, looks like that one-liner fixed the background problem. I verified it in model-viewer and cleaned up the example to remove toDataTexture: #27230

@daniele-pelagatti
Copy link
Contributor Author

@elalish release 2.0.7 has been published on NPM with those fixes, sorry yesterday was late for me :)

@marcofugaro marcofugaro mentioned this pull request Dec 27, 2023
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
* initial work for adding gainmap loader example

* added tags

* better example with file size and resolution comparison

* updated example to latest gainmap-js release 2.0.3 which fixes some compatibility problems in some browsers and platforms + adds progress handler for separate gainmap data

* should solve test failing problems

* updated loader name to HDRJPGLoader

* renamed vars for clarity

* some more renaming

* restored magFilter configuration for HDR
@Mugen87 Mugen87 mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants