Uploading the picture taken with my phone.

Deleted: photo.JPG 73.8 KB

	photo.JPG 73.8 KB View Download

I do not see this on kevin.
I wonder is this hardware related?

I can see the blocks when running under remote desktop.

+reveman@ for more advice.

With the same linux build, I cannot see blocks locally. But I can see black boxes when running under remote desktop.

Interesting, when I add flag --ui-show-paint-rects, those boxes are gone.

+danakj@

These boxes are very likely the rects containing the cursor, shelf and some others.
Why adding the debug layer can remove the boxes?

If you --ui-disable-partial-swap does it stop also? Perhaps its partial-swap/damage related. --ui-show-paint-rects would change what is damaged because we fade a rect around any painted areas out over a few frames.

RE#8: --ui-disable-partial-swap stops it also.

partial-swap might be the reason. I suspect it is the similar reason of this bleeding black bug: 622128.

danakj@, but from the code, it looks like that if there is a blur filter, we do not use partial swap?
https://cs.chromium.org/chromium/src/cc/surfaces/surface_aggregator.cc?l=419

Aggregation is not the same as drawing. In this case it looks like we aggregate everything. But partial draw/swap happens in DirectRenderer.

From here: https://cs.chromium.org/chromium/src/cc/output/direct_renderer.cc?rcl=5dfca923a6135d4d464d8aa2b9078151954fead1&l=258

Is this partial_swap texture a totally new buffer with the smaller size to contain the partial_swap rect? Or it is just a size for the gl_->scissor where to clip and redraw the partial content?

We scissor when we draw to the backbuffer, and then swap only that area, which copies it to the front buffer. When we copy from that buffer, outside the scissor would be black I assume, and maybe we're only copying a smaller part out of the back buffer for blurring than what's been drawn.

I think +jbauman fixed something related to partial swap on Windows; maybe this is a similar bug?

Possibly a dupe of https://bugs.chromium.org/p/chromium/issues/detail?id=645484 ?

Looks like even for partial damage, the background_texture texture size in ApplyBackgroundFilters is still full screen size. Do we clear the framebuffer for every frames? So for the partial damage case, only the partial damaged area is not black?

Maybe we need to expand the partial damaged area by 3*sigma and send it to skia, then use the unexpanded rect to swap the partial damage area.

Right, outside teh area we're drawing it'd be black. But I'd expect damage tracker to expand damage to cover the pixels we need so we'd not see black in the blur. Maybe you can investigate why that isnt working out.

If damage_tracker already expands the damage area, then in partial swap, we might need to clip the swap rect before sending to GLES2Implementation::PartialSwapBuffers(const gfx::Rect& sub_buffer).

Maybe change code at here in gl_renderer:
...
} else if (use_partial_swap_) {
****** swap_buffer_rect_.inset(3*sigma); ******
swap_buffer_rect_.Intersect(gfx::Rect(surface_size));
...
}

*above code may not work for the area near the viewport edges. inset(3*sigma) will clip the real damaged area.

Oh I see, that's a good point. It's like GLRenderer needs 2 rects here. A true 
"damage rect" that is what it should swap, and a "damage inputs rect" which can be larger for filters that require content outside the damage rect, or so.

Computing the 2nd rect in GLRenderer sounds problematic, as it doesn't really know what the DamageTracker intended. I feel like maybe we need a 2nd rect on RenderPass and DamageTracker needs to produce both. WDYT?

I could've sworn there was something similar done for foreground filters (expanding the damage rect to compensate for filter margins), but I could be wrong.

Ya we do the same for foreground filters, but in that case we want to swap all the affected pixels, cuz we're bluring out into them. For a background filter, we're reading from the expanded area but not writing it.

This is going to be used for webui lock screen, which means we need it in 61.

Try to prototype the idea in #22,

Two problems left to investigate:
1. The 2nd unexpanded_damage_rect can be tracked and passed to gl_renderer, but the damage_rect for cursor is not included.

2. Even with a smaller unexpanded_damage_rect and set it to swap_buffer_rect_ in gl_renderer, but still see larger black rect.

Is there a way or setting to use partial_swap at linux build?

> Is there a way or setting to use partial_swap at linux build?

I think --use-gl=osmesa uses partial swap, but is also super slow so beware.

Software compositing via --disable-gpu also does partial swap.

I got some images before sending to skia (input) and blurred images processed by skia (blurred). The suffix is the frame number.

Deleted: test_input_1.png 1.6 MB

	test_input_1.png 1.6 MB View Download

Deleted: test_blurred_1.png 198 KB

	test_blurred_1.png 198 KB View Download

Deleted: test_input_2.png 802 KB

	test_input_2.png 802 KB View Download

Deleted: test_blurred_2.png 172 KB

	test_blurred_2.png 172 KB View Download

Deleted: test_input_3.png 1.0 MB

	test_input_3.png 1.0 MB View Download

Deleted: test_blurred_3.png 189 KB

	test_blurred_3.png 189 KB View Download

Deleted: test_input_4.png 815 KB

	test_input_4.png 815 KB View Download

