Image and image filter / mipmapping

pjcozzi commented 11 years ago

I'll have a spec proposal soon, but we need to be careful not to decouple images and image filters because this is not supported in WebGL and OpenGL ES without duplicating the image, which is painful on the client.

The fix could be as simple as the spec disallowing multiple textures to be bound at the same time with different filters. Otherwise, this requires GL_ARB_sampler_objects.

I'll think about it a bit more before proposing something...

fabrobinet commented 11 years ago

Ok, I am curious why it shouldn't be handled with techniques/passes, I would prefer not adding too much concept especially if they can be handled generically.

RemiArnaud commented 11 years ago

What I was saying too is that the surface information is important for the run-time, but the image indirection not so much.

In COLLADA it is important, as it is designed to enable decoupling the image format from the format in the surface. For example the image may have a different size, texel format... But with a final format one can expect the images to have been pre-processed and correspond exactly to the format needed by the surface. So I think image can be removed from glTF and the reference to the data dIrectly stored in the surface.

pjcozzi commented 11 years ago

Currently in WebGL, the sampler state (filters and repeat) is part of the texture object (however, they are separate in OpenGL ES 3.0). This means that we can't bind a texture twice, each with a different sampler state, for the same draw call. To do so, we have to duplicate the texture since the sampler state is part of the texture.

Currently, glTF decouples the image and the sampler state, making it possible to declare two parameters used in the same technique that use the same image, but different sampler state. Although this is a rare case, it puts an implementation burden on the client, who will have to detect this and duplicate the texture to create a conformant implementation.

To solve this, all I suggest is wording in the spec that explicitly disallows this for the WebGL 1.0.x profile. Assuming WebGL 2.0 adds sampler objects, the WebGL 2.0 profile would lift this restriction.

RemiArnaud commented 11 years ago

yes, but also have the converted to duplicate automatically the texture.

On Wed, Apr 17, 2013 at 12:51 PM, Patrick Cozzi notifications@github.comwrote:

Currently in WebGL, the sampler state (filters and repeat) is part of the texture object (however, they are separate in OpenGL ES 3.0). This means that we can't bind a texture twice, each with a different sampler state, for the same draw call. To do so, we have to duplicate the texture since the sampler state is part of the texture.

Currently, glTF decouples the image and the sampler state, making it possible to declare two parameters used in the same technique that use the same image, but different sampler state. Although this is a rare case, it puts an implementation burden on the client, who will have to detect this and duplicate the texture to create a conformant implementation.

To solve this, all I suggest is wording in the spec that explicitly disallows this for the WebGL 1.0.x profile. Assuming WebGL 2.0 adds sampler objects, the WebGL 2.0 profile would lift this restriction.

— Reply to this email directly or view it on GitHubhttps://github.com/KhronosGroup/glTF/issues/40#issuecomment-16531633 .

fabrobinet commented 11 years ago

@pjcozzi ok, in my first comment I misunderstood what you meant here...

fabrobinet commented 11 years ago

@pjcozzi weither you have or not sampler objects, you do not need to duplicate the texture you can change the filter and wrap mode for given texture on the fly OR duplicate the texture. It's a trade off & decision that has to be made on client side. It's easy to detect that different samplers are used for the same texture. In some situation when you have enormous images it's better to actually change the wrap mode, even if this happens each frames, but if you have reasonnably small images duplicating might be cleaner... It looks that you avoid the possibility of changing the wrap/filter mode on the fly but this does work (and even better with sampler objects)

pjcozzi commented 11 years ago

AFAIK in WebGL 1.0, If you have one texture bound to two different texture units used in the same draw call, they must use the same sampler state because the sampler state is part of the texture.

This is one reason why GL_ARB_sampler_objects was introduced.

fabrobinet commented 11 years ago

Ah yes, you're right. But I overlooked something else in my previous comment. Different texture parameters for the shader can just point to the same image and have different wrap/filter mode. Consequently, clients may then share the same image but create 2 different textures.

pjcozzi commented 11 years ago

Consequently, clients may then share the same image but create 2 different textures.

Yes, this is exactly what I am saying, but we should make it easy for clients. This is a pain to have to implement; we've done it before for COLLADA. I suggest just explicitly disallowing this in the WebGL 1.0.x profile of glTF. I don't know of any models that do this in practice, so I don't see enough value in making clients think they have to support this. When GL_ARB_sampler_objects are supported, we can support this in a later profile.

