Bug 46493 – [chromium] Add mipmap support for ImageLayerChromium

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[W. James MacLean]

Enable mipmap support for image textures rendered via GPU compositor.

[W. James MacLean]

Created attachment 68724 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 68724 [details]
Patch

You need a couple more things: 
1. Enable linear mip filtering (GL_LINEAR_MIPMAP_LINEAR) for texture accesses
2. Do this only when either the NPOT extension is supporter or the texture is POT

[W. James MacLean]

(In reply to comment #2)
> (From update of attachment 68724 [details])
> You need a couple more things: 
> 1. Enable linear mip filtering (GL_LINEAR_MIPMAP_LINEAR) for texture accesses
> 2. Do this only when either the NPOT extension is supporter or the texture is POT

I'm trying to get confirmation from Ken Russell, but I think the context3D wrapper looks after the case where the NPOT extension is not available. Will revise and resubmit.

[W. James MacLean]

Created attachment 68906 [details]
Patch

[W. James MacLean]

(In reply to comment #4)
> Created an attachment (id=68906) [details]
> Patch

The code checking for NPOT support is a bit clunky, although could be simplified if a method "bool checkExtension(const char *)" was added to GraphicsContext3D (?) to do the check.

[Vangelis Kokkevis]

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=68906) [details] [details]
> > Patch
> 
> The code checking for NPOT support is a bit clunky, although could be simplified if a method "bool checkExtension(const char *)" was added to GraphicsContext3D (?) to do the check.

A better place for checking the extension would be to put it in the constructor of ContentLayerChromium::SharedValues  .  GraphicsContext3D is supposed to mirror the GL ES 2 API pretty closely so it shouldn't be doing any texture adjustments behind the scenes.

[W. James MacLean]

Created attachment 69041 [details]
Patch

[W. James MacLean]

(In reply to comment #7)
> Created an attachment (id=69041) [details]
> Patch

I've moved the one-time check for npot support into the constructor for ContentLayerChromium::SharedValues. This will make access to this information, should it be needed, a little more universal.

[W. James MacLean]

Question:

Do we need to add

#extension GL_OES_texture_npot : enable

to the fragmentShaderString in ContentLayerChromium if npot support is available? If we do, this can easily be added in the SharedValues constructor also ...

[Vangelis Kokkevis]

Comment on attachment 69041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69041&action=review

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:109
> +

can you use String::contains() here instead of converting to a char* ?

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:269
> +        GLC(context, context->texParameterf(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, 

The filter mode setting can move up to the part that allocates the texture (where you call context->texImage2D) so you don't execute it every time.  You also need to make sure that you reset the filter modes when the tests fail as the layer size can change.

Also, it should be calling texParameteri instead of texParameterf (I think we have the same problem elsewhere in the code)

[W. James MacLean]

Created attachment 69101 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 69101 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69101&action=review

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:281
> +            GLC(context, context->generateMipmap(GraphicsContext3D::TEXTURE_2D));

You'll need to also call generateMipmap in the "else" clause as well, if the conditions are met. I suggest doing something like:

bool doMipmap =  generateMipmap && (layerRenderer()->contentLayerSharedValues()->npotSupported() || (isPowerOfTwo(updateRect.width()) && isPowerOfTwo(updateRect.height())))

outside the if statement and the in the "if" part do:

if (doMipmap)
  set filter modes


and outside the if statement
if (doMipmap)
  generateMipmap

[W. James MacLean]

D'oh, sorry about that ... yes, you're quite right. I'll file a fix in the morning.

[W. James MacLean]

Created attachment 69182 [details]
Patch

[W. James MacLean]

(In reply to comment #9)
> Question:
> 
> Do we need to add
> 
> #extension GL_OES_texture_npot : enable
> 
> to the fragmentShaderString in ContentLayerChromium if npot support is available? If we do, this can easily be added in the SharedValues constructor also ...

No ... only extensions that modify the shader language need this ...

[Vangelis Kokkevis]

Comment on attachment 69182 [details]
Patch

Code looks good.  Can you please also add a layout test that demonstrates that this works? What I'm after is making sure that we don't get hardware which fails to create mipmaps or reports that it supports mipmapping but doesn't. The test could for example create an image layer with a colored checkerboard texture, apply a perspective 3D transform on it and read back the pixels to make sure they are not black. Thanks.

[W. James MacLean]

(In reply to comment #16)
> (From update of attachment 69182 [details])
> Code looks good.  Can you please also add a layout test that demonstrates that this works? What I'm after is making sure that we don't get hardware which fails to create mipmaps or reports that it supports mipmapping but doesn't. The test could for example create an image layer with a colored checkerboard texture, apply a perspective 3D transform on it and read back the pixels to make sure they are not black. Thanks.

Sure, not a problem.

I have a question: if hardware claims to support mip-mapping, and doesn't, are we guaranteed to get black pixels? Might it possibly fall-back to linear/nearest interpolation of the raw texture, in which case the pixels won't be black?

[Vangelis Kokkevis]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 69182 [details] [details])
> > Code looks good.  Can you please also add a layout test that demonstrates that this works? What I'm after is making sure that we don't get hardware which fails to create mipmaps or reports that it supports mipmapping but doesn't. The test could for example create an image layer with a colored checkerboard texture, apply a perspective 3D transform on it and read back the pixels to make sure they are not black. Thanks.
> 
> Sure, not a problem.
> 
> I have a question: if hardware claims to support mip-mapping, and doesn't, are we guaranteed to get black pixels? Might it possibly fall-back to linear/nearest interpolation of the raw texture, in which case the pixels won't be black?

If the drivers are faulty, they could really be doing anything.  If it does end up falling back to nearest, it would be somewhat fortunate. It would be nice to have a layout test in place that we can run on a number of machines to make sure there are no unexpected failures.  Probably worthwhile having both at POT and an NPOT layer.

[Vangelis Kokkevis]

On second though, I think the existing layout tests on image layers (e.g. direct-image-background-color.html) would provide an early warning if something's wrong with the mipmapping and the image doesn't show up.

[James Robinson]

Comment on attachment 69182 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69182&action=review

R- for nits, but overall looks fine.  I mostly want to make sure the testing situation is accurately reflected in the ChangeLog.

> WebCore/ChangeLog:8
> +        No change in behaviour, use existing tests.

Comment is not really accurate, this does change behavior.  Could you list some tests that would cover this?

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:270
> +    bool doMipmap = generateMipmap && (layerRenderer()->contentLayerSharedValues()->npotSupported()

Not sure what it means to "do" mipmap. It looks like this boolean is actually recording whether to generate mipmaps, so why isn't it called generateMipmaps?  The parameter to this function could be called something like "requestMipmaps" to indicate that the implementation will not necessarily respect the caller's wishes.

> WebCore/platform/graphics/chromium/ContentLayerChromium.h:83
> +                           const IntRect& updateRect, unsigned textureId, bool generateMipmap = false);

In general, it's better to have a two-state enum for APIs like this so it's clearer at a callsite what true or false mean.

> WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:154
> +    const bool generateMipmap = true;

Why does this local exist?

[W. James MacLean]

Created attachment 69637 [details]
Patch

[W. James MacLean]

(In reply to comment #20)
> (From update of attachment 69182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69182&action=review
> 
> R- for nits, but overall looks fine.  I mostly want to make sure the testing situation is accurately reflected in the ChangeLog.
> 
> > WebCore/ChangeLog:8
> > +        No change in behaviour, use existing tests.
> 
> Comment is not really accurate, this does change behavior.  Could you list some tests that would cover this?

Done.
 
> > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:270
> > +    bool doMipmap = generateMipmap && (layerRenderer()->contentLayerSharedValues()->npotSupported()
> 
> Not sure what it means to "do" mipmap. It looks like this boolean is actually recording whether to generate mipmaps, so why isn't it called generateMipmaps?  The parameter to this function could be called something like "requestMipmaps" to indicate that the implementation will not necessarily respect the caller's wishes.

Done.

> > WebCore/platform/graphics/chromium/ContentLayerChromium.h:83
> > +                           const IntRect& updateRect, unsigned textureId, bool generateMipmap = false);
> 
> In general, it's better to have a two-state enum for APIs like this so it's clearer at a callsite what true or false mean.

Agreed, this does make the code clearer ... done.

> > WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:154
> > +    const bool generateMipmap = true;
> 
> Why does this local exist?

I think I was doing it to (1) make it clear what the final parameter was doing, and that (2) the compiler would not generate an actual local, but collapse it into a literal constant.

[James Robinson]

Comment on attachment 69637 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69637&action=review

Looks good

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:272
> +    bool generateMipmap = (requestMipmap == useMipmap)
> +                          && (layerRenderer()->contentLayerSharedValues()->npotSupported()
> +                          || (isPowerOfTwo(updateRect.width()) && isPowerOfTwo(updateRect.height())));

nit: the indentation is a little odd here.  Maybe indent the || line a bit more so it's clearer?

[W. James MacLean]

Created attachment 69832 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 69832 [details]
Patch

Clearing flags on attachment: 69832

Committed r69172: <http://trac.webkit.org/changeset/69172>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.