Bug 43864 – [chromium] Canvas2D does not support images larger than system's GPU max texture size

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Vincent Scheib]

GPU acceleration of Canvas2D is limited by the maximum size of a texture that can be manipulated by a GPU. Source images must be split into tiles and managed as if they were logically one large image.

[Vincent Scheib]

Created attachment 64139 [details]
patch

[WebKit Review Bot]

Attachment 64139 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
/tests/TilingDataTest.cpp:185:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:186:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:187:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:188:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:190:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:191:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:192:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:193:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:194:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:196:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:197:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:201:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:202:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:204:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:205:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:206:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:207:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:209:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:210:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:211:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:212:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:214:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:215:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:216:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:217:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:218:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:220:  Extra space after ( in function call  [whitespace/parens] [4]
WebKit/chromium/tests/TilingDataTest.cpp:221:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 141 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vincent Scheib]

Created attachment 64164 [details]
Patch

[WebKit Review Bot]

Attachment 64164 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/tests/TilingDataTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vincent Scheib]

Created attachment 64177 [details]
Patch

[WebKit Review Bot]

Attachment 64177 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/tests/TilingDataTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 64177 [details]
Patch

> diff --git a/WebCore/platform/graphics/chromium/GLES2Canvas.cpp b/WebCore/platform/graphics/chromium/GLES2Canvas.cpp
>  
> +    m_context->vertexAttribPointer(m_texPositionLocation, 3, GraphicsContext3D::FLOAT, false, 0, 0);
> +
> +    m_context->enableVertexAttribArray(m_texPositionLocation);
> +
> +    const TilingData& tiles = texture->tiles();
> +    IntRect tileIdxRect = tiles.overlappedTileIndices(srcRect);
> +
> +    for (int y = tileIdxRect.y(); y <= tileIdxRect.bottom(); y++)
> +        for (int x = tileIdxRect.x(); x <= tileIdxRect.right(); x++)
> +            drawTexturedRectTile(texture, tiles.tileIndex(x, y), srcRect, dstRect, transform);
> +}
> +
> +void GLES2Canvas::drawTexturedRectTile(GLES2Texture* texture, int tile, const FloatRect& srcRect, const FloatRect& dstRect, const AffineTransform& transform)
> +{
> +    if (dstRect.isEmpty())
> +        return;
> +
> +    const TilingData tiles = texture->tiles();

It's strange that we grab texture->tiles() in drawTexturedRect(), then call drawTexturedRectTile() which then grabs texture->tiles() again.  Feels like we should only do this once.

Also, did you mean to copy TilingData here or did you mean const TilingData& tiles = ...?  I feel more likely the latter.

> +
> +    texture->bindTile(tile);
> +
> +    FloatRect srcRectClippedInTileSpace;
> +    FloatRect dstRectIntersected;
> +    tiles.intersectDrawQuad(srcRect, dstRect, tile, srcRectClippedInTileSpace, dstRectIntersected);

Please make intersectDrawQuad() take its out parameters by pointer rather than non-const reference so that it's easier to tell at this callsite which parameters are being modified by the function.

> diff --git a/WebCore/platform/graphics/chromium/GLES2Texture.cpp b/WebCore/platform/graphics/chromium/GLES2Texture.cpp
> index 5e8a141dd282f294413a851f124ba80df7fd1d40..9ebbc78d19b7d867aede16fb7ab38ca8b48c0ce3 100644
> --- a/WebCore/platform/graphics/chromium/GLES2Texture.cpp
> +++ b/WebCore/platform/graphics/chromium/GLES2Texture.cpp
> @@ -35,42 +35,61 @@
>  #include "GLES2Texture.h"
>  
>  #include "GraphicsContext3D.h"
> -
> +#include "IntRect.h"
>  #include <wtf/OwnArrayPtr.h>
>  
>  namespace WebCore {
>  
> -GLES2Texture::GLES2Texture(GraphicsContext3D* context, unsigned textureId, Format format, int width, int height)
> +
> +GLES2Texture::GLES2Texture(GraphicsContext3D* context, const Vector<unsigned int>& tileTextureIds, Format format, int width, int height, int maxTextureSize)
>      : m_context(context)
> -    , m_textureId(textureId)
>      , m_format(format)
> -    , m_width(width)
> -    , m_height(height)
> +    , m_tiles(maxTextureSize, width, height, true)

It looks like we always set the borderTexels parameter to 'true' currently (outside of test code).  Will we ever set this to false?  If we aren't then it might be simpler to drop the parameter.

>  PassRefPtr<GLES2Texture> GLES2Texture::create(GraphicsContext3D* context, Format format, int width, int height)

> +    TilingData tiling(maxTextureSize, width, height, true);
> +    int numTiles = tiling.numTiles();
> +
> +    Vector<unsigned int> textureIds(numTiles);

Nit: it's kind of a bummer to stack-allocate a vector, populate it with a bunch of values, and then in the TilingData constructor do a deep copy of this vector into another vector and then throw the stack-allocated one away.  I think it'd be nicer to have GLES2Texture have an OwnPtr<Vector<...> > and pass in a heap-allocated vector so we only have to fill it out once.  Same goes for TilingData.


> +    textureIds.fill(0, numTiles);
> +    for (int i = 0; i < numTiles; i++)
> +         textureIds[i] = context->createTexture();
> +
> +    for (int i = 0; i < numTiles; i++) {
> +        unsigned int textureId = textureIds.begin()[i];

Not sure why we have two loops from i..numTiles here.  Also, I think the .begin() here is spurious - looks like you want textureIds[i].

> +        if (!textureId) {
Should probably check for this at createTexture() time.

> +            for (int i = 0; i < numTiles; i++)
> +                context->deleteTexture(textureIds[i]);
> +            return 0;
> +        }
>  
> -    return adoptRef(new GLES2Texture(context, textureId, format, width, height));
> +        IntRect tileBoundsWithBorder = tiling.tileBoundsWithBorder(i);
> +
> +        context->bindTexture(GraphicsContext3D::TEXTURE_2D, textureId);
> +        context->texImage2D(GraphicsContext3D::TEXTURE_2D,
> +            0,
> +            GraphicsContext3D::RGBA,
> +            tileBoundsWithBorder.width(),
> +            tileBoundsWithBorder.height(),
> +            0,
> +            GraphicsContext3D::RGBA,
> +            GraphicsContext3D::UNSIGNED_BYTE,
> +            0);

I think it's more efficient to go ahead and set the texParameter()s we want for this texture here rather than setting them on every bindTexture().

> +    }
> +    return adoptRef(new GLES2Texture(context, textureIds, format, width, height, maxTextureSize));
>  }
>  
>  static void convertFormat(GLES2Texture::Format format, unsigned int* glFormat, unsigned int* glType, bool* swizzle)
> @@ -94,31 +113,65 @@ static void convertFormat(GLES2Texture::Format format, unsigned int* glFormat, u
>      }
>  }
>  
>  void GLES2Texture::load(void* pixels)
>  {
> +    uint32_t* pixels32 = static_cast<uint32_t*>(pixels);
>      unsigned int glFormat, glType;
>      bool swizzle;
>      convertFormat(m_format, &glFormat, &glType, &swizzle);
> -    m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, m_textureId);
>      if (swizzle) {
>          ASSERT(glFormat == GraphicsContext3D::RGBA && glType == GraphicsContext3D::UNSIGNED_BYTE);
>          // FIXME:  This could use PBO's to save doing an extra copy here.
> -        int size = m_width * m_height;
> -        unsigned* pixels32 = static_cast<unsigned*>(pixels);
> -        OwnArrayPtr<unsigned> buf(new unsigned[size]);
> -        unsigned* bufptr = buf.get();
> -        for (int i = 0; i < size; ++i) {
> -            unsigned pixel = pixels32[i];
> -            bufptr[i] = pixel & 0xFF00FF00 | ((pixel & 0x00FF0000) >> 16) | ((pixel & 0x000000FF) << 16);
> -        }
> -        m_context->texSubImage2D(GraphicsContext3D::TEXTURE_2D, 0, 0, 0, m_width, m_height, glFormat, glType, buf.get());
> -    } else
> -        m_context->texSubImage2D(GraphicsContext3D::TEXTURE_2D, 0, 0, 0, m_width, m_height, glFormat, glType, pixels);
> +    }
> +    uint32_t* tempBuff = new uint32_t[m_tiles.maxTextureSize()*m_tiles.maxTextureSize()];

