Closed Bug 1080957 Opened 10 years ago Closed 10 years ago

WebGL2: support TEXTURE_BASE_LEVEL and TEXTURE_MAX_LEVEL

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch base-max-mipmap-levels.patch (obsolete) — Splinter Review
WebGL2 allows restricting which levels of a texture are used for determining mipmap-completeness. (Patch is untested.)
Attached patch bug1080957-mipmap-levels.patch (obsolete) — Splinter Review
The bug I'd mentioned turned out to be the fake black status being set incorrectly because level 0 was unset. I changed that check to be GetBaseMipmapLevle() though I'm not sure if that's always okay.

Conformance spec PR is here: https://github.com/KhronosGroup/WebGL/pull/746
Attachment #8502936 - Attachment is obsolete: true
Attachment #8503486 - Flags: review?(jgilbert)
Comment on attachment 8503486 [details] [diff] [review]
bug1080957-mipmap-levels.patch

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1498,5 @@
> +            }
> +            if (pname == LOCAL_GL_TEXTURE_BASE_LEVEL)
> +                tex->SetBaseMipmapLevel(intParam);
> +            else
> +                tex->SetMaxMipmapLevel(intParam);

It looks like you don't call through to GL here.

::: dom/canvas/WebGLTexture.cpp
@@ +67,5 @@
>          if (mHaveGeneratedMipmap) {
>              // Each mipmap level is 1/4 the size of the previous level
>              // 1 + x + x^2 + ... = 1/(1-x)
>              // for x = 1/4, we get 1/(1-1/4) = 4/3
> +            result += ImageInfoAtFace(face, GetBaseMipmapLevel()).MemoryUsage() * 4 / 3;

I don't know as this is true.
I don't think GenerateMipmaps deallocs levels <base.

We could have a case like this:
texImage2D(level=0, w=h=10000)
texImage2D(level=1, w=h=100)
texParam(MIP_BASE, 1)
generateMipmaps()

@@ +221,1 @@
>          return false;

Check that Base is <= Max.

::: dom/canvas/WebGLTexture.h
@@ +277,5 @@
> +        return std::min(mBaseMipmapLevel, mMaxLevelWithCustomImages);
> +    }
> +    unsigned GetMaxMipmapLevel() const {
> +        // Clamp to [base, levels - 1]
> +        return std::min(std::max(mBaseMipmapLevel, mMaxMipmapLevel), mMaxLevelWithCustomImages);

Don't do the comparison of base and max here. We need to return a max > base when max is > base, so we can realize it and mark it mip-incomplete.
Attachment #8503486 - Flags: review?(jgilbert) → review-
Attached patch bug1080957-mipmap-levels.patch (obsolete) — Splinter Review
The glTexParameter call is at the very bottom of the function so I think it's ok. Addressed the rest of the review comments.
Attachment #8503486 - Attachment is obsolete: true
Comment on attachment 8505739 [details] [diff] [review]
rebased

Whoops forgot to re-r?
Attachment #8505739 - Flags: review?(jgilbert)
Comment on attachment 8505739 [details] [diff] [review]
rebased

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

A lot of this code seems to bend over backwards to optimize for textures we've called generateMipmaps on. This will generally save a maximum of 13ish iterations per texture, which doesn't seem worth it.

::: dom/canvas/WebGLTexture.cpp
@@ +91,5 @@
> +
> +        // Check the raw value here, not the clamped one, since we don't want
> +        // to terminate early if there aren't enough levels defined.
> +        if (level == mMaxMipmapLevel)
> +            return true;

It looks like this is not useful, since we'd break at the end of this iteration anyways.

::: dom/canvas/WebGLTexture.h
@@ +286,4 @@
>      bool IsImmutable() const { return mImmutable; }
>      void SetImmutable() { mImmutable = true; }
>  
> +    void SetBaseMipmapLevel(unsigned level) { mBaseMipmapLevel = level; }

Don't use `unsigned` as a type. It's storing a size_t, so why not use that type?

@@ +289,5 @@
> +    void SetBaseMipmapLevel(unsigned level) { mBaseMipmapLevel = level; }
> +    void SetMaxMipmapLevel(unsigned level) { mMaxMipmapLevel = level; }
> +    size_t GetBaseMipmapLevel() const {
> +        // Clamp to [0, levels - 1]
> +        return std::min(mBaseMipmapLevel, mMaxLevelWithCustomImages);

Why is this useful? If mMaxLevelWithCustomImages is < mBaseMipmapLevel, GetBaseMipmapLevel returns a level value which should not be considered.

@@ +293,5 @@
> +        return std::min(mBaseMipmapLevel, mMaxLevelWithCustomImages);
> +    }
> +    size_t GetMaxMipmapLevel() const {
> +        // Clamp to [base, levels - 1]
> +        return std::min(mMaxMipmapLevel, mMaxLevelWithCustomImages);

This comment is wrong. `max` can be lower than `base`. Assert if you mean it to be otherwise.
Attachment #8505739 - Flags: review?(jgilbert) → review-
The mHaveGeneratedMipmap modeswitch was intended not so much to avoid doing work, as to avoid storing ImageInfo's for all the generated mipmaps (memory optimization, not speed optimization). However, I agree that it was not worth all the extra complexity. It saves a few dozen bytes per texture level, which is negligible compared to the texture image size.
> Why is this useful? If mMaxLevelWithCustomImages is < mBaseMipmapLevel, 
> GetBaseMipmapLevel returns a level value which should not be considered.

The spec says:

"For immutable-format textures, levelbase is clamped to the range [0, levels − 1], levelmax is then clamped to the range [levelbase, levels − 1]"
(In reply to David Anderson [:dvander] from comment #8)
> > Why is this useful? If mMaxLevelWithCustomImages is < mBaseMipmapLevel, 
> > GetBaseMipmapLevel returns a level value which should not be considered.
> 
> The spec says:
> 
> "For immutable-format textures, levelbase is clamped to the range [0, levels
> − 1], levelmax is then clamped to the range [levelbase, levels − 1]"

Sorry, I meant that the comment does not match the implementation. The implementation should then be:

> std::max(GetBaseMipmapLevel(), std::min(mMaxMipmapLevel, mMaxLevelWithCustomImages))

I do not think these should be helper functions, since I believe it would be more clear if we calculated them explicitly in the caller. Otherwise, the helper functions should be renamed. Right now they look like getters for their respective members, when they actually calculate an intermediate value as detailed by the spec. CalcSpec_levelBase()?
This version (I think) follows the spec more closely.
Attachment #8503516 - Attachment is obsolete: true
Attachment #8505739 - Attachment is obsolete: true
Attachment #8505906 - Flags: review?(jgilbert)
Comment on attachment 8505906 [details] [diff] [review]
bug1080957-mipmap-levels.patch

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

::: dom/canvas/WebGLContextGL.cpp
@@ +923,5 @@
>                                                    ? LOCAL_GL_TEXTURE_2D
>                                                    : LOCAL_GL_TEXTURE_CUBE_MAP_POSITIVE_X;
> +    if (!tex->IsMipmapRangeValid())
> +    {
> +        return ErrorInvalidOperation("generateMipmap: Texture does not have a valid mipmap range.");

Single-statement blocks should be either:

> if (foo)
>   return bar;

or

> if (foo) {
>   return bar;
> }

Looks like this was pre-existing.
Attachment #8505906 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/2cf543c271dc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
I accidentally pushed the wrong patch, correct one: https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdbd3e97f00
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/4bdbd3e97f00
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.