Deleted: test_blurred_4.png 173 KB

	test_blurred_4.png 173 KB View Download

Deleted: test_input_5.png 651 KB

	test_input_5.png 651 KB View Download

Deleted: test_blurred_5.png 164 KB

	test_blurred_5.png 164 KB View Download

Deleted: test_input_6.png 326 KB

	test_input_6.png 326 KB View Download

Deleted: test_blurred_6.png 135 KB

	test_blurred_6.png 135 KB View Download

danakj@ and reveman@,
If I understand correctly, the idea in #22 may not work.

From the images at #35, for partial swap it is important to keep the back buffer as the same as the front buffer. We modify the back buffer and swap partial to front. If we only swap a small area from back buffer to front buffer, they are not in sync any more.

I am thinking of a solution that whether we can mark all the blurring layer to be damaged, we might solve this problem.

The current problem is that part of the blurring layer is damaged, so we need to recalculate the blur of that small area. It is no a good solution to make it seamless with other blurred part.
However if the whole blurring layer is damaged, this is not a problem. This has some performance cost. But I cannot think of better solution.

Any pixels that we would write should be swapped, and what we write should be the same with/without partial swap. Would we get out of sync if we maintain that invariant? I feel like when we're not doing that it's a bug, as in this case.

> The current problem is that part of the blurring layer is damaged, so we need to recalculate the blur of that small area

Why can't we compute what area we need to get the correct result?

Yes, the area we write to must always match the area we swap. If we draw to an area of the current buffer that is larger than what we swap then we'll see artifacts on ChromeOS as we track the damage area of each buffer based on what is swapped.

reveman@, WDYT the following:

I have a quick implementation for #37.

The idea is that add a new flag which can be accessed at render_surface in damage_tracker.cc. If this flag is set (during app launcher is visible), then mark the render_surface of the app launcher is totally damaged.

Tested on intel device (link), it works just fine.

Could we put all of this implementation behind a flag only affect in ChromeOS for now before we have better solution?

I'd much rather we fix the bug. There is nothing chromeos specific going on here.

danakj@
We have a feature we real want in M61 depend on resolving this.

But I can think of long term solution, could be adding another buffer to handle this partial swap before sending to skia. I can think more on this path.

Not to sound too blunt, but if a feature isn't ready for M61 then it shouldn't ship in M61. That's why we have a 6 week release cycle.

I have something do not understand now, why partial-swap does not cause a problem for foreground blur filter?

On foreground blur, the rect we swap to the screen == the rect we read for the filter. For background blur, we blur into a layer but the results are clipped to that layer's bounds, so they differ.

#41, we should find the real issue and not enable blur effects until this is working as expected.

I think reveman@'s idea to draw background into a 3rd buffer could be the solution for both this bug (partial swap) and performance improvement.

For blur, we need a clean buffer to send skia, noot like the image in #35, where we send skia a texture with viewport size and mixed with re-drawn area and un-damaged area in previous frame.

Not sure I totally understand the problem, but if the problem is that the input texture contains pixels you don't want to sample, there are a couple of solutions. If you're using foreground filters, you could use a CropRect on the first filter in the chain. If you're using backdrop filters, moving to the SkImage::makeWithFilter() fast path allows the use of a "subset" rect, which has the same constraints: the texture will not be sampled outside the rect (WIP patch: https://chromium-review.googlesource.com/c/581294/)

However, it will apply the edge treatment to the given pixels. In order for the blur to blend smoothly with the existing content, you'll definitely need to pass an expanded source region (with valid, unblurred image pixels in the filter margin).

#49, that is the problem "you'll definitely need to pass an expanded source region".

My understanding it that in partial swap, to keep back buffer in sync of front buff, the re-drawn area of the back buffer (expanded region) must be all swapped to the front buffer.
Currently we expanded the real damage by 3*sigma already, and re-drawn it on the back buffer (for partial swap, we did not clear the back buffer to keep the back/front buffer in sync). So there are two problems, 1) we send full size (viewport) of the back buffer to skia, so the boarder of 3*sigma, we are blurring different contents (re-drawn content and un-re-drawn previous frame). 2. we need to swap all the expanded region (3*sigma) to front buffer.

That is why we got black boxes when partial swap is happening.

My quick solution is invalidate (mark all the burring-layer as damaged) when there is a blur for partial-swap.
But for long term solution, we might need a third texture to temporary drawn to with expanded region, and only copy the real-damaged region to back/front buffer.

But because some of the "expanded region" is only for blur, not for display, 

Thanks for the clarification. I think I understand the problem now.

One request I'd like to make: please don't focus exclusively on blur. Backdrop filters can theoretically include many other pixel-moving types of filters, e.g., offset, morphology, convolve-matrix, displacement, etc. If possible, don't hardcode the 3*sigma value, but rather use FilterOperation::MapRectReverse() (or SkImageFilter::filterBounds(..., SkImageFilter::kReverse_MapDirection)). These should handle the required bounds computation for an arbitrary filter DAG.