Use an OwnArrayPtr<> here to avoid having to do the delete[].  This'll save us from memory leaks if we have to early-out of this function in the future.

> +
> +    for (int i = 0; i < m_tiles.numTiles(); i++) {
> +        IntRect tileBoundsWithBorder = m_tiles.tileBoundsWithBorder(i);
> +
> +        uint32_t* uploadBuff = 0;
> +        if (swizzle)
> +            uploadBuff = copySubRect<true>(
> +            pixels32, tileBoundsWithBorder.x(), tileBoundsWithBorder.y(),
> +            tempBuff, tileBoundsWithBorder.width(), tileBoundsWithBorder.height(), m_tiles.totalSizeX());
> +        else
> +            uploadBuff = copySubRect<false>(
> +            pixels32, tileBoundsWithBorder.x(), tileBoundsWithBorder.y(),
> +            tempBuff, tileBoundsWithBorder.width(), tileBoundsWithBorder.height(), m_tiles.totalSizeX());
> +
> +        m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, m_tileTextureIds[i]);
> +        m_context->texSubImage2D(GraphicsContext3D::TEXTURE_2D, 0, 0, 0,
> +            tileBoundsWithBorder.width(),
> +            tileBoundsWithBorder.height(), glFormat, glType, uploadBuff);
> +    }
> +
> +    delete[] tempBuff;
>  }
> diff --git a/WebCore/platform/graphics/chromium/GLES2Texture.h b/WebCore/platform/graphics/chromium/GLES2Texture.h
>      void load(void* pixels);
>      Format format() const { return m_format; }
> -    int width() const { return m_width; }
> -    int height() const { return m_height; }

