Bug 83424

Created attachment 136141 [details] Patch

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?

(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

Created attachment 136175 [details] Patch

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? :-)

(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.

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

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.

Created attachment 136232 [details] Patch for landing

Created attachment 136234 [details] Patch for landing

Comment on attachment 136234 [details] Patch for landing Removed from commit-queue so that andersca could take a look if interested.

(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).

Created attachment 136286 [details] Patch

(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 on attachment 136286 [details] Patch Attachment 136286 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12369313

Created attachment 136383 [details] Patch for landing

Comment on attachment 136383 [details] Patch for landing Clearing flags on attachment: 136383 Committed r113678: <http://trac.webkit.org/changeset/113678>

All reviewed patches have been landed. Closing bug.

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

(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.