I think I understand the problem now: when you expand the damage rect, you fill the backbuffer in the filter margins outside the damage rect with unblurred pixels. You need those in order to produce correct edges in the interior border of the damage rect. But you also need the (correct) blurred versions of those pixels you had in the previous frame, which you've just clobbered. The backbuffer can't contain both blurred and unblurred pixels in a given region, so you can't satisfy both requests.

Is that correct? If so, I do think the fix will require some extra storage *somewhere* to accommodate both. We'd either have to redirect all background image drawing to that extra texture (as wutao@ suggests), or copy the valid border region there temporarily before drawing, and blit it back after blurring. (The latter seems more complex, though.)

I think that effectively disabling partial swap for layers containing pixel-moving filters would fix the correctness until we can come up with the correct solution allocating extra storage for performance. Would that be acceptable, cc folks?

#52, that is correct. Thanks senorblanco@.

Uploaded a cl for review to fix the correctness of partial swap when there is pixle moving background filter:
https://chromium-review.googlesource.com/c/584307

There is a claim that the backbuffer needs to be matching the front buffer, is that true? I would expect that it does not, because any time we swap we redraw whatever pixels we swap.

We shouldn't need extra storage if we draw a larger area than we swap (and ignore the pixels that are incorrect as a result)?

 Issue 626762  has been merged into this issue.

#55, yes, we redraw whatever pixels we swap, so in this bug we are swapping wrong content outside the real damage rect at the filter margin (3*sigma for blur).

The blur or other pixel-moving bg filter results are not correct as explained in #52.

Whether we need to make sure all pixels are correct depends on the platform. On windows with DirectComposition and Chrome OS we use flipping, so even pixels outside the partial swap area are actually displayed on-screen.

 Issue 645484  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3af153f48e439140eeeed51e800a4526364a7688

commit 3af153f48e439140eeeed51e800a4526364a7688
Author: wutao <wutao@chromium.org>
Date: Tue Jul 25 21:49:52 2017

Do not allow partial swap for pixel-moving background filter.

Partial swap does not handle the case with pixel-moving backgroud
filter. When expandng the damage rect, we fill the backbuffer in the
filter margins outside the damage rect with unblurred pixels. We need
those in order to produce correct edges in the interior border of the
damage rect. But we also need the blurred and blended versions of those
pixels we had in the previous frame, which we've just clobbered. The
backbuffer can't contain both blurred and unblurred pixels in a given
region, so we can't satisfy both requests.

Effectively disabling partial swap for layers containing pixel-moving
background filters by marking the whole output_rect as damage would fix
the correctness.

Bug:  737255 
Test: SurfaceAggregatorPartialSwapTest.IgnoreOutside && intel devices.
Change-Id: Ib1431c4e1912636c5ac4a7babf296b58b50177c0
Reviewed-on: https://chromium-review.googlesource.com/584307
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: John Bauman <jbauman@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489451}
[modify] https://crrev.com/3af153f48e439140eeeed51e800a4526364a7688/components/viz/service/display/surface_aggregator.cc
[modify] https://crrev.com/3af153f48e439140eeeed51e800a4526364a7688/components/viz/service/display/surface_aggregator_unittest.cc

[Auto-generated comment by a script] We noticed that this issue is targeted for M-61; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-61 label, otherwise remove Merge-TBD label. Thanks.

Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/040f5baa9657c9f69fc8604f874b44310aae20f3

commit 040f5baa9657c9f69fc8604f874b44310aae20f3
Author: Qiang Xu <warx@chromium.org>
Date: Fri Jul 28 23:20:34 2017

[merge to m61] Do not allow partial swap for pixel-moving background filter.

merge to m61 on behalf of wutao@

Partial swap does not handle the case with pixel-moving backgroud
filter. When expandng the damage rect, we fill the backbuffer in the
filter margins outside the damage rect with unblurred pixels. We need
those in order to produce correct edges in the interior border of the
damage rect. But we also need the blurred and blended versions of those
pixels we had in the previous frame, which we've just clobbered. The
backbuffer can't contain both blurred and unblurred pixels in a given
region, so we can't satisfy both requests.

Effectively disabling partial swap for layers containing pixel-moving
background filters by marking the whole output_rect as damage would fix
the correctness.

(cherry picked from commit 3af153f48e439140eeeed51e800a4526364a7688)

TBR: jbauman@chromium.org, reveman@chromium.org
Bug:  737255 
Test: SurfaceAggregatorPartialSwapTest.IgnoreOutside && intel devices.
Change-Id: Ib1431c4e1912636c5ac4a7babf296b58b50177c0
Reviewed-on: https://chromium-review.googlesource.com/584307
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: John Bauman <jbauman@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489451}
Reviewed-on: https://chromium-review.googlesource.com/592511
Reviewed-by: Qiang(Joe) Xu <warx@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#123}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/040f5baa9657c9f69fc8604f874b44310aae20f3/components/viz/service/display/surface_aggregator.cc
[modify] https://crrev.com/040f5baa9657c9f69fc8604f874b44310aae20f3/components/viz/service/display/surface_aggregator_unittest.cc