1093439 - Use format info tables to handle format queries

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Rebuild-format-code-with-tables.patch

ANGLE and Mesa use something like this. Instead of juggling format types around, and implementing `DoesFormatHaveDepth(format)` with a big switch, just have a central format query API.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Supplant-IsGLDepth-Stencil-Format-with-FormatAuthori.patch

Comment 2

[u480271]

Comment on attachment 8516428 [details] [diff] [review]
0001-Rebuild-format-code-with-tables.patch

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

LGTM.

::: dom/canvas/WebGLFormats.cpp
@@ +105,5 @@
> +static void
> +InitSizedFormats()
> +{
> +    // "Texture and renderbuffer color formats"
> +    AddSizedFormat(LOCAL_GL_RGBA32I,      EffectiveFormat::RGBA32I);

Potential you could macro this and use a fruit list:

    ADD_SIZED_FORMAT(RGBA32I);
    ...

@@ +245,5 @@
> +    default:
> +        MOZ_CRASH("Missing UnsizedFormat case.");
> +    }
> +
> +    EffectiveFormatInfo info = {unsizedFormat, colorComponentType, hasAlpha,

Space after { and before }

@@ +338,5 @@
> +    ret->AddEffectiveFormat(EffectiveFormat::RGBA32I,            UnsizedFormat::RGBA, ComponentType::Int,          true,  false, true );
> +    ret->AddEffectiveFormat(EffectiveFormat::RGBA32UI,           UnsizedFormat::RGBA, ComponentType::UInt,         true,  false, true );
> +
> +    // GLES 3.0.4, p133, table 3.14
> +    ret->AddEffectiveFormat(EffectiveFormat::DEPTH_COMPONENT16,  UnsizedFormat::D,  ComponentType::None,           true,  false, true );

Does this really have a none component type and not uint16?

::: dom/canvas/WebGLFormats.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace webgl {
> +
> +typedef uint8_t EffectiveFormatUIntT;

But it's a UInt8T? When I see UIntT, I think 32-bit values but it's stored as 8-bits. Does that matter?

@@ +105,5 @@
> +    LA,
> +    L,
> +    A,
> +    SRGB,
> +    SRGB_A,

Are these any different to RGB and RGBA? Ie. Do you use those values to signify something?

@@ +136,5 @@
> +    bool isRenderable;
> +};
> +
> +bool EffectiveFormatFromFormatAndType(GLenum format, GLenum type,
> +                                      EffectiveFormat* const out);

Is const necessary here, or are you trying out a new style?

@@ +163,5 @@
> +        return const_cast<FormatAuthority*>(this)->MutableInfo(format);
> +    }
> +};
> +
> +/*

Is this here for later?

Comment 3

[u480271]

Comment on attachment 8516429 [details] [diff] [review]
0002-Supplant-IsGLDepth-Stencil-Format-with-FormatAuthori.patch

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

::: dom/canvas/WebGLContext.cpp
@@ +319,5 @@
>      mDisableFragHighP = false;
>  
>      mDrawCallsSinceLastFlush = 0;
> +
> +    mFormatAuthority = Move(webgl::FormatAuthority::CreateForGLES2());

Should this be in WebGL1Context? WebGL1Context and WebGL2Context both inherit WebGLContext, so in WebGL2 mFormatAuthority will be initialized with GLES2 and then GLES3.

::: dom/canvas/WebGLContext.h
@@ +1437,5 @@
>  
> +    UniquePtr<webgl::FormatAuthority> mFormatAuthority;
> +
> +    const webgl::EffectiveFormatInfo&
> +    FormatInfo(webgl::EffectiveFormat format) const {

Is there a performance gain from having this in the header?

Comment 4

[u480271]

Wasn't sure to r+ or r- for the WebGL1Context questions, so I just published the comments.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #2)
> Comment on attachment 8516428 [details] [diff] [review]
> 0001-Rebuild-format-code-with-tables.patch
> 
> Review of attachment 8516428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: dom/canvas/WebGLFormats.cpp
> @@ +105,5 @@
> > +static void
> > +InitSizedFormats()
> > +{
> > +    // "Texture and renderbuffer color formats"
> > +    AddSizedFormat(LOCAL_GL_RGBA32I,      EffectiveFormat::RGBA32I);
> 
> Potential you could macro this and use a fruit list:
> 
>     ADD_SIZED_FORMAT(RGBA32I);
>     ...
We could, yeah. It's nice not to have to juggle macros, for once.

> @@ +245,5 @@
> > +    default:
> > +        MOZ_CRASH("Missing UnsizedFormat case.");
> > +    }
> > +
> > +    EffectiveFormatInfo info = {unsizedFormat, colorComponentType, hasAlpha,
> 
> Space after { and before }
Ok, I have no real preference here.

> 
> @@ +338,5 @@
> > +    ret->AddEffectiveFormat(EffectiveFormat::RGBA32I,            UnsizedFormat::RGBA, ComponentType::Int,          true,  false, true );
> > +    ret->AddEffectiveFormat(EffectiveFormat::RGBA32UI,           UnsizedFormat::RGBA, ComponentType::UInt,         true,  false, true );
> > +
> > +    // GLES 3.0.4, p133, table 3.14
> > +    ret->AddEffectiveFormat(EffectiveFormat::DEPTH_COMPONENT16,  UnsizedFormat::D,  ComponentType::None,           true,  false, true );
> 
> Does this really have a none component type and not uint16?
The callee's argument there is `colorComponentType`. This avoids problems like what to put for D24S8, and component type for non-colors isn't relevant yet, that I've seen. Even D16 isn't *really* uint16, or even really unorm16.

> 
> ::: dom/canvas/WebGLFormats.h
> @@ +12,5 @@
> > +
> > +namespace mozilla {
> > +namespace webgl {
> > +
> > +typedef uint8_t EffectiveFormatUIntT;
> 
> But it's a UInt8T? When I see UIntT, I think 32-bit values but it's stored
> as 8-bits. Does that matter?
I think EffectiveFormatValueT is probably better. Really I just want it to be something I can use below to establish a map, etc.
> 
> @@ +105,5 @@
> > +    LA,
> > +    L,
> > +    A,
> > +    SRGB,
> > +    SRGB_A,
> 
> Are these any different to RGB and RGBA? Ie. Do you use those values to
> signify something?
They have an extra pack/unpack step, so I've kept them separate.
> 
> @@ +136,5 @@
> > +    bool isRenderable;
> > +};
> > +
> > +bool EffectiveFormatFromFormatAndType(GLenum format, GLenum type,
> > +                                      EffectiveFormat* const out);
> 
> Is const necessary here, or are you trying out a new style?
It's not necessary, though making `foo* out` `foo* const out` prevents things like `out = bar`.
It's rather vanishingly useful, but it's nice to prevent `out = 0`, which IIRC is valid otherwise.

I wish arguments were const by default.

> 
> @@ +163,5 @@
> > +        return const_cast<FormatAuthority*>(this)->MutableInfo(format);
> > +    }
> > +};
> > +
> > +/*
> 
> Is this here for later?
Yeah, but I haven't needed it yet, so it should be taken out.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8516429 [details] [diff] [review]
0002-Supplant-IsGLDepth-Stencil-Format-with-FormatAuthori.patch

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

::: dom/canvas/WebGLContext.cpp
@@ +319,5 @@
>      mDisableFragHighP = false;
>  
>      mDrawCallsSinceLastFlush = 0;
> +
> +    mFormatAuthority = Move(webgl::FormatAuthority::CreateForGLES2());

Sure, let's move it.

::: dom/canvas/WebGLContext.h
@@ +1437,5 @@
>  
> +    UniquePtr<webgl::FormatAuthority> mFormatAuthority;
> +
> +    const webgl::EffectiveFormatInfo&
> +    FormatInfo(webgl::EffectiveFormat format) const {

Yes, since we're immediately calling (another inlined function, which immediately calls) a non-inlined function.