fabrobinet commented 11 years ago

The decision of duplicating or not the image is on the client side, and I don't think it is a pain to implement. You can just create a hascode with the sampler infos. If a client really wants to prevent re-applying wrap / filter mode then we probably need a textureID (as Remi suggested), and then we could decide in the converter if textures needs to be duplicated or not. Though, I am not sure yet how the schema will look...

RemiArnaud commented 11 years ago

following the email discussion. The 'images' indirection between an image ID and its path is not useful. In fact an image unique ID is better give by its path than by any other ID.

More to that point, I ran into a problem. Loading one glTF, and then another one. They had the same imageID, pointing to a different image. not using imageID and using the image path only fixed the problem immediately.

So I think that the spec should be changed to:

remove the 'images'. use the path directly when defining the texture
replace this indirection with a 'samplers' where the texture information is described. the material is very simplified now since all materials parameters contain 'type' and 'value'. there is no more special case for textures

before:

 "material.6": {
            "name": "artezin_bottle",
            "technique": "technique1",
            "techniques": {
                "technique1": {
                    "parameters": {
                        "ambient": {
                            "type": "FLOAT_VEC3",
                            "value": [
                                0.2,
                                0.2,
                                0.2
                            ]
                        },
                        "diffuse": {
                            "image": "image_0",
                            "magFilter": "LINEAR",
                            "minFilter": "LINEAR",
                            "type": "SAMPLER_2D",
                            "wrapS": "REPEAT",
                            "wrapT": "REPEAT"
                        },
                        "shininess": {
                            "type": "FLOAT",
                            "value": -1
                        }
                    }
                }
            }
        }
