Bug 83424

Summary: [WK2] Enable using a single ShareableBitmap for multiple updates
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: Layout and RenderingAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, igor.oliveira, jturcotte, laszlo.gombos, menard, mrobinson, ossy, ostap73, webkit.review.bot, zoltan
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 83582    
Bug Blocks: 78675    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch
none
Patch for landing none

Description Noam Rosenthal 2012-04-07 09:57:06 PDT
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.
Comment 1 Noam Rosenthal 2012-04-07 18:26:44 PDT
Created attachment 136141 [details]
Patch
Comment 2 Igor Trindade Oliveira 2012-04-07 19:06:45 PDT
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?
Comment 3 Noam Rosenthal 2012-04-08 09:00:13 PDT
(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
Comment 4 Noam Rosenthal 2012-04-08 20:50:45 PDT
Created attachment 136175 [details]
Patch
Comment 5 Kenneth Rohde Christiansen 2012-04-09 06:47:56 PDT
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? :-)
Comment 6 Noam Rosenthal 2012-04-09 07:03:59 PDT
(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.
Comment 7 Noam Rosenthal 2012-04-09 07:04:30 PDT
> > Maybe RepaintAtlas would be a better name?
> It's related to UpdateAtlas.
I mean UpdateInfo. Repaint is more of a command.
Comment 8 Martin Robinson 2012-04-09 08:37:06 PDT
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.
Comment 9 Noam Rosenthal 2012-04-09 09:19:42 PDT
Created attachment 136232 [details]
Patch for landing
Comment 10 Noam Rosenthal 2012-04-09 09:22:27 PDT
Created attachment 136234 [details]
Patch for landing
Comment 11 Noam Rosenthal 2012-04-09 10:06:28 PDT
Comment on attachment 136234 [details]
Patch for landing

Removed from commit-queue so that andersca could take a look if interested.
Comment 12 Noam Rosenthal 2012-04-09 10:16:50 PDT
(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).
Comment 13 Noam Rosenthal 2012-04-09 12:37:59 PDT
Created attachment 136286 [details]
Patch
Comment 14 Noam Rosenthal 2012-04-09 12:39:09 PDT
(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.
Comment 15 Early Warning System Bot 2012-04-09 13:04:07 PDT
Comment on attachment 136286 [details]
Patch

Attachment 136286 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12369313
Comment 16 Noam Rosenthal 2012-04-09 20:02:28 PDT
Created attachment 136383 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-04-09 21:57:49 PDT
Comment on attachment 136383 [details]
Patch for landing

Clearing flags on attachment: 136383

Committed r113678: <http://trac.webkit.org/changeset/113678>
Comment 18 WebKit Review Bot 2012-04-09 21:57:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Csaba Osztrogon√°c 2012-04-10 08:14:15 PDT
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
Comment 20 Noam Rosenthal 2012-04-10 08:16:02 PDT
(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.