[Webkit-unassigned] [Bug 43864] Canvas2D does not support images larger than system's GPU max texture size

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43864





--- Comment #11 from Vincent Scheib <scheib at chromium.org>  2010-08-12 17:14:43 PST ---
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));

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.