New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 738517 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression

Blocking:
issue 713889



Sign in to add a comment

Disabled <input type=button> state is wrong

Project Member Reported by rsesek@chromium.org, Jun 30 2017

Issue description

Chrome Version: 61.0.3145.0
OS: mac 10.12.5

What steps will reproduce the problem?
(1) Go to |data:text/html,<input type=button disabled value=hello>|
(2) Look at the rendered button
(3) Compare the results with the screen shot

What is the expected result?
The button should look like the one on the right in the screenshot.

What happens instead?
Instead, it looks flat like the one on the left.

Please use labels and text to provide additional information.


For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.

 
Screen Shot 2017-06-30 at 2.06.17 PM.png
30.3 KB View Download

Comment 1 by rsesek@chromium.org, Jun 30 2017

Owner: ccameron@chromium.org
Status: Assigned (was: Untriaged)
You are probably looking for a change made after 482902 (known good), but no later than 482905 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/d95e4951a3bab57b4839e3a3d7e64d43c8079d42..19bd384f656790e8e409086b5b97a53b3a6f5921

Guessing https://chromium.googlesource.com/chromium/src/+/1894d423068be735560e0171922c833c4091b5d1.

Comment 2 by ajha@chromium.org, Jul 7 2017

Cc: hubbe@chromium.org
Labels: -Pri-2 Pri-1
Friendly ping to get an update on this issue marked as Blocker.

per-revision-bisect result also points to the same CL as pointed in C#1.

Last good build: 61.0.3143.0
First bad build: 61.0.3144.0

Changelog:
==========
https://chromium.googlesource.com/chromium/src/+log/8b4643f67dc1c6b142a152aa2b76c771287e3a22..1894d423068be735560e0171922c833c4091b5d1

Comment 3 by ajha@chromium.org, Jul 7 2017

Labels: hasbisect-per-revision
Note: Worked fine on the latest canary(61.0.3150.0) of Windows and Linux. This is Mac specific.

Comment 4 by gov...@chromium.org, Jul 11 2017

A friendly reminder that M61 branch is coming soon on 07/20! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix ASAP to trunk. This way we branch M61 from a high quality trunk. Thank you.

Comment 5 by ajha@chromium.org, Jul 14 2017

Friendly ping to get an update on this marked as Blocker.
Interesting, taking a look.
Cc: -hubbe@chromium.org fmalita@chromium.org hu...@chromium.orgm brianosman@chromium.org
The color conversion being done here is landing in SkImage_Raster::onMakeColorSpace.

I instrumented the function to print the "before" and "after" getColor values.

The input color is
  fBitmap.getColor(30,38) = 80ffffff
  *fBitmap.getAddr(30,38) = 80808080

The output color is, depending on the monitor's color space
  dst.getColor(30,38) = 80adadad
  *dst.getAddr(30,38) = 80575757

The SkTransferFunctionBehavior is kIgnore. If I change the behavior to kRespect, then I get

The output color is, depending on the monitor's color space
  dst.getColor(30,38) = 80d9d9d9
  *dst.getAddr(30,38) = 806d6d6d

This is when using Apple's "Generic RGB" profile. If I switch to something closer to sRGB, then I sometimes end up with kRespect giving me 80ffffff/80808080 as the output color.

To read the documentation of SkTransferFunctionBehavior::kIgnore, this seems like the expected behavior, but I think that we'll want the kRespect behavior.
Cc: -hu...@chromium.orgm hubbe@chromium.org
I find it odd that this is the only instance of this bug that has come up so far -- shouldn't this affect any content?
Still looking more into why white isn't mapping to white...
Might also be related to  issue 747640 
Blocking: 713889
URGENT - PTAL.
Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the M61 branch #3163 ASAP to have enough baking time in Beta before Stable promotion. Thank you!

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label.

Attaching output from P3 versus sRGB.

Talked offline w/Brian and Florin, and this is still somewhat mysterious.
srgb.png
310 KB View Download
Found the bug. It's in constructing the pixel conversion pipeline at:

https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkConvertPixels.cpp?rcl=5f9b59b52db2b9a11d41bb7fa93ec5e5d6104236&l=304

