sounds like a dup of https://bugs.chromium.org/p/chromium/issues/detail?id=830545 ?

The symptoms certainly sound the same. Is this affecting all devices? I don't see it on soraka or lumpy devices with older system images.

Issue 850986 was filed from monitoring external display chameleon testing and looks similar - started with R69-10757.0.0 build.

@2: Yes it's everywhere, since it's the GL code that does the blur which generates banding. Of course different display panel qualities will outline this even further.

@3: Issue 850986 is unrelated to this.

Adding some people who appear to have worked on layer blurring and may know what's changed here.

Issue 830545 describes a similar-seeming issue on rockchip, but I'm not sure that it's the same thing. I've seen color banding on blurred wallpaper for a long time (since blurring was introduced, I think) on a veyron_speedy device, but the regression described here on caroline is more recent. marcheu@'s comment there: "Hw dithering is working fine on my jaq, so this is a UI thing. At first glance it seems like the blur effect on the background generates this banding."

In any case, this looks bad enough that I don't think we want it to go out to the stable channel. I'm not sure if it's in M68 or just started in M69.

Blur was introduced since M61 last year. Dis we see this at that time?

If it is a regression to make banding more obviously, we probably can bisect to find out which cl caused it.

But if it is the underlining banding due to blur, I am not sure what will be the best solution here.

+wzang@, do we have any blur related changes on the login/lock screen?

I think that it's much more recent than M61, at least on caroline. I first noticed it in late May or early June, I believe.

Re wutao@, I'm sure there's none.

I will start bisect.

I can repro it on EVE.


Currently finding is that the culprit cl could be in the range of 69.0.3449.0 and 69.0.3451.0 or OS between 10756.0.0 and 10757.0.0.

Will continue working on it.

+dnicoara@ and dcastagna@

In case it is related.
There are some color correction/matrix changes in the above range in #12.

@5: I just tried on caroline, testing on 66 vs 69, and there is definitely more banding in 69. But the colors of the wallpaper also seem brighter, so it might be that the banding was always there, and was made more visible by color correction, or a change of wallpaper assets, for example.

This is turn would mean that the issue might have always been there, and we just made it more visible.

+afakhry@, whose gamma correction cl is also in that ranges.

Hi Ahmed,

It is your Night Light cl (Night Light: Account for gamma correction) makes the banding more obviously. Assigning this bug to you for now.

https://chromium-review.googlesource.com/c/chromium/src/+/1068112

Thanks.

That banding has always been there. The default gamma/degamma LUTs we're applying is just making them more obvious. I'm not sure if there's anything we can do about this without fixing issue 830545. dcastagna@ WDYT?

Who owns the Chrome-side blurring code? Would that be someone on the Skia team?

The default wallpaper looks really, really bad at the login screen on my caroline device due to this. It's bad enough that I would assume the display was malfunctioning if I saw this device in a store and not buy it. 

What would be the negative effect(s) of reverting https://crrev.com/c/1068112?

Deleted: IMG_20180613_182549.jpg 2.8 MB

	IMG_20180613_182549.jpg 2.8 MB View Download

The implementation is at Skia side: https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkBlurImageFilter.cpp?l=651&rcl=7c525e62d405d57ae7a7742cf607b9770a83a0ab

Thanks! Adding some recent committers there for their takes.

Hi! I'm the original author of the Skia blur code. I haven't looked into the ChromeOS color correction code, but I've seen similar issues when LUTs are applied to blurs in other contexts. For example, in SVG's feGaussianBlur when color-interpolation-filters is set to linearRGB (the default), it applies an 8-bit sRGB->linear LUT, blurs in linear colorspace, then linear->sRGB. While this is mathematically correct, an 8-bit-per-component LUT inevitably introduces banding near the blacks, since the final LUT "spaces out" the values near the blacks to visible increments. (Similar things happen to gradients when an 8-bit LUT is applied.) Some authors set color-interpolation-filters to sRGB, which avoids the LUTs and blurs the sRGB data as if it was linear. While not mathematically correct, it eliminates the banding. (Note that the SVG default for all other operations in SVG including blending and gradients, color-interpolation, is actually this mathematically-incorrect one).

