Last Comment Bug 634759 - Re-enable rotation of buffers using buffer-space rotation coordinates
: Re-enable rotation of buffers using buffer-space rotation coordinates
Status: RESOLVED FIXED
: mobile, perf
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Benjamin Stover (:stechz)
:
Mentors:
Depends on: 635373
Blocks: 650569 650589
  Show dependency treegraph
 
Reported: 2011-02-16 15:20 PST by Benjamin Stover (:stechz)
Modified: 2011-09-23 17:32 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10.25 KB, patch)
2011-02-24 02:22 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
Remove ExtendForScaling and stop pretending to clamp (1.78 KB, patch)
2011-02-24 11:47 PST, Benjamin Stover (:stechz)
no flags Details | Diff | Review
Part 2: Use buffer space coordinates for rotations (10.71 KB, patch)
2011-02-24 11:47 PST, Benjamin Stover (:stechz)
no flags Details | Diff | Review
Use buffer space coordinates for rotations (12.62 KB, patch)
2011-02-24 14:37 PST, Benjamin Stover (:stechz)
no flags Details | Diff | Review
Patch plus debug output of texture upload (2.89 KB, text/plain)
2011-08-22 17:20 PDT, Oleg Romashin (:romaxa)
no flags Details
Allow buffer rotation for shadowed layers (1.31 KB, patch)
2011-08-24 15:30 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Review
Allow buffer rotation for shadowed layers (1.28 KB, patch)
2011-08-24 15:31 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Review
Allow buffer rotation for shadowed layers (1.15 KB, patch)
2011-08-24 15:32 PDT, Benjamin Stover (:stechz)
roc: review+
Details | Diff | Review
Allow buffer rotation for shadowed layers (2.37 KB, patch)
2011-08-25 11:18 PDT, Benjamin Stover (:stechz)
roc: review+
Details | Diff | Review

Description Benjamin Stover (:stechz) 2011-02-16 15:20:10 PST
This causes corruption for repainting invalidated regions. Reverting this patch fixes the assertion and the corruption:

https://bugzilla.mozilla.org/attachment.cgi?id=511513&action=diff

(The corruption is much more noticeable with patch in bug 634397)
Comment 1 Matt Woodrow (:mattwoodrow) 2011-02-16 16:51:00 PST
This was caused by bug 633040. We no longer clamp the scale factor to an integer or reciprocal of an integer and hit this assertion.

Fixing this without the text regression is hard since we want to draw the text at the correct resolution (0.816300 for example) but to prevent seaming we need all points of the scaled region to be on integer boundaries.

Currently we snap the scale to a simple number and then extend the rects outwards until they will scale and remain as integers. I don't see a way to do this while still drawing at non-integer resolutions.

The only solution I see here is to represent the unscaled region in floating point numbers so that it can scale by 0.816300 to integer values.

We'd need a new nsRegion implementation for this and let layout accept this and pass it through to cairo (which does support floating point coords). Unsure of how much work this actually is.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-16 16:58:02 PST
Can't we just extend aRegionToDraw to a large enough size that it extends at least one pixel beyond the cliprect in every direction?
Comment 3 Matt Woodrow (:mattwoodrow) 2011-02-16 17:01:50 PST
Wouldn't we need to do that for every rect in the region to stop seaming between rects? And that would mean repainting the entire layer in most cases.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-16 18:10:54 PST
Benjamin, can you tell what causes the seams to appear? Is it copying from the ThebesLayer into the destination surface? Or some operation that draws into the ThebesLayer?
Comment 5 Benjamin Stover (:stechz) 2011-02-17 08:52:20 PST
I'm not sure; wouldn't they be related? If the clip that defines what we are drawing into ThebesLayer doesn't precisely match up with where the buffer gets rotated, it could be either the copying or the drawing. I think the seaming is caused by failing to match these up. If I read the code correctly, we clip the surface using surface units but we keep the rotation point in content units (for lack of a better term).

However, could there be a deeper issue like Matt suggests? Say we do pixel alignment to the surface. What happens when we draw a subpixel-aligned area A (relative to the content) and then scroll so that the new subpixel-aligned area we draw is now pasted adjacent A? Will that look right?

