[Webkit-unassigned] [Bug 43864] Canvas2D does not support images larger than system's GPU max texture size
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 11 18:12:21 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=43864
--- Comment #7 from James Robinson <jamesr at chromium.org> 2010-08-11 18:12:21 PST ---
(From update of attachment 64177)
> 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 @@
> +2010-08-11 Vincent Scheib <scheib at chromium.org>
> +
> + 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 <aa at chromium.org>
>
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list