Enable mipmap support for image textures rendered via GPU compositor.
Created attachment 68724 [details] Patch
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
(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.
Created attachment 68906 [details] Patch
(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.
(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.
Created attachment 69041 [details] Patch
(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.
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 ...
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)
Created attachment 69101 [details] Patch
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
D'oh, sorry about that ... yes, you're quite right. I'll file a fix in the morning.
Created attachment 69182 [details] Patch
(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 ...
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.
(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?
(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.
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.
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?
Created attachment 69637 [details] Patch
(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.
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?
Created attachment 69832 [details] Patch
Comment on attachment 69832 [details] Patch Clearing flags on attachment: 69832 Committed r69172: <http://trac.webkit.org/changeset/69172>
All reviewed patches have been landed. Closing bug.