Closed Bug 634759 Opened 14 years ago Closed 14 years ago

Re-enable rotation of buffers using buffer-space rotation coordinates

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla9

People

(Reporter: stechz, Assigned: stechz)

References

Details

(Keywords: mobile, perf)

Attachments

(3 files, 6 obsolete files)

Use buffer space coordinates for rotations
12.62 KB, patch
Details | Diff | Splinter Review
Patch plus debug output of texture upload
2.89 KB, text/plain
Details
Allow buffer rotation for shadowed layers
2.37 KB, patch
roc
: review+
Details | Diff | Splinter Review
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)
Blocks: 634397
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.
Can't we just extend aRegionToDraw to a large enough size that it extends at least one pixel beyond the cliprect in every direction?
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.
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?
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. :)
I understand the problem better and now I need to think about it some more :-)
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.
tracking-fennec: --- → ?
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
Sorry, you also need to remove ClampToScaleFactor.
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.
Assignee: nobody → ben
tracking-fennec: ? → 2.0+
blocks a blocker
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.
I *think* this bug is mainly about 3a and possibly 3b
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.
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.
... which is some amount of work, but not hard in principle. It could probably be done without changing the ThebesLayerBuffer interface.
Attached patch patch (obsolete) — — Splinter Review
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.
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 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.
Attachment #514860 - Flags: review?(roc)
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.
Attachment #514860 - Flags: feedback?(roc)
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 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.
No longer blocks: 634397
Changing this bug to be about re-enabling rotation buffers that roc says will be disabled by bug 635373.
Depends on: 635373
Summary: Assertion: Multiplication factors must be integers or 1/integer → Re-enable rotation of buffers using buffer-space rotation coordinates
Attachment #514860 - Attachment is obsolete: true
Attachment #514860 - Flags: review?(roc)
Attachment #514860 - Flags: feedback?(roc)
Attachment #514859 - Attachment is obsolete: true
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.
Attachment #514751 - Attachment is obsolete: true
Comment on attachment 514922 [details] [diff] [review] Use buffer space coordinates for rotations is this ready for review?
No, its dependency bug needs to stabilize first because it is based on top of that.
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.
tracking-fennec: 2.0+ → ---
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).
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.
(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?)
(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.
(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.
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.
Assignee: ben → nobody
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 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...
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.
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
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...
Ok, full repaint caused by sendDisplayportUpdate/updateCSSViewport call... can we make that working without force full update?
Keywords: mobile, perf
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...
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.
(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.
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.
(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.
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.
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?
Attachment #555571 - Attachment is obsolete: true
Attachment #555572 - Attachment is obsolete: true
Comment on attachment 555573 [details] [diff] [review] Allow buffer rotation for shadowed layers Something like this?
Attachment #555573 - Flags: review?(roc)
Comment on attachment 555573 [details] [diff] [review] Allow buffer rotation for shadowed layers Review of attachment 555573 [details] [diff] [review]: ----------------------------------------------------------------- Sure
Attachment #555573 - Flags: review?(roc) → review+
No need to #ifndef MOZ_GFX_OPTIMIZE_MOBILE this. It won't affect desktop.
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...
We need to fix this bug 681192, + implement scale values adjustment in order to avoid HasNonIntegerTranslation problem
Attachment #555573 - Attachment is obsolete: true
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?
Attachment #555795 - Flags: review?(roc)
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.
Attachment #555795 - Flags: review?(roc) → review+
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
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.
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. :)
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
Blocks: 650569
Blocks: 650589
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.
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 :-)
Assignee: nobody → ben
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla9
Followup marking tests as expected passing: http://hg.mozilla.org/mozilla-central/rev/b9905024a884
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Any unacceptable rendering issues, that is.
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?
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?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: