Closed Bug 1075305 Opened 10 years ago Closed 10 years ago

WebGL2 - Track GL internalformat in WebGLTexture::ImageInfo

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: u480271, Assigned: u480271)

References

Details

Attachments

(4 files, 3 obsolete files)

Track GL internalformat in WebGLTexture::ImageInfo
Attached patch WebGL2 - 3D Textures (obsolete) — Splinter Review
Attachment #8497939 - Attachment is obsolete: true
Summary: WebGL2 - 3D Textures → WebGL2 - Track GL internalformat in WebGLTexture::ImageInfo
Add internal format tracking WebGLTexture::ImageInfo.  This is a step
on the path to sorting out the confusion.  Once we have "effective
internal format" tracking, format and type can be removed.
Attachment #8497945 - Flags: review?(bjacob)
remove WebGL prefix from internalformat, format and type.  It doesn't add anything but verbosity.
Attachment #8497947 - Flags: review?(bjacob)
Attachment #8497949 - Flags: review?(bjacob)
Blocks: 1072680
Comment on attachment 8497945 [details] [diff] [review]
WebGL2 - Add internal format state to WebGLTexture::ImageInfo, in preparation for WebGL2 texture functions.

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

::: dom/canvas/WebGLTexture.h
@@ +75,5 @@
>  
>          ImageInfo(GLsizei width,
>                    GLsizei height,
> +                  TexInternalFormat webGLInternalFormat,
> +                  TexFormat webGLFormat,

There's a contradiction between the title of this patch, and what it's doing here.

As we can see here, we already have

    TexInternalFormat webGLFormat,

indeed webGLFormat was converted to type TexInternalFormat in bug 1071340, a week ago, which you reviewed!

So WebGLTexture::ImageInfo is _already_ storing a TexInternalFormat.

There were a few instances left where we still confused that 'TexInternalFormat webGLFormat' for a non-internal format, and they are taken care of in bug 1072680.

Why would you want to add a TexFormat field there? Non-internal texture formats are only used to describe incoming texture data in entry points such as texImage2D. Here, we are describing the actual format of the already-unpacked texture.
Attachment #8497945 - Flags: review?(bjacob) → review-
As noted in bug 1072680 comment 8, it is unfortunate that the TexInternalFormat field is currently named "webGLFormat" suggesting that it's not an internalformat, but it really is an internalformat. A patch to just rename it to webGLInternalFormat would be r+.
Attachment #8497947 - Flags: review?(bjacob) → review+
Attachment #8497949 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #6)
> As noted in bug 1072680 comment 8, it is unfortunate that the
> TexInternalFormat field is currently named "webGLFormat" suggesting that
> it's not an internalformat, but it really is an internalformat. A patch to
> just rename it to webGLInternalFormat would be r+.

This is what I really want to do. I'll update the patch to rename format to internalformat.
Attachment #8497945 - Attachment is obsolete: true
Carry r=bjacob.
Attachment #8498643 - Flags: review?(bjacob)
Attachment #8497947 - Attachment is obsolete: true
Attachment #8498643 - Flags: review?(bjacob) → review+
Attachment #8498713 - Flags: review?(bjacob)
Comment on attachment 8498713 [details] [diff] [review]
Sort strong GL type TexFormat.

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

Quite good idea, to put the numeric values in comments!

I had been using this program to do the sorting:

#include <iostream>
#include <algorithm>
#include <vector>
#include "GLDefs.h"

struct Val {
    int value;
    const char* name;

    Val(int v, const char* n) : value(v), name(n) {}

    bool operator<(const Val& other) const { return value < other.value; }
    bool operator==(const Val& other) const { return value == other.value; }
};

int main() {

    std::vector<Val> values = {

#define IGNORE //
#define STRONG_GLENUM_VALUE(X) {LOCAL_GL_##X, #X}

    STRONG_GLENUM_VALUE(RGB),
    // pasted values to be sorted here

#undef STRONG_GLENUM_VALUE

    };

    std::sort(values.begin(), values.end());

    for (auto i = values.begin(); i != values.end(); i++) {
        std::cout << "    STRONG_GLENUM_VALUE(" << i->name << ")," << std::endl;
    }
};
Attachment #8498713 - Flags: review?(bjacob) → review+
Comment on attachment 8498641 [details] [diff] [review]
Rename WebGLTexture::ImageInfo::WebGLFormat to WebGLTexture::ImageInfo::WebGLInternalFormat.

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

OK to land this, but this is going to be changed again with the effective-internalformats change.
Attachment #8498641 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #15)
> Comment on attachment 8498641 [details] [diff] [review]
> Rename WebGLTexture::ImageInfo::WebGLFormat to
> WebGLTexture::ImageInfo::WebGLInternalFormat.
> 
> Review of attachment 8498641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK to land this, but this is going to be changed again with the
> effective-internalformats change.

I understand and are in agreement with you.
(In reply to Dan Glastonbury :djg :kamidphish from comment #12)
> https://tbpl.mozilla.org/?tree=Try&rev=a5f47c3f198b

TexFormat strong type isn't sorted. Added patch.
You need to log in before you can comment on or make changes to this bug.