I think we still want to have width()/height() exposed on GLES2Texture (even if they just call to TilingData under the hood).

> +    const TilingData& tiles() const { return m_tiles; }
>  private:
> -    GLES2Texture(GraphicsContext3D*, unsigned textureId, Format, int width, int height);
> +    GLES2Texture(GraphicsContext3D*, const Vector<unsigned int>& tileTextureIds, Format format, int width, int height, int maxTextureSize);
>      GraphicsContext3D* m_context;
> -    unsigned m_textureId;
>      Format m_format;
> -    int m_width;
> -    int m_height;
> +    TilingData m_tiles;
> +    Vector<unsigned int> m_tileTextureIds;
>  };
>  
>  }
> diff --git a/WebCore/platform/graphics/chromium/TilingData.cpp b/WebCore/platform/graphics/chromium/TilingData.cpp
> +IntRect TilingData::tileBoundsWithBorder(int tile) const
> +{
> +    IntRect bounds = tileBounds(tile);
> +
> +    if (m_borderTexels) {
> +        int x1 = bounds.x();
> +        int x2 = bounds.right();
> +        int y1 = bounds.y();
> +        int y2 = bounds.bottom();
> +
> +        if (tileXIndex(tile) > 0)
> +            x1--;

If m_borderTexels is an arbitrary int, don't we want to do x1 -= m_borderTexels;?  I may not be following the math perfectly.  If m_borderTexels is only ever 0 or 1 it should be a bool.

> +        if (tileXIndex(tile) < (numTilesX()-1))
> +            x2++;
> +        if (tileYIndex(tile) > 0)
> +            y1--;
> +        if (tileYIndex(tile) < (numTilesY()-1))
> +            y2++;
> +
> +        bounds = IntRect(x1, y1, x2-x1, y2-y1);
> +    }
> +
> +    return bounds;
> +}
> +
> +IntRect TilingData::overlappedTileIndices(const WebCore::FloatRect &srcRect) const
> +{
> +    return overlappedTileIndices(IntRect(
> +        static_cast<int>(srcRect.x()),
> +        static_cast<int>(srcRect.y()),
> +        static_cast<int>(ceil(srcRect.width())),
> +        static_cast<int>(ceil(srcRect.height()))));

There's an enclosingIntRect() function to go from FloatRect->IntRect that I believe does what you want here.  It's in FloatRect.h

> +void TilingData::intersectDrawQuad(const FloatRect& srcRect, const FloatRect& dstRect, int tile,
> +                                   FloatRect& newSrc, FloatRect& newDst) const
> +{
> +    // Intersect with tile
> +    FloatRect tileBounds = this->tileBounds(tile);
> +    FloatRect srcRectIntersected = srcRect;
> +    srcRectIntersected.intersect(tileBounds);
> +
> +    if (srcRectIntersected.isEmpty()) {
> +        newSrc = newDst = FloatRect(0, 0, 0, 0);
> +        return;
> +    }
> +
> +    float srcRectIntersectedNormX = (srcRectIntersected.x() - srcRect.x()) / srcRect.width();
> +    float srcRectIntersectedNormY = (srcRectIntersected.y() - srcRect.y()) / srcRect.height();
> +    float srcRectIntersectedNormW = srcRectIntersected.width() / srcRect.width();
> +    float srcRectIntersectedNormH = srcRectIntersected.height() / srcRect.height();
> +
> +    newSrc = srcRectIntersected;
> +    newSrc.move(-tileBounds.x(), -tileBounds.y());
> +    if (m_borderTexels) {
> +        if (tileXIndex(tile) > 0)
> +            newSrc.move(1, 0);

Is 1 a magic value, or is this supposed to be moved by m_borderTexels?

> +        if (tileYIndex(tile) > 0)
> +            newSrc.move(0, 1);
> +    }
> +
> +    newDst = FloatRect(
> +        srcRectIntersectedNormX * dstRect.width() + dstRect.x(),
> +        srcRectIntersectedNormY * dstRect.height() + dstRect.y(),
> +        srcRectIntersectedNormW * dstRect.width(),
> +        srcRectIntersectedNormH * dstRect.height());
> +}
> +
> +}

