Bug 83424 – [WK2] Enable using a single ShareableBitmap for multiple updates

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Creating a ShareableBitmap for each content update could be inefficient, as it requires shm allocation/deallocation.
We should enable a configuration where very few larger ShareableBitmaps are created, and are used as atlases for small updates.

[Noam Rosenthal]

Created attachment 136141 [details]
Patch

[Igor Trindade Oliveira]

Comment on attachment 136141 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:446
> +    glPixelStorei(GL_UNPACK_ROW_LENGTH, bytesPerLine / 4);

by definition TextureMapperGL is a "An OpenGL-ES2 implementation of TextureMapper". GL_UNPACK_ROW_LENGTH is not supported by default in opengles, it needs the GL_EXT_unpack_subimage extension.
However i think we should not care about this problem for now. Just put a XXX or FIXME :).

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:38
> +static int nextPowerOfTwo(int number)

I think there is a trick to make nextPowerOfTwo simpler than this for 32 bits integer.

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:48
> +static const int BufferIsInUse = 1;

maybe an enumerator?

[Noam Rosenthal]

(In reply to comment #2)
> by definition TextureMapperGL is a "An OpenGL-ES2 implementation of TextureMapper". GL_UNPACK_ROW_LENGTH is not supported by default in opengles, it needs the GL_EXT_unpack_subimage extension.
> However i think we should not care about this problem for now. Just put a XXX or FIXME :).
I care about this :)
I'll add a proper test and add an image-copy fallback when I get to test this on ES.

> 
> > Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:38
> > +static int nextPowerOfTwo(int number)
> 
> I think there is a trick to make nextPowerOfTwo simpler than this for 32 bits integer.
Sure, I can use that... tried to keep it straightforward since it's not code that gets called a lot.

> 
> > Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:48
> > +static const int BufferIsInUse = 1;
> 
> maybe an enumerator?
OK

[Noam Rosenthal]

Created attachment 136175 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 136175 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Modify BitmapTexture::updateContets to include a source offset.

Contents*

> Source/WebKit2/ChangeLog:12
> +        In this iteration, UpdateAtlas has a simple behavior where a rect inside the bitmap is

Maybe RepaintAtlas would be a better name?

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:70
> +    virtual void updateContents(const void*, const IntRect& target, const IntPoint& offset, int bytesPerLine) = 0;

atlasOffset?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:459
> +    // For ES drivers that don't support sub-imaages, transfer the pixels row-by-row.

images*

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-444
> +    if (!driverSupportsSubImage()) {
> +        const char* bits = static_cast<const char*>(data);
> +        for (int y = 0; y < targetRect.height(); ++y) {
> +            const char *row = bits + ((sourceOffset.y() + y) * bytesPerLine + sourceOffset.x() * 4);
> +            GL_CMD(glTexSubImage2D(GL_TEXTURE_2D, 0, targetRect.x(), targetRect.y() + y, targetRect.width(), 1, glFormat, GL_UNSIGNED_BYTE, row))
> +        }
> +        return;
>      }
>  
> -    GL_CMD(glTexSubImage2D(GL_TEXTURE_2D, 0, targetRect.x(), targetRect.y(), targetRect.width(), targetRect.height(), glFormat, GL_UNSIGNED_BYTE, data))

Why do you need to do it per line now, you didn't seem to do that before

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.h:90
> +    virtual void updateContents(const void*, const IntRect& target, const IntPoint& sourceOffset, int bytesPerLine);

updateContentsAtlas?

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:53
> +        InUse

Taken? :-)

[Noam Rosenthal]

(In reply to comment #5)
> (From update of attachment 136175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136175&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Modify BitmapTexture::updateContets to include a source offset.
> 
> Contents*
> 
> > Source/WebKit2/ChangeLog:12
> > +        In this iteration, UpdateAtlas has a simple behavior where a rect inside the bitmap is
> 
> Maybe RepaintAtlas would be a better name?
It's related to UpdateAtlas.
My first choice was ScratchBufferAtlas. (The word ScratchBuffer is used in other places in webkit as well).

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapper.h:70
> > +    virtual void updateContents(const void*, const IntRect& target, const IntPoint& offset, int bytesPerLine) = 0;
> 
> atlasOffset?
Maybe sourceOffset. Atlas is a term only used in the web-process, I don't want to introduce it to TextureMapper.

> > -    GL_CMD(glTexSubImage2D(GL_TEXTURE_2D, 0, targetRect.x(), targetRect.y(), targetRect.width(), targetRect.height(), glFormat, GL_UNSIGNED_BYTE, data))
> 
> Why do you need to do it per line now, you didn't seem to do that before
Before we uploaded full images (with no gaps) to the GPU. With the atlas approach, we upload a partial rect from the source image; This requires a sub-image extension, which is not guaranteed on ES.

[Noam Rosenthal]

> > Maybe RepaintAtlas would be a better name?
> It's related to UpdateAtlas.
I mean UpdateInfo. Repaint is more of a command.

[Martin Robinson]

Comment on attachment 136175 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:500
> +    bytesPerLine= qtImage.bytesPerLine();

Nit: Missing a space after bytesPerLine here.

[Noam Rosenthal]

Created attachment 136232 [details]
Patch for landing

[Noam Rosenthal]

Created attachment 136234 [details]
Patch for landing

[Noam Rosenthal]

Comment on attachment 136234 [details]
Patch for landing

Removed from commit-queue so that andersca could take a look if interested.

[Noam Rosenthal]

(In reply to comment #11)
> (From update of attachment 136234 [details])
> Removed from commit-queue so that andersca could take a look if interested.

(note that this does not change any existing behavior - only adding a function to ShareableBitmap for painting into an internal-rect).

[Noam Rosenthal]

Created attachment 136286 [details]
Patch

[Noam Rosenthal]

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

Kenneth, could you re-review? It's a minor change from the last patch.

[Early Warning System Bot]

Comment on attachment 136286 [details]
Patch

Attachment 136286 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12369313

[Noam Rosenthal]

Created attachment 136383 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 136383 [details]
Patch for landing

Clearing flags on attachment: 136383

Committed r113678: <http://trac.webkit.org/changeset/113678>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It caused a regression (crash!), could you check it?
I filed a new bug about it - https://bugs.webkit.org/show_bug.cgi?id=83582

[Noam Rosenthal]

(In reply to comment #19)
> It caused a regression (crash!), could you check it?
> I filed a new bug about it - https://bugs.webkit.org/show_bug.cgi?id=83582
Looks unrelated; I will look later today. Please dont roll out.