1170462 - WebGL 2 - Finish implementing 3D Textures

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

There are a number of MOZ_CRASH("Not Implemented.") in dom/canvas/WebGL2ContextTextures.cpp:

void WebGL2Context::TexSubImage3D(...);
void WebGL2Context::CopyTexSubImage3D(...);
void WebGL2Context::CompressedTexImage3D(...);
void WebGL2Context::CompressedTexSubImage3D(...);

Comment 1

[Kyle Fung]

Attachment: implement-texSubImage3d.patch

Implemented a version of TexSubImage3D based off of an existing version.
I haven't been able to test it yet. Do you know if the Khronos conformance tests test this function?

Comment 2

[Kelsey Gilbert [:jgilbert]]

(In reply to kfung from comment #1)
> Created attachment 8635358 [details] [diff] [review]
> implement-texSubImage3d.patch
> 
> Implemented a version of TexSubImage3D based off of an existing version.
> I haven't been able to test it yet. Do you know if the Khronos conformance
> tests test this function?

You are the third person to be working on this simultaneously, I fear. :) We don't have a ton of tests for this stuff, so we're leaning pretty hard on code correctness and review.

Comment 3

[u480271]

Comment on attachment 8635358 [details] [diff] [review]
implement-texSubImage3d.patch

Review of attachment 8635358 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks to be on the right path. I'd like you to try using the webgl::FormatInfo for checking the effective internal formats.

::: dom/canvas/WebGL2ContextTextures.cpp
@@ +432,4 @@
>                               ErrorResult& rv)
>  {
> +    if (IsContextLost()) {
> +        return;

No {}'s for simple, single line if. ie:

if (IsContextLost())
    return;

@@ +444,5 @@
> +
> +    dom::Uint8ClampedArray arr;
> +    DebugOnly<bool> inited = arr.Init(imageData->GetDataObject());
> +    MOZ_ASSERT(inited);
> +    arr.ComputeLengthAndData();

Move these 4 lines down closer to where arr is used.

@@ +468,5 @@
> +    const WebGLTexture::ImageInfo& imageInfo = tex->ImageInfoAt(texImageTarget, level);
> +    const TexInternalFormat existingEffectiveInternalFormat = imageInfo.EffectiveInternalFormat();
> +    TexInternalFormat existingUnsizedInternalFormat = LOCAL_GL_NONE;
> +    TexType existingType = LOCAL_GL_NONE;
> +    UnsizedInternalFormatAndTypeFromEffectiveInternalFormat(existingEffectiveInternalFormat,

We should replace this function with the new webgl::FormatInfo from WebGLFormats.cpp

@@ +485,5 @@
> +        return ErrorInvalidOperation("texSubImage3D: type differs from that of the existing image");
> +    }
> +
> +    js::Scalar::Type jsArrayType = js::Scalar::MaxTypedArrayViewType;
> +    void* data = arr.Data();

ie. arr is used here.

@@ +496,5 @@
> +    const size_t bitsPerTexel = GetBitsPerTexel(existingEffectiveInternalFormat);
> +    MOZ_ASSERT((bitsPerTexel % 8) == 0); // should not have compressed formats here.
> +    size_t srcTexelSize = bitsPerTexel / 8;
> +
> +    if (width == 0 || height == 0) {

Can this be done earlier, or does that cause param checking failures? (ie. no error when one of the other params would cause a error to be returned)

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Dan Glastonbury :djg :kamidphish from comment #3)
> ...
> 
> ::: dom/canvas/WebGL2ContextTextures.cpp
> @@ +432,4 @@
> >                               ErrorResult& rv)
> >  {
> > +    if (IsContextLost()) {
> > +        return;
> 
> No {}'s for simple, single line if. ie:
> 
> if (IsContextLost())
>     return;
> 

Uhm, ignore that comment from Dan.  We do want {} even on single line conditionals, following our coding guidelines.  It's enough that we're randomly choosing 4 spaces instead of 2, may as well not introduce other differences into this code.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> (In reply to Dan Glastonbury :djg :kamidphish from comment #3)
> > ...
> > 
> > ::: dom/canvas/WebGL2ContextTextures.cpp
> > @@ +432,4 @@
> > >                               ErrorResult& rv)
> > >  {
> > > +    if (IsContextLost()) {
> > > +        return;
> > 
> > No {}'s for simple, single line if. ie:
> > 
> > if (IsContextLost())
> >     return;
> > 
> 
> Uhm, ignore that comment from Dan.  We do want {} even on single line
> conditionals, following our coding guidelines.  It's enough that we're
> randomly choosing 4 spaces instead of 2, may as well not introduce other
> differences into this code.

It would be a change from the module's prevailing style to require brackets for single-line returns following single-line conditionals. For at least `if () return` code, the single concern mentioned in the Mozilla style guide does not apply. Because this pattern is particularly common in our module (due to heavy validation), Dan and I decided that we shouldn't require brackets for this pattern.

Let's have a separate discussion on this if you want to talk about merging styles.

Comment 6

[u480271]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Uhm, ignore that comment from Dan.  We do want {} even on single line
> conditionals, following our coding guidelines.  It's enough that we're
> randomly choosing 4 spaces instead of 2, may as well not introduce other
> differences into this code.

Then the code doesn't match the rest of WebGL-style.

Also, 4 space is not randomly chosen. It was a conscious decision by the active maintainers of the code.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Is this done?

Comment 8

[Kelsey Gilbert [:jgilbert]]

Yes.
Fixed by bug 1221822.