....
 "images": {
        "image_0": {
            "path": "artezin_bottle.jpg"
        },

after:

 "material.6": {
            "name": "artezin_bottle",
            "technique": "technique1",
            "techniques": {
                "technique1": {
                    "parameters": {
                        "ambient": {
                            "type": "FLOAT_VEC3",
                            "value": [
                                0.2,
                                0.2,
                                0.2
                            ]
                        },
                        "diffuse": {
                            "type": "SAMPLER_2D",
                            "value": "texture_0"
                        },
                        "shininess": {
                            "type": "FLOAT",
                            "value": -1
                        }
                    }
                }
            }
        }
....
 "textures": {
        "texture_0": {
            "path": "artezin_bottle.jpg"
            "magFilter": "LINEAR",
            "minFilter": "LINEAR",
            "wrapS": "REPEAT",
            "wrapT": "REPEAT"
        },

fabrobinet commented 11 years ago

Thanks Remi.

I like most of your proposal. I agree for the textures, it's probably good to unify uniform of type sampler the way you suggest. There was also that thread we had about surface that I need to re-check, but this looks mostly good to me.

About the image indirection I have to disagree:

we envisioned that images could also be concatenated in a single blob, I don't see how to handle this without the indirection.
the fact that the same id can come up when you load another file is common for all resources (same for meshes...) so the "problem" cannot be resolved by putting the datas straight in all objects. Instead you should clear your map of images when you load a new scene and that should fix your problem, and you don't want to accumulate all these textures anyway. I.e I believe it's more a problem with your code than with the spec about images.
As an aside, the image (within texture) property will drop eventually to something more generic like "source" , it will still be an indirection and it will point to either a video id or an image id.

tparisi commented 11 years ago

I agree with @fabricerobinet about image ID "leakage" between scenes: it seems more like an implementation issue than a spec issue.

We should definitely allow for images to be packed ("concatenated"), this is a very important high-performance web technique, not to mention a long-established graphics programming trick.

Tony

On Fri, May 10, 2013 at 9:16 AM, Fabrice Robinet notifications@github.comwrote:

Thanks Remi.

I like most of your proposal. I agree for the textures, it's probably good to unify uniform of type sampler the way you suggest. There was also that thread we had about surface that I need to re-check, but this looks mostly good to me.

About the image indirection I have to disagree:

we envisioned that images could also be concatenated in a single blob, I don't see how to handle this without the indirection.

the fact that the same id can come up when you load another file is common for all resources (same for vertices...) so the "problem" cannot be resolved by putting the datas straight in all objects. Instead you should clear your map of images when you load a new scene and that should fix your problem, and you don't want to accumulate all these textures anyway. I.e I believe it's more a problem with your code than with the spec about images.

As an aside, the image property will drop eventually to something more generic like "source" , it will still be an indirection and it will point to either a video id or an image id.

— Reply to this email directly or view it on GitHubhttps://github.com/KhronosGroup/glTF/issues/40#issuecomment-17729611 .

Tony Parisi tparisi@gmail.com CTO at Large 415.902.8002 Skype auradeluxe Follow me on Twitter! http://twitter.com/auradeluxe Read my blog at http://www.tonyparisi.com/ Learn WebGL http://learningwebgl.com/

Read my book! WebGL, Up and Running http://shop.oreilly.com/product/0636920024729.do http://www.amazon.com/dp/144932357X

fabrobinet commented 11 years ago

@RemiArnaud @pjcozzi @tparisi I can move forward on the converter about this. To sum up: The points where we agreed here is to have textures being a root property so that we can share them and also unify parameters and have parameter.value point to a texture. This way all parameters have same properties...cool.

I explained above why we should keep the image property, it should be seen more as a way to share images client side than an indirection (among other reasons above).. But eventually, texture should probably point to a "source" property that could be either an "image" or a "video" (still to be introduced in the spec). We will discuss this in another issue. (about source for media...).

@pjcozzi can you remind me what was your proposal ? I see you prefer to not allow to specify different filter for a given texture and the updated proposal does not (not easily, but still possible if developer really want to). Looks like an alternativ to be more generic would be to create yet another object to hold filter and wrap mode but I would hold off on that for now.

pjcozzi commented 11 years ago

I agree with @fabricerobinet about image ID "leakage" between scenes: it seems more like an implementation issue than a spec issue. We should definitely allow for images to be packed ("concatenated"), this is a very important high-performance web technique, not to mention a long-established graphics programming trick. Tony

:+1:

@pjcozzi can you remind me what was your proposal ? I see you prefer to not allow to specify different filter for a given texture and the updated proposal does not (not easily, but still possible if developer really want to).

Currently in WebGL, the sampler state (filters and repeat) is part of the texture object (however, they are separate in OpenGL ES 3.0). This means that we can't bind a texture twice, each with a different sampler state, for the same draw call. To do so, we have to duplicate the texture since the sampler state is part of the texture.

Currently, glTF decouples the image and the sampler state, making it possible to declare two parameters used in the same technique that use the same image, but different sampler state. Although this is a rare case, it puts an implementation burden on the client, who will have to detect this and duplicate the texture to create a conformant implementation.

To solve this, all I suggest is wording in the spec that explicitly disallows this for the WebGL 1.0.x profile. Assuming WebGL 2.0 adds sampler objects, the WebGL 2.0 profile would lift this restriction.

Looks like an alternativ to be more generic would be to create yet another object to hold filter and wrap mode but I would hold off on that for now.

Later, we would do this to mirror sampler_objects, which are in ES 3.0.

fabrobinet commented 11 years ago

@pjcozzi actually, I was referring to the latest iteration based on Remi's feedback to introduce texture. As discussed above it would unify parameters. Then having the sampler right in the texture object solve the problem of having different samplers per texture.... but then, It will be harder to support sampler objects eventually. So maybe to make things right from the beginning we should add samplers (yet another property :()) and for WebGL 1.0 the file would be invalid (due to consistency check) if 2 samplers refer the same texture.

The other change a bit on side of this that I would like to introduce is to use source in texture instead of image, so that we could refer to either an image or video.

fabrobinet commented 11 years ago

Just spent some time trying to put all this in JSON... One major annoyance is to handle target and levels for textures. In the following proposal default is for the base level (0). It shows:

a 2d texture that refers to an image
a texture cube for which sources refer to the 6 images in the cube map. The proposed order should be specified in glTF SPEC.
a blank texture, to be used for render target purpose.
note: samplers are included to get this example to make it more complete, but there are not detailed.

samplers" : {
  "sampler0" : {
    "minFilter" : ..
    "magFilter" : ..
    "wrapS" : ..
    "wrapT" : ..
  }
},

"textures" : {
  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "source" : "image_0",
    "sampler" : "sampler0"
  },
  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "sources" ["img1_negX","img1_posX","img1_negY","img1_posY","img1_negZ","img1_posZ"],
    "sampler" : "sampler1"
  },
  "texture2" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA",
    "width": 512,
    "height": 512,
    "sampler" : "sampler0"
  },
}