It would be possible to do the mathematically-correct thing while avoiding banding by doing all the math in FP16. However, it would be a rather significant hit in performance, VRAM and (likely) power. It would also require additions to the Skia API and significant work on the CPU raster side.

If possible, I'd suggest to avoid using an 8-bit LUT when blurring is involved.

Following on... The Skia code is almost certainly doing the mathematically incorrect (but optimal to reduce banding) thing. We only blur in linearRGB if the surface is tagged with a color space, and no one (AFAIK) does that today.

I'd like to see the original (unblurred) image, as well as an actual screenshot of the blurred image. Walking through the process:

1) Hopefully (and likely), the blurred image turns into a really shallow gradient, with bands being consecutive sRGB-encoded values. This is optimal (up to the potential addition of dithering). If the blur is producing bands with large discrete jumps, that sounds like a failure in the blurring code itself.

2) It looks like now we're using LUTs as part of a final color correction step at the display level? It also looks like those are 16 bit? That should give good precision, but any discretized continuous function like this, particularly when applying to something like 8-bit data has the potential to severely comb the value histogram, which is going to manifest as this kind of banding. Am I reading the code right that we're using a (16 bit) LUT to degamma, applying some HW matrix, then another LUT to gamma encode? If this step is what's quantizing the blurred results, there is very little we can do - dithering the blurred results will help, but still won't introduce new values in the final output, just rearrange the existing ones.

Getting those images from multiple points in the pipeline will help narrow down how best to deal with this, in any case.

@#22: As #24 mentioned, the degamma/gamma operations are done in the display controller and we expect to be at higher precision than 8-bit. For each LUT we have 512 entries and 16 bits per component.

We still believe that setting degamma/gamma LUTs might be the cause of the issue and we're investigating further.

A way to prevent this issue, while we keep investigating degamma/gamma issue, could be to avoid setting the LUTs and use the same night light CTM we use when we do night light with GPU compositing.

Yeah, assuming I'm right about the source of the banding, I don't think the higher-precision LUTs are going to help as long as the output of the blur (and input to the LUT) is 8 bit. To really avoid banding, the blur has to be done at a higher bitdepth as well. (I should've made that clearer in #22, rather than pointing the blame at the LUT depth.)

Re#26: 

> To really avoid banding, the blur has to be done at a higher bitdepth as well.

If that's the correct solution, is it possible to implement, or is it what you referred to in #22 as "would be a rather significant hit in performance .."?

Thanks for all the details!

#24: Here are some screenshots: blurred.png (from the login screen) and unblurred.png (after logging in).

I'm also attaching what I believe is the source wallpaper image that's used here.

Deleted: blurred.png 659 KB

	blurred.png 659 KB View Download

Deleted: unblurred.png 3.4 MB

	unblurred.png 3.4 MB View Download

Deleted: wallpaper.jpg 1.2 MB

	wallpaper.jpg 1.2 MB View Download

Re: #27: 

> If that's the correct solution, is it possible to implement, or is it what you > referred to in #22 as "would be a rather significant hit in performance .."?

Both. It's possible to implement, but would be a perf and VRAM hit.

We looked into it yesterday, and I think there are two issues:
- the existing blur code produces banding
- the gamma/degamma support also seems to produce banding of its own

Of course when you add the two together, it looks pretty bad. We are still looking at gamma/degamma, but once we understand what's going on, it should probably get its own bug.

So I was looking at what the intel hw does for gamma/degamma. The hw documentation https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf page 153 has the answer: the split gamma mode doesn't do interpolation of the values (although with the 12 bit gamma ramp there is interpolation, but that mode doesn't offer split gamma). In other words, it seems like the split gamma mode is fairly useless for high gamma values (like 2.2).