Matt was accomplishing pixel alignment both ways previously by clamping scale values (like .816 or similar for Google News) to whole numbers and blitting to the area, which made the text look bad. We landed the patch to stop clamping the resolution, which caused the assertion since resolution was no longer a whole number or a reciprocal of a whole number.

Maybe we could tweak his algorithm to clamp to values between .5 and 1? For instance, if we clamped to units of 10ths we only have to draw up to 10 extra pixels. We'd need a number N large enough s.t. clamping the res and scaling doesn't make funny text, but small enough s.t. we aren't repainting a lot of wasted pixels.

^^^ Late disclaimer: I don't understand this code well and I could be on the completely wrong track. :)
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-17 13:35:17 PST
I understand the problem better and now I need to think about it some more :-)
Comment 7 Benjamin Stover (:stechz) 2011-02-23 09:42:41 PST
Hey Roc, have you come up with anything? Even something that fixes the seaming issues might be fine for 4.0.

I've been playing with ThebesLayerBuffer storing its rotation and its dimensions in "surface space" to stop seaming, but there's still some major bug in my patch somewhere. If everyone else is busy, I am all ears for advice on how to fix this.
Comment 8 Benjamin Stover (:stechz) 2011-02-23 10:24:15 PST
For the record, with bug 634397 and removing the extendscaling line you can reproduce with test-ipcbrowser on google.com:

1. Set displayport to 0, 0, 600, 600
2. Set resolution to 0.8, 0.8
2. Set displayport to 0, 25, 600, 600
Comment 9 Benjamin Stover (:stechz) 2011-02-23 10:25:22 PST
Sorry, you also need to remove ClampToScaleFactor.
Comment 10 Benjamin Stover (:stechz) 2011-02-23 12:46:39 PST
OK, this is definitely a problem with a discrepancy between rounding out for the dimensions and rotating the buffer. For our example, since the numbers round so well, if we change ScaledSize to round instead of round out, the problem goes away.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-02-23 14:31:04 PST
blocks a blocker
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-23 19:01:09 PST
If a ThebesLayer's effective transform is a scale, we really really want to use a resolution that exactly matches the scale, so that no resampling will be performed, for performance and quality reasons. So I think we should strongly prefer solutions that allow arbitrary resolutions.

Here are the issues I know about with arbitrary resolutions:

1) Issues with drawing into a surface using arbitrary scale factors:
1a) Most layout drawing snaps rectangles to buffer pixel edges via gfx. This should not introduce any problems.
1b) For performance reasons layout includes snapping in its calculations of display item bounds, using its idea of device pixels. These calculations have to assume no extra scaling. So this optimization needs to be turned off if there could be extra scaling applied.
1c) There are some other places layout snaps based on its idea of device pixels, for example border widths. These affect layout and there's nothing we can do here.

2) Issues with the ThebesLayer region-to-draw
2a) The region-to-draw is computed by subtracting the valid region from the visible region. Everything in that region needs to be redrawn, but for performance reasons we need to clip to a region that's aligned at buffer pixel boundaries. We need to scale by the resolution rounding out, and then clip. This is OK since it's fine to repaint a bit more than we need.
2b) To ensure that whole area is painted by layout, we need to convert that region back to layer coordinates, rounding out again, passing the result into the DrawThebesLayer callback.

