Last Comment Bug 725209 - BasicBufferOGL redraws it's entire content when it attempts to self copy.
: BasicBufferOGL redraws it's entire content when it attempts to self copy.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-07 22:19 PST by Matt Woodrow (:mattwoodrow)
Modified: 2012-02-15 09:18 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't redraw data that we copied from the old buffer (4.61 KB, patch)
2012-02-12 17:02 PST, Matt Woodrow (:mattwoodrow)
joe: review+
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2012-02-07 22:19:18 PST
We when try do a self copy with BasicBufferOGL, we allocate a new TextureImage and copy the contents across. This calls Resize() on the texture image, which sets the texture state to 'Allocated'. Then when we attempt to update this texture, it resets the draw area to that of the entire texture (via BasicTextureImage::GetUpdateRegion).

I tried just forcibly updating the texture state after doing the copy, but this sometimes gives us uninitialized memory showing through.

I'd suggest that self-copy of GL buffers has been broken since forever and we've always just happily redrawn the entire contents.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-07 23:09:32 PST
Gadzooks!
Comment 2 Matt Woodrow (:mattwoodrow) 2012-02-12 17:02:58 PST
Created attachment 596536 [details] [diff] [review]
Don't redraw data that we copied from the old buffer
Comment 3 Joe Drew (not getting mail) 2012-02-13 17:23:18 PST
Comment on attachment 596536 [details] [diff] [review]
Don't redraw data that we copied from the old buffer

Review of attachment 596536 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +603,5 @@
> +        if (mBufferRotation != nsIntPoint(0, 0)) {
> +        // If mBuffer is rotated, then BlitTextureImage will only be copying the bottom-right section
> +        // of the buffer. We need to invalidate the remaining sections so that they get redrawn too.
> +        // Alternatively we could teach BlitTextureImage to rearrange the rotated segments onto
> +        // the new buffer.

all these comments need to be indented or moved above

@@ +612,5 @@
> +                                   mBufferRect.y + mBufferRect.height - mBufferRotation.y);
> +
> +          // Mark the remaining quadrants (bottom-left&right, top-right) as invalid.
> +          nsIntRect bottom(mBufferRect.x, rotationPoint.y, mBufferRect.width, mBufferRotation.y);
> +          nsIntRect topright(rotationPoint.x, mBufferRect.y, mBufferRotation.x, rotationPoint.y - mBufferRect.y);

Maybe you could sketch out a little ASCII diagram so people can follow more easily?
Comment 4 Matt Woodrow (:mattwoodrow) 2012-02-15 01:29:26 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/858aab8397af
Comment 5 Marco Bonardo [::mak] 2012-02-15 09:18:11 PST
https://hg.mozilla.org/mozilla-central/rev/858aab8397af

Note You need to log in before you can comment on or make changes to this bug.