but... then what if someone wants to pass manually the mip levels?, I propose this: note2: level is implicit starts at 0 and is increment for each entries in nextLevels

samplers" : {
  "sampler0" : {
    "minFilter" : ..
    "magFilter" : ..
    "wrapS" : ..
    "wrapT" : ..
  }
},

"textures" : {
  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA",  
    "source" : "image_0",
    "nextLevels" : [ { source: "image_1_level1" }, { source: ""image_1_level2"}]
    "sampler" : "sampler0"
  },
  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA",  
    "sources" ["img1_negX","img1_posX","img1_negY","img1_posY","img1_negZ","img1_posZ"],
        "nextLevels" : [ { sources : ["image_1_level1",...] }, { sources: ["image_1_level2",..]}]
    "sampler" : "sampler1"
  },
  "texture2" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA",
    "width": 512,
    "height": 512,
    "sampler" : "sampler0"
  },
}

pjcozzi commented 11 years ago

I really like this direction. For specifying the mip levels:

Why the name nextLevels? Why not mipLevels?

Because we describe level 0 as the base level. miplevels would work too but it might be confusing if it includes every level but the base level.

Or even if mipLevels is provided then it can be all mip levels > and source/sources is not required.

having sources are needed regardless of the levels, they are required for texture cube, where you need 6 images even if you specify just one level for each of them.

Do we require that all mip levels be provided? I can look more carefully at the WebGL spec to see its requirements, > but since we can't render to a non-zero mip level in WebGL AFAIK I suspect we would want all mip levels.

Yes, If you provide mip levels, you need to provide them all till you reach 1x1 size otherwise the mip map chain will be considered incomplete.

How do we specify for the app to generate the levels ala generateMipmap?

I would that if filtering specify mip mapping AND there is no nextLevels or mipLevels (whatever we decide to call it), then it means that generate mipmap should be called by the client.

pjcozzi commented 11 years ago

Concretely, I'm proposing something like this:

2D texture. No mipmapping:

  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "source" : "image_0",
    "sampler" : "sampler0"
  }

2D texture. Mipmapping:

  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA",  
    "mipLevels" : [ "image_0_level0", "image_0_level1", "image_0_level2" ],
    "sampler" : "sampler0"
  }

2D texture. Client-side generated mipmaps:

  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "source" : "image_0",
    "sampler" : "sampler0",
    "generateMipmap" : true
  }

As for cube maps...

Cube map. No mipmapping.

  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "sources" ["img1_negX","img1_posX","img1_negY","img1_posY","img1_negZ","img1_posZ"],
    "sampler" : "sampler1"
  },

Cube map. Mipmapping.

  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "mipLevels" [["img1_negX_level0", "img1_negX_level1", "img1_negX_level2"],["img1_posX_level0", ...],["img1_negY_level0", ...],["img1_posY_level0", ...],["img1_negZ_level0", ...],["img1_posZ_level0", ...]],
    "sampler" : "sampler1"
  },

Cube map. Client-side mipmaps.

  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "sources" ["img1_negX","img1_posX","img1_negY","img1_posY","img1_negZ","img1_posZ"],
    "sampler" : "sampler1",
    "generateMipmap" : true
  },

My only questions here are:

This is not flexible enough to (client-side) generate mipmaps for some cube map faces and not others. I don't think this matters in practice.
I made mipLevels an array of array of strings - as opposed to an array of array of objects with source properties. I think this is cleaner, but was there a good reason to use an object instead of a string originally?

fabrobinet commented 11 years ago

Thanks for putting this together Patrick.

I have 2 issues:

I find confusing the switch from source to mipLevels. (more comments about it in last question answered below)
based on my previous input I do not see why we need to specify generateMipMap , if mipLevels is not specified and a filter mode in the sampler specify mip mapping, then generateMipMap map should be called, simple.

This is not flexible enough to (client-side) generate mipmaps for some cube map faces and not others. I don't think >this matters in practice.

Same here, I agree with this trade-off

I made mipLevels an array of array of strings - as opposed to an array of array of objects with source properties. I >think this is cleaner, but was there a good reason to use an object instead of a string originally?

I was not particularly happy to have an object but removing source is a short cut, and is confusing because what you refer to is source per level and thus having an object is IMO more appropriate. This short-cut imply that mipLevel == source which is wrong.