> diff --git a/WebCore/platform/graphics/chromium/TilingData.h b/WebCore/platform/graphics/chromium/TilingData.h

> +class TilingData {

If possible, make this inherit from Noncopyable.  It's a cheap class to copy today, but it looks like we never need to copy this now and it's probably best to keep it that way in case the class has to grow.

> +public:
> +    TilingData(int maxTextureSize, int totalSizeX, int totalSizeY, bool borderTexels);
> +    int maxTextureSize() const { return m_maxTextureSize; }
> +    int totalSizeX() const { return m_totalSizeX; }
> +    int totalSizeY() const { return m_totalSizeY; }
> +
> +    int numTiles() const { return numTilesX() * numTilesY(); }
> +    int numTilesX() const { return m_numTilesX; }
> +    int numTilesY() const { return m_numTilesY; }
> +    int tileIndex(int x, int y) const { return x + y * numTilesX(); }
> +    int tileXIndex(int tile) const { assertTile(tile); return tile % numTilesX(); }
> +    int tileYIndex(int tile) const { assertTile(tile); return tile / numTilesX(); }
> +    int tileXIndexFromSrcCoord(int) const;
> +    int tileYIndexFromSrcCoord(int) const;
> +
> +    IntRect tileBounds(int tile) const;
> +    IntRect tileBoundsWithBorder(int tile) const;
> +    FloatRect tileBoundsNormalized(int tile) const;
> +    int tilePositionX(int xIndex) const;
> +    int tilePositionY(int yIndex) const;
> +    int tileSizeX(int xIndex) const;
> +    int tileSizeY(int yIndex) const;
> +    IntRect overlappedTileIndices(const IntRect& srcRect) const;
> +    IntRect overlappedTileIndices(const FloatRect& srcRect) const;
> +
> +    // Given a set of source and destination coordinates for a drawing quad
> +    // in texel units, returns adjusted data to render just the one tile.
> +    void intersectDrawQuad(const FloatRect& srcRect, const FloatRect& dstRect, int tile, FloatRect& newSrc, FloatRect& newDst) const;

(also mentioned this above) - this should take its out parameters (newSrc, newDst) as pointers to FloatRects instead of non-const references.  This makes it easier to tell what things will get modified at callsites to this function.

> +
> +private:
> +    TilingData() : m_maxTextureSize(0), m_totalSizeX(0), m_totalSizeY(0) {}
> +    void assertTile(int tile) const { ASSERT(tile >= 0 && tile < numTiles()); }
> +
> +    int m_maxTextureSize;
> +    int m_totalSizeX;
> +    int m_totalSizeY;
> +    int m_borderTexels;
> +
> +    // computed values:
> +    int m_numTilesX;
> +    int m_numTilesY;
> +};
> +
> +}
> +
> +#endif // TilingData_h
> diff --git a/WebCore/platform/graphics/skia/ImageSkia.cpp b/WebCore/platform/graphics/skia/ImageSkia.cpp
> index 024bf50dc1333a20aa2daae1ac7e5a4f5eec0e10..b514b1a6b61b5d70cbd877cb3b1164a3be1d517b 100644
> --- a/WebCore/platform/graphics/skia/ImageSkia.cpp
> +++ b/WebCore/platform/graphics/skia/ImageSkia.cpp
> @@ -465,18 +465,18 @@ void BitmapImage::draw(GraphicsContext* ctxt, const FloatRect& dstRect,
>      if (!bm)
>          return;  // It's too early and we don't have an image yet.
>  
> -#if  USE(GLES2_RENDERING)
> -    if (ctxt->platformContext()->useGPU()) {
> -        drawBitmapGLES2(ctxt, bm, srcRect, dstRect, colorSpace, compositeOp);
> -        return;
> -    }
> -#endif
>      FloatRect normDstRect = normalizeRect(dstRect);
>      FloatRect normSrcRect = normalizeRect(srcRect);
>  
>      if (normSrcRect.isEmpty() || normDstRect.isEmpty())
>          return;  // Nothing to draw.
>  
> +#if  USE(GLES2_RENDERING)
> +    if (ctxt->platformContext()->useGPU()) {
> +        drawBitmapGLES2(ctxt, bm, normSrcRect, normDstRect, colorSpace, compositeOp);
> +        return;
> +    }
> +#endif
>      ctxt->platformContext()->prepareForSoftwareDraw();
>  
>      paintSkBitmap(ctxt->platformContext(),
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index b5247d5ed62e0982545241323653261ec666de1b..1f87b7529ba6128bddb2a9bf08d37b91566b7cc3 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,14 @@
> [phone]  Vincent Scheib  <[e-mail]>
> +
> +        Canvas2D does not support images larger than system's GPU max texture size
> +        https://bugs.webkit.org/show_bug.cgi?id=43864
> +
> +        Unit tests for TilingData class.
> +
> +        * WebKit.gyp:
> +        * tests/TilingDataTest.cpp: Added.
> +        (WebCore::TEST):
> +
>  2010-08-10  Aaron Boodman  <[e-mail]>
>

[David Levin]

Comment on attachment 64177 [details]
Patch

r- based on James' feedback alone.


nits:
Please make sure to add spaces around operators. For example, "2*m_borderTexels" is done in a few places and there are many places where there aren't spaces around +, -, *.

If the clause of an if (or else) statement spans more than one physical line it needs braces around it. Here's one such example:
154         if (swizzle)
 155             uploadBuff = copySubRect<true>(
 156             pixels32, tileBoundsWithBorder.x(), tileBoundsWithBorder.y(),
 157             tempBuff, tileBoundsWithBorder.width(), tileBoundsWithBorder.height(), m_tiles.totalSizeX());

There are others.

[Vincent Scheib]

Created attachment 64285 [details]
Patch

[WebKit Review Bot]

Attachment 64285 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/tests/TilingDataTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vincent Scheib]

Comments addressed inline:


> > +void GLES2Canvas::drawTexturedRectTile(GLES2Texture* texture, int tile,
> const FloatRect& srcRect, const FloatRect& dstRect, const AffineTransform&
> transform)
> > +{
> > +    if (dstRect.isEmpty())
> > +        return;
> > +
> > +    const TilingData tiles = texture->tiles();
>
> It's strange that we grab texture->tiles() in drawTexturedRect(), then call
> drawTexturedRectTile() which then grabs texture->tiles() again.  Feels like
> we should only do this once.
>
>
Fixing to be a reference. The call is just for code convenience,
texture->tiles() is intended to be used directly as a const reference, there
is no cost to accessing it at multiple frames in the stack. Passing the
reference as a parameter would take as many or more bytes in code and be
equal or less easy for the compiler to ignore.


> +    tiles.intersectDrawQuad(srcRect, dstRect, tile,
> srcRectClippedInTileSpace, dstRectIntersected);
>
> Please make intersectDrawQuad() take its out parameters by pointer


done

> +GLES2Texture::GLES2Texture(GraphicsContext3D* context, const
> Vector<unsigned int>& tileTextureIds, Format format, int width, int height,
> int maxTextureSize)
> >      : m_context(context)
> > -    , m_textureId(textureId)
> >      , m_format(format)
> > -    , m_width(width)
> > -    , m_height(height)
> > +    , m_tiles(maxTextureSize, width, height, true)
>
> It looks like we always set the borderTexels parameter to 'true' currently
> (outside of test code).  Will we ever set this to false?  If we aren't then
> it might be simpler to drop the parameter.
>
>
I expect the compositor to be able to use this class, and it will not use
border texels.



> >  PassRefPtr<GLES2Texture> GLES2Texture::create(GraphicsContext3D*
> context, Format format, int width, int height)
> > +    Vector<unsigned int> textureIds(numTiles);
>
> ... nicer to have GLES2Texture have an OwnPtr<Vector<...> > and pass in a
> heap-allocated vector so we only have to fill it out once.  Same goes for
> TilingData.
>
>
Done for the Vector.
TilingData has no heap allocation, and has trivial constructor cost.
TilingData is just convenience math, and is cheaper to construct than to
heap allocate.



> > +    textureIds.fill(0, numTiles);
> > +    for (int i = 0; i < numTiles; i++)
> > +         textureIds[i] = context->createTexture();
> > +
> > +    for (int i = 0; i < numTiles; i++) {
> > +        unsigned int textureId = textureIds.begin()[i];
>
> Not sure why we have two loops from i..numTiles here.  Also, I think the
> .begin() here is spurious - looks like you want textureIds[i].
>
>
Fixed, it was legacy from GLES (could array allocate) vs GraphicsContext3D.


> +        if (!textureId) {
> Should probably check for this at createTexture() time.
>
>
Fixed.



> > +        context->bindTexture(GraphicsContext3D::TEXTURE_2D, textureId);
> > +        context->texImage2D(GraphicsContext3D::TEXTURE_2D,
> > +            0,
> > +            GraphicsContext3D::RGBA,
> > +            tileBoundsWithBorder.width(),
> > +            tileBoundsWithBorder.height(),
> > +            0,
> > +            GraphicsContext3D::RGBA,
> > +            GraphicsContext3D::UNSIGNED_BYTE,
> > +            0);
>
> I think it's more efficient to go ahead and set the texParameter()s we want
> for this texture here rather than setting them on every bindTexture().
>
>
We need to do it near the draw call. This state applies to the "active
texture" not the particular texture bound with texture ID.
e.g.
bind(1)
texParameter(foo)
bind(2)
texParameter(bar)
bind(1)
  // texParameter is still bar, not the foo that was set after bind(1)

> +    uint32_t* tempBuff = new
> uint32_t[m_tiles.maxTextureSize()*m_tiles.maxTextureSize()];
>
> Use an OwnArrayPtr<>
>

Done.

> diff --git a/WebCore/platform/graphics/chromium/GLES2Texture.h
> b/WebCore/platform/graphics/chromium/GLES2Texture.h
> >      void load(void* pixels);
> >      Format format() const { return m_format; }
> > -    int width() const { return m_width; }
> > -    int height() const { return m_height; }
>
> I think we still want to have width()/height() exposed on GLES2Texture
> (even if they just call to TilingData under the hood).
>
>
I disagree, I want client code to explicitly work with the tiled nature of
the texture. No easy bugs due to .width() compiling and someone only testing
with normal sized textures.

> diff --git a/WebCore/platform/graphics/chromium/TilingData.cpp
> b/WebCore/platform/graphics/chromium/TilingData.cpp
>
> If m_borderTexels is an arbitrary int, don't we want to do x1 -=
> m_borderTexels;?  I may not be following the math perfectly.  If
> m_borderTexels is only ever 0 or 1 it should be a bool.
>
>
Done.

> +    return overlappedTileIndices(IntRect(
> > +        static_cast<int>(srcRect.x()),
> > +        static_cast<int>(srcRect.y()),
> > +        static_cast<int>(ceil(srcRect.width())),
> > +        static_cast<int>(ceil(srcRect.height()))));
>
> There's an enclosingIntRect() function to go from FloatRect->IntRect that I
> believe does what you want here.  It's in FloatRect.h
>
>
Done.

> +    if (m_borderTexels) {
> > +        if (tileXIndex(tile) > 0)
> > +            newSrc.move(1, 0);
>
> Is 1 a magic value, or is this supposed to be moved by m_borderTexels?
>

Reworked to:
    newSrc->move(
        -tileBounds.x() + m_borderTexels * (tileXIndex(tile) > 0),
        -tileBounds.y() + m_borderTexels * (tileYIndex(tile) > 0));

[David Levin]

nit: several places in WebCore/platform/graphics/chromium/TilingData.cpp don't put spaces around "-".

[David Levin]

One more nit:  PassOwnPtr<Vector<unsigned int>>

I'm surprised that this isn't causing compilers issues. I've always had to put a space between the two >>. (I would do that just to make sure no compilers have troubles.)

[James Robinson]

Comment on attachment 64285 [details]
Patch

> diff --git a/WebCore/platform/graphics/chromium/GLES2Canvas.h b/WebCore/platform/graphics/chromium/GLES2Canvas.h
> index cea90aeaabb26147f1f243267180925cc8aa733c..0ad07fcdbfed99ad6ecd7e5746001fbbadafbe7c 100644
> --- a/WebCore/platform/graphics/chromium/GLES2Canvas.h
> +++ b/WebCore/platform/graphics/chromium/GLES2Canvas.h
> @@ -81,6 +81,7 @@ public:
>      GLES2Texture* getTexture(NativeImagePtr);
>  
>  private:
> +    void drawTexturedRectTile(GLES2Texture* texture, int tile, const FloatRect& srcRect, const FloatRect& dstRect, const AffineTransform&);
>      void applyCompositeOperator(CompositeOperator);
>      void checkGLError(const char* header);
>      unsigned getQuadVertices();
> diff --git a/WebCore/platform/graphics/chromium/GLES2Texture.cpp b/WebCore/platform/graphics/chromium/GLES2Texture.cpp
> index 5e8a141dd282f294413a851f124ba80df7fd1d40..7a2cad3772d23496aef349642c59cf781ac37a55 100644
> --- a/WebCore/platform/graphics/chromium/GLES2Texture.cpp
> +++ b/WebCore/platform/graphics/chromium/GLES2Texture.cpp
> @@ -35,42 +35,60 @@
>  #include "GLES2Texture.h"
>  
>  #include "GraphicsContext3D.h"
> -
> +#include "IntRect.h"
>  #include <wtf/OwnArrayPtr.h>
>  
>  namespace WebCore {
>  
> -GLES2Texture::GLES2Texture(GraphicsContext3D* context, unsigned textureId, Format format, int width, int height)
> +
> +GLES2Texture::GLES2Texture(GraphicsContext3D* context, PassOwnPtr<Vector<unsigned int>> tileTextureIds, Format format, int width, int height, int maxTextureSize)

You need to say PassOwnPtr<Vector<unsigned int> > (space between the >'s), sadly.  C++0x allows the syntax you used but not all of our compilers do yet :(

>  GLES2Texture::~GLES2Texture()
>  {
> -    m_context->deleteTexture(m_textureId);
> +    for (unsigned int i = 0; i < m_tileTextureIds->size(); i++)
> +        m_context->deleteTexture(m_tileTextureIds->at(i));

Just 'unsigned', not 'unsigned int'.  If you used an iterator here you could avoid having to do "->at()".

>  PassRefPtr<GLES2Texture> GLES2Texture::create(GraphicsContext3D* context, Format format, int width, int height)
>  {
> -    int max;
> -    context->getIntegerv(GraphicsContext3D::MAX_TEXTURE_SIZE, &max);
> -    if (width > max || height > max) {
> -        ASSERT(!"texture too big");
> -        return 0;
> -    }
> +    int maxTextureSize = 0;
> +    context->getIntegerv(GraphicsContext3D::MAX_TEXTURE_SIZE, &maxTextureSize);
>  
> -    unsigned textureId = context->createTexture();
> -    if (!textureId)
> -        return 0;
> +    TilingData tiling(maxTextureSize, width, height, true);
> +    int numTiles = tiling.numTiles();
>  
> -    context->bindTexture(GraphicsContext3D::TEXTURE_2D, textureId);
> -    context->texImage2D(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, width, height, 0, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, 0);
> +    PassOwnPtr<Vector<unsigned int>> textureIds = new Vector<unsigned int>(numTiles);

"> >"


>  static void convertFormat(GLES2Texture::Format format, unsigned int* glFormat, unsigned int* glType, bool* swizzle)
> @@ -94,31 +112,65 @@ static void convertFormat(GLES2Texture::Format format, unsigned int* glFormat, u
>      }
>  }
>  
>  void GLES2Texture::load(void* pixels)
>  {
> +    uint32_t* pixels32 = static_cast<uint32_t*>(pixels);
>      unsigned int glFormat, glType;
>      bool swizzle;
>      convertFormat(m_format, &glFormat, &glType, &swizzle);
> -    m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, m_textureId);
>      if (swizzle) {
>          ASSERT(glFormat == GraphicsContext3D::RGBA && glType == GraphicsContext3D::UNSIGNED_BYTE);
>          // FIXME:  This could use PBO's to save doing an extra copy here.
> -        int size = m_width * m_height;
> -        unsigned* pixels32 = static_cast<unsigned*>(pixels);
> -        OwnArrayPtr<unsigned> buf(new unsigned[size]);
> -        unsigned* bufptr = buf.get();
> -        for (int i = 0; i < size; ++i) {
> -            unsigned pixel = pixels32[i];
> -            bufptr[i] = pixel & 0xFF00FF00 | ((pixel & 0x00FF0000) >> 16) | ((pixel & 0x000000FF) << 16);
> +    }
> +    OwnArrayPtr<uint32_t> tempBuff = 
> +        OwnArrayPtr<uint32_t>(new uint32_t[m_tiles.maxTextureSize() * m_tiles.maxTextureSize()]);

A slightly terser to do this is:

OwnArrayPtr<uint32_t> tempBuff(new uint32_t[m_tiles.maxTextureSize() * m_tiles.maxTextureSize()]);

> diff --git a/WebCore/platform/graphics/chromium/GLES2Texture.h b/WebCore/platform/graphics/chromium/GLES2Texture.h
> +
> +int TilingData::tileSizeX(int xIndex) const
> +    if (!xIndex && m_numTilesX > 1)
> +        return m_maxTextureSize - m_borderTexels;
> +    if (xIndex < numTilesX() - 1)
> +        return m_maxTextureSize - 2 * m_borderTexels;

Doing math on a bool is a little too subtle - I can see this is why you had m_borderTexels defined as an int before.  IIRC C++ says that this always does what you expect but it's very tricky.  I think you should do (m_borderTexels ? 1 : 0) and the like.  It's not as concise but the next person through this function will appreciate being explicit.

> +int TilingData::tileSizeY(int yIndex) const

> +        return m_maxTextureSize - m_borderTexels;
> +    if (yIndex < numTilesY() - 1)
> +        return m_maxTextureSize - 2 * m_borderTexels;

Same here.

> +void TilingData::intersectDrawQuad(const FloatRect& srcRect, const FloatRect& dstRect, int tile,
> +                                   FloatRect* newSrc, FloatRect* newDst) const
> +{

> +    *newSrc = srcRectIntersected;
> +    newSrc->move(
> +        -tileBounds.x() + m_borderTexels * (tileXIndex(tile) > 0), 
> +        -tileBounds.y() + m_borderTexels * (tileYIndex(tile) > 0));

This is doubly subtle since it's multiplying a bool by another bool and then using that in arithmetic.  This should be expanded out.

> diff --git a/WebCore/platform/graphics/chromium/TilingData.h b/WebCore/platform/graphics/chromium/TilingData.h
> +    bool m_borderTexels;

Could this be made more more obviously predicate?  Maybe m_hasBorderTexels.  From the name I'd guess this was a numerical value, not a boolean.

[David Levin]

(In reply to comment #14)
> (From update of attachment 64285 [details])
> > +    PassOwnPtr<Vector<unsigned int>> textureIds = new Vector<unsigned int>(numTiles);
> 
> "> >"

This should be OwnPtr instead of PassOwnPtr.

PassOwnPtr/OwnPtr are analogous to PassRefPtr/RefPtr (and you should never have a local variable which is a PassRefPtr -- http://webkit.org/coding/RefPtr.html).

[James Robinson]

Comment on attachment 64285 [details]
Patch

r- for the nits.  This feels very close!

[Vincent Scheib]

Created attachment 64361 [details]
Patch

[WebKit Review Bot]

Attachment 64361 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/tests/TilingDataTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vincent Scheib]

New patch:

"-" -> " - ".

">>" -> "> >".

> Doing math on a bool is a little too subtle

Reverted back to int with comment in header, rearranged logic involving booleans to use ? :.

"unsigned int" has 2 to 1 existing dominance in WebCore over "unsigned".

> Terse assignmenet
Done.

OwnPtr used instead of PassOwnPtr.

[WebKit Review Bot]

Attachment 64361 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3731139

[Vincent Scheib]

Created attachment 64429 [details]
Patch

[WebKit Review Bot]

Attachment 64429 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/tests/TilingDataTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Looks good to me!

[Dimitri Glazkov (Google)]

Comment on attachment 64429 [details]
Patch

if it's good enough for James, it's good enough for me.

[Vincent Scheib]

Comment on attachment 64429 [details]
Patch

commit-queue request

[WebKit Commit Bot]

Comment on attachment 64429 [details]
Patch

Rejecting patch 64429 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 64429, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=64429&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=43864&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 64429 from bug 43864.
ERROR: /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Vincent Scheib]

Created attachment 64509 [details]
Patch

[Vincent Scheib]

Comment on attachment 64509 [details]
Patch

Updated ChangeLog to enable commit-queue to pass.

[WebKit Review Bot]

Attachment 64509 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/tests/TilingDataTest.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 64509 [details]
Patch

Clearing flags on attachment: 64509

Committed r65455: <http://trac.webkit.org/changeset/65455>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Vincent Scheib]

Created attachment 64865 [details]
Patch

[David Levin]

Comment on attachment 64865 [details]
Patch

This bug has already been resolved/fixed. Please attach your patch to a new bug.

In fact, I think you already created a new bug for this issue :)