3) Issues with the ThebesLayerBuffer
3a) The "buffer rotation edges" need to be aligned at buffer pixel edges. Right now it isn't. (One way to do that, which is probably bad for performance, would be to force rotation off.)
3b) We need to make sure that the region-to-draw in buffer pixel space does not intersect the buffer rotation edges.
3c) When drawing a rotated buffer with resampling (i.e. an effective transform that's not an integer translation), we need to not be using rotation, otherwise we'll incorrectly sample across the rotation boundary. Bug 635373 should take care of this, and this need not be terrible for performance when zoomed since the desired common case is for the scale to cancel out the resolution so there is no resampling.

I think 1b, 2a, 2b, 3a and 3b need work.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-24 01:31:26 PST
I *think* this bug is mainly about 3a and possibly 3b
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-24 01:34:22 PST
It might be worth trying to fix 3a and 3b here simply by forcing mBufferRotation to be 0,0 when the x or y resolution are not 1.0. That will hurt performance a bit, but I'm not sure what else would work for arbitrary scale factors and scroll amounts.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-24 01:41:28 PST
Note that we should still be able to do a self-copy so disabling rotation isn't as bad as repainting everything.

If that hurts performance too much, then the next best thing I can think of is to introduce a new kind of buffer that uses tiles or something similar, for use when we have zooming.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-24 01:44:07 PST
... which is some amount of work, but not hard in principle. It could probably be done without changing the ThebesLayerBuffer interface.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-24 02:22:28 PST
Created attachment 514751 [details] [diff] [review]
patch

I'm going to attach this in bug 635373 because it fits better there with the other work I'm doing, but this alone should be enough to fix issues 3a and 3b.
Comment 18 Benjamin Stover (:stechz) 2011-02-24 10:04:27 PST
Thanks for this patch! I had a feeling that rotation needed to be stored in buffer space. I have a patch that does such; maybe it would be useful here?

I understand that this disables rotation, but I don't understand why we don't sometimes draw the entire visible region any more in this patch. But then again, I don't understand what happens with mRegionToInvalidate--what happens in what order after this function is called? Do we draw, then invalidate, then draw again? Do we skip the first draw? If so, the neededRegion stuff would be equivalent.

My question boils down to: how does neededRegion change the behavior of our rendering?

My other concern is that we don't have self copying on mobile now, so we are allocating a new shared memory buffer every time. There's another patch to fix this to use regular memory, so maybe we'll get accuracy *and* speed once everything is put together. :)
Comment 19 Benjamin Stover (:stechz) 2011-02-24 11:47:04 PST
Created attachment 514859 [details] [diff] [review]
Remove ExtendForScaling and stop pretending to clamp
Comment 20 Benjamin Stover (:stechz) 2011-02-24 11:47:14 PST
Created attachment 514860 [details] [diff] [review]
Part 2: Use buffer space coordinates for rotations
Comment 21 Benjamin Stover (:stechz) 2011-02-24 11:48:18 PST
Comment on attachment 514860 [details] [diff] [review]
Part 2: Use buffer space coordinates for rotations

This is my quick stab at using buffer space rotations. Very WIP, but it removes a lot of seaming for me.
Comment 22 Benjamin Stover (:stechz) 2011-02-24 11:48:30 PST
Comment on attachment 514860 [details] [diff] [review]
Part 2: Use buffer space coordinates for rotations

This is my quick stab at using buffer space rotations. Very WIP, but it removes a lot of seaming for me.
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-24 13:02:29 PST
Using buffer-space coordinates for rotations is a good idea. That's probably a better fix for 3a and 3b than what I have.

It looks like you've changed mBufferRect to be in buffer pixel space? Can you add comments to ThebesLayerBuffer.h to make it clear what you've changed?

I think this can work but I'll probably rebase your patch on top of the patches I have for 635373.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-02-24 13:44:39 PST
Comment on attachment 514751 [details] [diff] [review]
patch

Drive-by comments.

>   BasicThebesLayerBuffer(BasicThebesLayer* aLayer)
>     : Base(ContainsVisibleBounds)
>     , mLayer(aLayer)
>-  {}
>+  {
>+    mEnableRotationOptimization = PR_TRUE;

This needs to be disabled for shadowed thebes layers, per our discussion.  It's a little awkward to do because the thebes layer doesn't know it's shadowed until after its ctor.

> private:
>   BasicThebesLayerBuffer(gfxASurface* aBuffer,
>                          const nsIntRect& aRect, const nsIntPoint& aRotation)
>     // The size policy doesn't really matter here; this constructor is
>     // intended to be used for creating temporaries
>     : ThebesLayerBuffer(ContainsVisibleBounds)
>   {
>+    mEnableRotationOptimization = PR_TRUE;

It doesn't matter whether this is true or false, since DrawTo() and DrawBufferWithRotation() don't care, but I guess it's safer to leave this true.
Comment 25 Benjamin Stover (:stechz) 2011-02-24 13:48:11 PST
Changing this bug to be about re-enabling rotation buffers that roc says will be disabled by bug 635373.
Comment 26 Benjamin Stover (:stechz) 2011-02-24 14:37:00 PST
Created attachment 514922 [details] [diff] [review]
Use buffer space coordinates for rotations
Comment 27 Benjamin Stover (:stechz) 2011-02-24 14:38:31 PST
Comment on attachment 514751 [details] [diff] [review]
patch

Putting roc's patch as obsolete since it is in bug 635373. The new attachment is based off of roc's patch and adds documentation about mBufferRect to the header file.
Comment 28 Brad Lassey [:blassey] (use needinfo?) 2011-03-01 09:35:30 PST
Comment on attachment 514922 [details] [diff] [review]
Use buffer space coordinates for rotations

is this ready for review?
Comment 29 Benjamin Stover (:stechz) 2011-03-01 09:54:42 PST
No, its dependency bug needs to stabilize first because it is based on top of that.
Comment 30 Benjamin Stover (:stechz) 2011-03-02 14:50:12 PST
Nice to have, but Chris and I don't think this should block release anymore. Chris's self-copy should be fast enough for 4.0. Feel free to renom if you disagree.
Comment 31 Chris Lord [:cwiiis] 2011-07-27 04:13:33 PDT
Is this still being considered? I've been having a look at performance with GL layers on Android, and because we don't have a fast-path swap/copy, not using rotation is an issue.

If you stop setting the PAINT_WILL_RESAMPLE flag (and apply the patch in bug #674494), GL layers performance on android really flies, compared to how it is otherwise.

Of course, it also shows up that tiled rendering + rotation doesn't work at the moment, but that's a separate issue (that I'll take a look at).
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-27 11:17:27 PDT
The problem here is, shadow layers may always be resampled by the chrome process (even on desktop, with drawWindow()).  If some of those shadow thebes layers have a buffer rotation, then resampling across the rotation will lead to artifacts, in general.  That's why rotation is disabled for shadow layers.

If we re-enable rotation, then we save a memcpy in the content process (the buffer "self copy") that's off the critical UI painting path (i.e. don't block the UI).  That's good, saves on memory bandwidth.  I wouldn't expect that to have a huge impact on panning fps, but it certainly might.

But, with GL layers, even if we re-enable rotation, we're still going to pay the cost of a full texture upload on each update, because we're not doing partial uploads.  Those uploads are on the critical path (block the UI).

So I think there are some interesting things to check here
 (1) Leave PAINT_WILL_RESAMPLE on, but comment out the self-copy (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ThebesLayerBuffer.cpp#247).  That should estimate whether the savings in memory bandwidth is improving chrome fps.
 (2) Check to make sure that, when PAINT_WILL_RESAMPLE is enabled, that we're *only* repainting the area outside what was self-copied.  This may have regressed; we don't have good automated tests for this yet.  If we're repainting more than we need, that would be a big find and a huge easy win.
 (3) As in the texture-upload bug, it would be interesting to measure the cost of partial-update vs. full-update.

We have some options for re-enabling buffer rotation (one option is just to live with resampling artifacts), but let's find out whether the improvement you're seeing is coming from (1) or (2) first.
Comment 33 Chris Lord [:cwiiis] 2011-07-27 11:28:52 PDT
(In reply to comment #32)
> The problem here is, shadow layers may always be resampled by the chrome
> process (even on desktop, with drawWindow()).  If some of those shadow
> thebes layers have a buffer rotation, then resampling across the rotation
> will lead to artifacts, in general.  That's why rotation is disabled for
> shadow layers.

I realise this, but for the case where artifacts would be noticeable, can't we just re-render with no rotation? Also, the shadow thebes layer buffer rotation is mirrored, so I don't really get why this applies specifically to shadow layers and not other types of layer?

> If we re-enable rotation, then we save a memcpy in the content process (the
> buffer "self copy") that's off the critical UI painting path (i.e. don't
> block the UI).  That's good, saves on memory bandwidth.  I wouldn't expect
> that to have a huge impact on panning fps, but it certainly might.

While I don't have numbers, if you enable rotation and remove the resample checks, the fluidity of panning is noticeably better even on my Core i5 laptop. The difference is huge on a Tegra 2 tablet.

> But, with GL layers, even if we re-enable rotation, we're still going to pay
> the cost of a full texture upload on each update, because we're not doing
> partial uploads.  Those uploads are on the critical path (block the UI).

We do have partial uploads though? DirectUpdate is called with a destRegion that's derived from the updated area, and it does indeed do a partial update.

> So I think there are some interesting things to check here
>  (1) Leave PAINT_WILL_RESAMPLE on, but comment out the self-copy
> (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ThebesLayerBuffer.
> cpp#247).  That should estimate whether the savings in memory bandwidth is
> improving chrome fps.

Given that we zoom to fit width by default, there are practically no situations where we don't resample. Perhaps we need to re-think this check (and possibly how we set translation coordinates).

>  (2) Check to make sure that, when PAINT_WILL_RESAMPLE is enabled, that
> we're *only* repainting the area outside what was self-copied.  This may
> have regressed; we don't have good automated tests for this yet.  If we're
> repainting more than we need, that would be a big find and a huge easy win.
>  (3) As in the texture-upload bug, it would be interesting to measure the
> cost of partial-update vs. full-update.

Ok, it looks like I'm going to have to present numbers to convince anyone about this :)

> We have some options for re-enabling buffer rotation (one option is just to
> live with resampling artifacts), but let's find out whether the improvement
> you're seeing is coming from (1) or (2) first.

We get practically nothing from (1) due to needing to resample in almost every case, and (2) would only save time for content rendering (which is nice, but oughtn't have much effect on panning speed?)
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-27 11:55:46 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > The problem here is, shadow layers may always be resampled by the chrome
> > process (even on desktop, with drawWindow()).  If some of those shadow
> > thebes layers have a buffer rotation, then resampling across the rotation
> > will lead to artifacts, in general.  That's why rotation is disabled for
> > shadow layers.
> 
> I realise this, but for the case where artifacts would be noticeable, can't
> we just re-render with no rotation? Also, the shadow thebes layer buffer
> rotation is mirrored, so I don't really get why this applies specifically to
> shadow layers and not other types of layer?
>

Content processes don't ever know when resampling artifacts might occur in chrome, because they don't know what's going on in chrome.  Chrome doesn't know that resampling artifacts might happen until it decides to paint with a scale that mismatches the shadow layers' (nowadays, that might be just "non-identity scale").  If chrome gets to that point, then it has two options in the current code: ask content to rerender and wait for new pixels before drawing with scale (bad perceived responsiveness), draw with scale and suffer artifacts.

> > If we re-enable rotation, then we save a memcpy in the content process (the
> > buffer "self copy") that's off the critical UI painting path (i.e. don't
> > block the UI).  That's good, saves on memory bandwidth.  I wouldn't expect
> > that to have a huge impact on panning fps, but it certainly might.
> 
> While I don't have numbers, if you enable rotation and remove the resample
> checks, the fluidity of panning is noticeably better even on my Core i5
> laptop. The difference is huge on a Tegra 2 tablet.

I believe you.  What does the fps meter say?  The more quantitative and repeatable the measurement, the easier it is to tell when we've fixed the bug.  If the fps meter isn't the right tool, let's build one.

> > But, with GL layers, even if we re-enable rotation, we're still going to pay
> > the cost of a full texture upload on each update, because we're not doing
> > partial uploads.  Those uploads are on the critical path (block the UI).
> 
> We do have partial uploads though? DirectUpdate is called with a destRegion
> that's derived from the updated area, and it does indeed do a partial update.

Great!  That didn't exist last I was in this code.

> > So I think there are some interesting things to check here
> >  (1) Leave PAINT_WILL_RESAMPLE on, but comment out the self-copy
> > (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ThebesLayerBuffer.
> > cpp#247).  That should estimate whether the savings in memory bandwidth is
> > improving chrome fps.
> 
> Given that we zoom to fit width by default, there are practically no
> situations where we don't resample. Perhaps we need to re-think this check
> (and possibly how we set translation coordinates).

No, there's a difference between rendering at non-identity resolution in the content process (fit-width causes that, among other things) and resampling in chrome (pinch-zoom animations cause that, among other things).  The former case is fine, because the chrome process won't apply a scale when painting.  The latter case is not fine, because chrome will apply a scale.  Does that make sense?

> 
> >  (2) Check to make sure that, when PAINT_WILL_RESAMPLE is enabled, that
> > we're *only* repainting the area outside what was self-copied.  This may
> > have regressed; we don't have good automated tests for this yet.  If we're
> > repainting more than we need, that would be a big find and a huge easy win.
> >  (3) As in the texture-upload bug, it would be interesting to measure the
> > cost of partial-update vs. full-update.
> 
> Ok, it looks like I'm going to have to present numbers to convince anyone
> about this :)

I believe you, but quantitative measurements help everyone, including yourself ;).

> 
> > We have some options for re-enabling buffer rotation (one option is just to
> > live with resampling artifacts), but let's find out whether the improvement
> > you're seeing is coming from (1) or (2) first.
> 
> We get practically nothing from (1) due to needing to resample in almost
> every case,

Resampling where?

> and (2) would only save time for content rendering (which is
> nice, but oughtn't have much effect on panning speed?)

Correct, in theory.  Repainting too much will chew memory bandwidth and CPU, which shouldn't have a big effect on chrome-process panning but it's worth checking if only for the rerender delay.
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-31 22:34:21 PDT
(In reply to comment #34)
> Content processes don't ever know when resampling artifacts might occur in
> chrome, because they don't know what's going on in chrome.  Chrome doesn't
> know that resampling artifacts might happen until it decides to paint with a
> scale that mismatches the shadow layers' (nowadays, that might be just
> "non-identity scale").  If chrome gets to that point, then it has two
> options in the current code: ask content to rerender and wait for new pixels
> before drawing with scale (bad perceived responsiveness), draw with scale
> and suffer artifacts.

There's another option: render the layer's rotated buffer in such a way that artifacts don't occur, e.g. by making an unrotated copy and rendering that. I think you could even be clever about figuring out the area that could be affected by artifacts --- zones around the rotation boundary, basically --- so you can draw the areas outside those zones directly, then copy just enough to paint the danger zones without artifacts.
Comment 36 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-01 10:30:33 PDT
Right, I was careful to say "two options in the current code" ;).  "Optimizing away" the temporary surface is an interesting idea.  That opportunity should arise quite a bit on fennec.
Comment 37 Oleg Romashin (:romaxa) 2011-08-22 16:48:32 PDT
I have disabled ThebesLayerBuffer::PAINT_WILL_RESAMPLE set in all places, and see content rendered without any artifacts, with rotation working, and partial updates on UI side... so while scrolling we do partial updates 20-30 pixel height chunks...

This looks pretty good for me, because we don't spend ages for rendering 2000px height buffer in child-process side, and do not upload same huge buffer on UI side (I was testing on N9 with eglImageLock direct rendering method...)
Comment 38 Oleg Romashin (:romaxa) 2011-08-22 17:20:56 PDT
Created attachment 554996 [details]
Patch plus debug output of texture upload
Comment 39 Oleg Romashin (:romaxa) 2011-08-22 17:28:20 PDT
Comment on attachment 554996 [details]
Patch plus debug output of texture upload

I tried different pages with different scaling, and haven't found any visible artifacts...
Comment 40 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-22 17:38:30 PDT
You're probably testing on pages where the top of the buffer and bottom of the buffer (wrt rotation) are both nearly the same color, so you're not seeing artifacts from resampling across the boundary.  I would expect that to be a pretty common case.

You could make artifacts occur using the test-ipcbrowser.xul harness.  You want something like
 - create a test page that's a gradient from one color to its complement, like red<-->green
 - render at offset <0,0>
 - force a scroll to <0,h/2>, where h is the displayport height
 - change the chrome scale to 2.0

You should see interesting things at the bottom of the window.
Comment 41 Oleg Romashin (:romaxa) 2011-08-22 18:57:47 PDT
I did exactly this, also loaded the page in fennec, and don't see any artifacts not in test.xul not in fennec
Here is test gradient page...
http://pastebin.mozilla.org/1308215
Comment 42 Oleg Romashin (:romaxa) 2011-08-22 19:05:10 PDT
Everything looks perfect, but I've noticed that even with this patch we still have 1-2 full viewport updates... sometime on scroll start, and always on scroll finished.. if we can remove that, then I guess we are not going to see checkerboard in ideal cases anymore...
Comment 43 Oleg Romashin (:romaxa) 2011-08-22 19:20:58 PDT
Ok, full repaint caused by sendDisplayportUpdate/updateCSSViewport call... can we make that working without force full update?
Comment 44 Oleg Romashin (:romaxa) 2011-08-23 01:24:17 PDT
Created new bug about fullUpdate on domWindow::scrollTo , bug 681192

Also created try build with rotation enabled for mobile builds:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=f9264a92db95

Also tested rotation stuff without backingSurface (ImageLock) and still works fine without artifacts...
Comment 45 Chris Lord [:cwiiis] 2011-08-23 01:35:58 PDT
I've been testing for a while with rotation forced on (on android and desktop) and hadn't noticed any artifacing, but I assume it's because at various points we end up forcing full-buffer updates via other means so you don't get a chance to see them...

If you create a page that is just a large cross-hatch (like a full page of this: http://chrislord.net/images/misaligned-sample-grid.png), I'd have thought you'd be able to see the artifacing, but I've not tried it out yet.

Personally, I think if we force a full-buffer update when scrolling is finished (and make this purposeful, rather than a side-effect), we should just enable this - the performance benefit is pretty big.
Comment 46 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 04:45:29 PDT
(In reply to Chris Lord [:cwiiis] from comment #45)
> Personally, I think if we force a full-buffer update when scrolling is
> finished (and make this purposeful, rather than a side-effect),

I don't think we need to do that. In the steady state (and assuming no CSS transforms), the ThebesLayer resolution is set so that the compositor doesn't need to resample the layers, so these artifacts will not occur.
Comment 47 Florian Hänel [:heeen] 2011-08-23 04:57:16 PDT
Am I right in saying these artifacts can only occur during panning *plus* pinch zooming? Because if so, we are showing the user an intermediate image anyways. I wouldn't worry about artifacts too much in this case, since all it takes is that the user stops zooming and it gets re-rendered at the new resolution anyways.

I think the story is as follows:
1) User loads page, text is to small due to layout or default initial zoom.
2) User magnifies to desired zoom level to be able to read, at this point the user doesn't care that there are some bands, he waits for the re-render at the right resolution anyways.
3) Page re-renders to accommodate for zoom. Artifacts vanish as buffer resolution matches display resolution again and rotation is ironed out.
Comment 48 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 04:59:09 PDT
(In reply to Florian Hänel [:heeen] from comment #47)
> I think the story is as follows:
> 1) User loads page, text is to small due to layout or default initial zoom.
> 2) User magnifies to desired zoom level to be able to read, at this point
> the user doesn't care that there are some bands, he waits for the re-render
> at the right resolution anyways.
> 3) Page re-renders to accommodate for zoom. Artifacts vanish as buffer
> resolution matches display resolution again and rotation is ironed out.

That's correct. However, panning is not required to see the artifacts.
Comment 49 Florian Hänel [:heeen] 2011-08-23 05:06:42 PDT
I thought the initial state after the back buffer has been painted to match the rendered resolution is that the display port is centered around the viewport? so rotation only happens after you panned a bit. You're probably right that is not required to happen simultaneously, though.
Comment 50 Benjamin Stover (:stechz) 2011-08-24 09:58:09 PDT
So, I think we on the Fennec side are perfectly OK with dealing with temporary issues with compositing for scaled images. Roc, could we land a patch and see what it is like on the nightlies?
Comment 51 Benjamin Stover (:stechz) 2011-08-24 15:30:46 PDT
Created attachment 555571 [details] [diff] [review]
Allow buffer rotation for shadowed layers
Comment 52 Benjamin Stover (:stechz) 2011-08-24 15:31:27 PDT
Created attachment 555572 [details] [diff] [review]
Allow buffer rotation for shadowed layers
Comment 53 Benjamin Stover (:stechz) 2011-08-24 15:32:15 PDT
Created attachment 555573 [details] [diff] [review]
Allow buffer rotation for shadowed layers
Comment 54 Benjamin Stover (:stechz) 2011-08-24 15:33:22 PDT
Comment on attachment 555573 [details] [diff] [review]
Allow buffer rotation for shadowed layers

Something like this?
Comment 55 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-24 19:21:21 PDT
Comment on attachment 555573 [details] [diff] [review]
Allow buffer rotation for shadowed layers

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

Sure
Comment 56 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-24 19:26:31 PDT
No need to #ifndef MOZ_GFX_OPTIMIZE_MOBILE this.  It won't affect desktop.
Comment 57 Chris Lord [:cwiiis] 2011-08-25 08:30:26 PDT
Comment on attachment 555573 [details] [diff] [review]
Allow buffer rotation for shadowed layers

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +707,5 @@
>      if (!GetEffectiveTransform().Is2D(&transform) ||
> +#ifndef MOZ_GFX_OPTIMIZE_MOBILE
> +        MustRetainContent() /*<=> has shadow layer*/ ||
> +#endif
> +        transform.HasNonIntegerTranslation()) {

I found that even disabling this check, transform.HasNonIntegerTranslation() almost always returns true - do you still find this, or has this problem disappeared? I wonder if we should just ditch the PAINT_WILL_RESAMPLE flag altogether, or ignore it in ThebesLayerBuffer.cpp...
Comment 58 Oleg Romashin (:romaxa) 2011-08-25 08:40:26 PDT
We need to fix this bug 681192, + implement scale values adjustment in order to avoid HasNonIntegerTranslation problem
Comment 59 Benjamin Stover (:stechz) 2011-08-25 11:18:49 PDT
Created attachment 555795 [details] [diff] [review]
Allow buffer rotation for shadowed layers
Comment 60 Benjamin Stover (:stechz) 2011-08-25 11:20:52 PDT
Comment on attachment 555795 [details] [diff] [review]
Allow buffer rotation for shadowed layers

Since getting the integer translation right seems to be giving us trouble, are you okay with just flat out disabling PAINT_WILL_RESAMPLE on mobile?
Comment 61 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-25 17:48:58 PDT
Comment on attachment 555795 [details] [diff] [review]
Allow buffer rotation for shadowed layers

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

I am if you are!

I'm pretty sure this will cause artifacts, but hopefully they're not very noticeable. I've thought about this a bit and I think the only pixels that will show artifacts will be right at the edge of the checkboard area, so hopefully the user doesn't see that often or for long.
Comment 62 Oleg Romashin (:romaxa) 2011-08-25 20:17:07 PDT
Btw, not sure but for such kind of non-certain fix probably make sense to add some env variable which could help us identify and duplicate bugs
Comment 63 Benjamin Stover (:stechz) 2011-08-29 11:00:47 PDT
Landed as is for now to inbound. http://hg.mozilla.org/integration/mozilla-inbound/rev/5e6848a5ca2a

Let's run this for a few nights and see if anything has complaints about artifacting.
Comment 64 Benjamin Stover (:stechz) 2011-08-29 12:09:29 PDT
philor found some unexpected passes now:
http://tbpl.allizom.org/php/getParsedLog.php?id=6175711#error0

I'm assuming these are permanent and we'll have to back out and figure these out. I wasn't expected things to pass more than usual. :)
Comment 65 Benjamin Stover (:stechz) 2011-08-29 13:44:56 PDT
After talking to philor, we decided to just mark the tests as expected passing now. Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b9905024a884
Comment 66 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-29 20:17:57 PDT
If you need more data on what's happening with an unexpected pass, you can change an == test to a != test (or vice-versa), then either run the reftest or push to try, and then use layout/tools/reftest/reftest-analyzer.xhtml on the log to see what's being compared.
Comment 67 Ed Morley [:emorley] 2011-08-30 04:43:47 PDT
http://hg.mozilla.org/mozilla-central/rev/5e6848a5ca2a

Given comment 64 and comment 65, not sure if this should be closed yet. Please close if in fact appropriate - thanks :-)
Comment 68 Ed Morley [:emorley] 2011-08-30 04:52:45 PDT
Followup marking tests as expected passing:
http://hg.mozilla.org/mozilla-central/rev/b9905024a884
Comment 69 Benjamin Stover (:stechz) 2011-08-30 09:11:18 PDT
Thanks Ed! I'm going to go ahead and close this. If we see any rendering issues on Android, we will need to back out, but so far so good for me.
Comment 70 Benjamin Stover (:stechz) 2011-08-30 09:13:00 PDT
Any unacceptable rendering issues, that is.
Comment 71 Marco Bonardo [::mak] 2011-08-31 01:55:20 PDT
annotations cleanup by philor http://hg.mozilla.org/mozilla-central/rev/f99e149090f5
Comment 72 Brad Lassey [:blassey] (use needinfo?) 2011-09-23 11:06:39 PDT
is this a permanent  fix or should there be a follow up bug to figure out why PAINT_WILL_RESAMPLE was causing issues on mobile?
Comment 73 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-23 17:32:41 PDT
There's really to "fix" here, it's a tradeoff between small transient artifacts and rerender speed.  ISTR that Bas had some ideas for getting rid of the resampling artifacts from scale-with-rotate-buffer, but it's more rocket-science-y.  Bas, did you file a bug for that?

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