I am seeing the same thing on Eve as well.

Fixes for this will hopefully land this week.

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

commit f148ff5dff4a33e05874618930d9f2e23dd928e4
Author: Daniele Castagna <dcastagna@chromium.org>
Date: Fri Jun 22 18:25:22 2018

ozone/drm: Expose if color correction is applied in linear space

For Night Light we tried to always apply the color transform matrix
in linear color space when using the display controller for the
transformation.

On Rockchip we'd get the BT.709 degamma/gamma automatically.
On Intel we used the split gamma pipeline and we set degamma/gamma
manually.

Unfortunately intel split gamma doesn't do interpolation of values and
introduced banding.

This patch exposes to the browser if a display color correction matrix
will be applied in linear color space or in a gamma compressed
one.

In this way we'll be able to change the night light matrix and avoid
setting gamma/degamma on intel.

Bug:  851156 
Test: display_unittests
Change-Id: Ia0d385697747afb42c5dbb1b939be661c4fdd559
Reviewed-on: https://chromium-review.googlesource.com/1108362
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569701}
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ash/display/display_error_observer_unittest.cc
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/manager/display_change_observer_unittest.cc
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/manager/fake_display_snapshot.cc
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/manager/fake_display_snapshot.h
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/mojo/display_snapshot.mojom
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/mojo/display_snapshot_struct_traits.cc
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/mojo/display_snapshot_struct_traits.h
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/mojo/display_struct_traits_unittest.cc
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/types/display_snapshot.cc
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/display/types/display_snapshot.h
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/ozone/common/gpu/ozone_gpu_message_params.h
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/ozone/common/gpu/ozone_gpu_messages.h
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/ozone/platform/drm/common/drm_util.cc
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/ozone/platform/headless/headless_native_display_delegate.cc
[modify] https://crrev.com/f148ff5dff4a33e05874618930d9f2e23dd928e4/ui/ozone/platform/wayland/wayland_output.cc

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

commit d4e731e37dc90628c499a21e3ad516eb4608827a
Author: Ahmed Fakhry <afakhry@google.com>
Date: Tue Jun 26 01:09:46 2018

Night Light: Don't apply gamma tables

We found out that the hardware split gamma mode
doesn't do interpolation of the values, which led
to some bad side effects such as ugly banding in
blur effects.

This CL removes all the default gamma tables, and
uses the newly exposed data to identify if the
hardware does color correction in linear gamma
space or not. This lets us select which type of
matrix to apply.

BUG= 851156 ,749250
TEST=Expanded tests

Change-Id: I0342b6e4a0257e833c7d13690930dfd32ce89fe7
Reviewed-on: https://chromium-review.googlesource.com/1112734
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570279}
[modify] https://crrev.com/d4e731e37dc90628c499a21e3ad516eb4608827a/ash/display/display_color_manager.cc
[modify] https://crrev.com/d4e731e37dc90628c499a21e3ad516eb4608827a/ash/display/display_color_manager.h
[modify] https://crrev.com/d4e731e37dc90628c499a21e3ad516eb4608827a/ash/display/display_color_manager_unittest.cc
[modify] https://crrev.com/d4e731e37dc90628c499a21e3ad516eb4608827a/ash/system/night_light/night_light_controller.cc
[modify] https://crrev.com/d4e731e37dc90628c499a21e3ad516eb4608827a/ash/system/night_light/night_light_controller.h
[modify] https://crrev.com/d4e731e37dc90628c499a21e3ad516eb4608827a/ash/system/night_light/night_light_controller_unittest.cc
[modify] https://crrev.com/d4e731e37dc90628c499a21e3ad516eb4608827a/ui/display/manager/fake_display_snapshot.cc
[modify] https://crrev.com/d4e731e37dc90628c499a21e3ad516eb4608827a/ui/display/manager/fake_display_snapshot.h

This should be fixed now. Please try on the latest canary. Device reboot is required.