We do ...

    SkAlphaType premulState = srcInfo.alphaType();
    if (kPremul_SkAlphaType == premulState && SkTransferFunctionBehavior::kIgnore == behavior) {
        pipeline.append(SkRasterPipeline::unpremul);
        premulState = kUnpremul_SkAlphaType;
    }

    SkColorSpaceTransferFn srcFn;
    if (isColorAware && srcInfo.gammaCloseToSRGB()) {
[*]     pipeline.append_from_srgb(srcInfo.alphaType());
    } else if (isColorAware && !srcInfo.colorSpace()->gammaIsLinear()) {
        SkAssertResult(srcInfo.colorSpace()->isNumericalTransferFn(&srcFn));
        pipeline.append(SkRasterPipeline::parametric_r, &srcFn);
        pipeline.append(SkRasterPipeline::parametric_g, &srcFn);
        pipeline.append(SkRasterPipeline::parametric_b, &srcFn);
    }

Notice that at the [*] we're using srcInfo.alphaType to do the srgb-to-linear conversion. But, in the previous step, we converted the alpha type from kPremul_SkAlphaType to kUnpremul_SkAlphaType.

Putting up a patch to fix this.
Cc: kavvaru@chromium.org ccameron@chromium.org brajkumar@chromium.org ajha@chromium.org
 Issue 749960  has been merged into this issue.
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 31 2017

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/179849efe5f5bb20e7036bb6e5a9f4bb9ad8b2f1

commit 179849efe5f5bb20e7036bb6e5a9f4bb9ad8b2f1
Author: Christopher Cameron <ccameron@chromium.org>
Date: Mon Jul 31 20:39:37 2017

Fix premul state in from_srgb in convert_with_pipeline

Without this patch, the pipeline
1. converts to unpremultiplied
2. applies the sRGB transfer assuming the pixel is premultiplied

In step 2, we should assume the pixel is unpremultiplied.

Bug:738517
Change-Id: Ic11fcf64faa423577ccb1cfc0cfe96feb57db09a
Reviewed-on: https://skia-review.googlesource.com/28404
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>

[modify] https://crrev.com/179849efe5f5bb20e7036bb6e5a9f4bb9ad8b2f1/src/core/SkConvertPixels.cpp

Labels: Merge-Request-61
Requesting merge for M61
Project Member

Comment 19 by sheriffbot@chromium.org, Aug 1 2017

Labels: -Merge-Request-61 Hotlist-Merge-Approved Merge-Approved-61
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
Project Member

Comment 20 by bugdroid1@chromium.org, Aug 2 2017

Labels: merge-merged-m61
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/218b516e7b2248bae53132b97822702dac4c8abc

commit 218b516e7b2248bae53132b97822702dac4c8abc
Author: Florin Malita <fmalita@chromium.org>
Date: Wed Aug 02 17:26:18 2017

Fix premul state in from_srgb in convert_with_pipeline

Without this patch, the pipeline
1. converts to unpremultiplied
2. applies the sRGB transfer assuming the pixel is premultiplied

In step 2, we should assume the pixel is unpremultiplied.

Bug:738517
Change-Id: Ic11fcf64faa423577ccb1cfc0cfe96feb57db09a
Reviewed-on: https://skia-review.googlesource.com/28404
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>

No-Tree-Checks: true
No-Try: true
No-Presubmit: true
Cherry-Pick: 179849efe5f5bb20e7036bb6e5a9f4bb9ad8b2f1
Approval:  https://crbug.com/738517#c19 
Change-Id: I637716ca2b722b863491e2bd66f6c7ed7e1fac0e
Reviewed-on: https://skia-review.googlesource.com/30041
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Florin Malita <fmalita@chromium.org>

[modify] https://crrev.com/218b516e7b2248bae53132b97822702dac4c8abc/src/core/SkConvertPixels.cpp

Labels: -Merge-Approved-61 merge-merged-3163
Labels: -Hotlist-Merge-Approved
Status: Fixed (was: Assigned)
Thanks fmalita for the merge!

Sign in to add a comment