pjcozzi commented 11 years ago

based on my previous input I do not see why we need to specify generateMipMap , if mipLevels is not specified and a filter mode in the sampler specify mip mapping, then generateMipMap map should be called, simple.

OK with me since it is minimal and complete, but this is likely to be a source of bugs. Users will just use the sampler without thinking to generate mipmaps. GL developers already do it all the time, and now we are making it really easy for them.

We need to document it prominently.

I find confusing the switch from source to mipLevels. (more comments about it in last question answered below)

Here's the code for it:

if (typeof texture.source !== 'undefined) {
// call texImage2D with level 0
} else {
    for each texture.mipLevels {
        // call texImage2D with each level
    }
}

Compared to the original proposal:

// call texImage2D with level 0
if (typeof texture.nextLevels === 'undefined) {
    for each texture.nextLevels {
        // call texImage2D with level
    }
}

The original is not as bad as I thought. Maybe rename nextLevels to additionalLevels?

As for string vs. object, if source is a string, and nextLevels is an array of sources, then why can't it be an array of strings?

fabrobinet commented 11 years ago

As for string vs. object, if source is a string, and nextLevels is an array of sources, then why can't it be an array of > strings?

If we keep source in the base level which make it complete and handle the remaining levels in additionalLevels it will be fine with me. My biggest concern was to "jump" from source to `xxxLevels" for the base level depending if we have multiple levels or not...

So I could live with additionalLevels being strings but this will bring limitations as in reality a level is actually not just a single string. I will give a pathological example to show why in theory this is wrong, but I admit this is far from being the typical use case: What if a user decide to use glTF to just initializes the size (width and height) of each additionalLevel ? (so that he can feed the mip maps individually at runtime with say some CPU generated content..)

Remember what a level is, in the base level ( == texture [*]) it's not just a source, if you do not specify a source you could specify a width and height, hence the need for an object in this case. This said, I admit the additionalLevels are a simpler type than the base type because internalFormat and format should not be respecified.

My point is to show that level != source and source is a sub part of level.

[*] I did this simplification to avoid an indirection right under texture, as in most case we'll just deal with the base level.

@pjcozzi Does this make more clear my concern ?

pjcozzi commented 11 years ago

@fabrobinet keeping it as an object is fine with me; I'm just trying to avoid lots of indirection as we've grown glTF into more levels of indirection that I expected and that our original charter called for. However, so far, it hasn't been a pain to implement.

Leave the object or leave the string and a later glTF version can allow either a string or an object with multiple properties.

fabrobinet commented 11 years ago

Thank you, I will implement this right after the skinning...

fabrobinet commented 11 years ago

done f4d8f40239b1c9e794cd9d5c7b4c47638f05022c . Removing the converter keyword then.

fabrobinet commented 10 years ago

flagging as resolved.

fabrobinet commented 10 years ago

The WG agreed that providing images of mipmaps wasn't critical for V1.0 and that just specifying the flag for mipmapping generation would be enough. Watchers of the project are welcome to provide feedback about this decision.

pjcozzi commented 9 years ago

@tparisi for the draft 1.0 spec add:

Mipmapping Implementation Note: When a sampler's minification filter (sampler.minFilter) uses mipmapping (NEAREST_MIPMAP_NEAREST, NEAREST_MIPMAP_LINEAR, LINEAR_MIPMAP_NEAREST, or LINEAR_MIPMAP_LINEAR), any texture referencing the sampler needs to have mipmaps, e.g., by calling GL's generateMipmap function.

Non-Power-Of-Two Texture Implementation Note: glTF does not guarantee that a texture's dimensions are a power-of-two. At runtime, if a texture's width or height is not a power-of-two, the texture needs to be resized so its dimensions are powers-of-two if the sampler the texture references:

Has wrapping mode (either sampler.wrapS or sampler.wrapT) equal to REPEAT or MIRRORED_REPEAT, or

Has a minification filter (sampler.minFilter) that uses mipmapping (NEAREST_MIPMAP_NEAREST, NEAREST_MIPMAP_LINEAR, LINEAR_MIPMAP_NEAREST, or LINEAR_MIPMAP_LINEAR).

@tparisi if you want to see the code in Cesium, see https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Model.js#L1476

KhronosGroup / glTF

Image and image filter / mipmapping #40