Found another review on 60.0.3087.3

user comments:
After I downloaded the app the pictures that I view online are of a really poor quality!!!!

system information

APPLICATION
Version code 308700300 Version name 60.0.3087.3
DEVICE
Device Y221-U12 (HWY221-U) Manufacturer Huawei Device type Phone Device language English CPU make Mediatek CPU model MT6572M Native platform armeabi-v7a, armeabi RAM (MB) 512 Screen size 320 × 480 Screen density (dpi) 160 OpenGL ES version 2.0 OS Android 4.4

prashanthpola@, can you check on our Huawei phones?  Thanks.

Further response from the user as below. Please kindly let me know if it'd be helpful to collect user's system log. Thanks!

"It happens on google images and everthing else that has a static image. Videos are fine once they are playing. I switched back to normal chrome when beta first got this issue, but normal chrome developed the same issue in one of its latest updates thats why i swithced back to chrome beta. Also, ive tried other none google browsers and their images are perfectly fine"

After the latest update on 18/5/2017, it still has the same issue.

Application Version (from "Chrome Settings > About Chrome"):58.0.3029.83
Android Build Number (from "Android Settings > About Phone/Tablet"): 4.4.2
Device: Huawei Ascend Y520

Steps to reproduce: 
1.Launch chrome
2.Search or Enter any URL for e.g(Ivanka Trump)
3.Click on images on top
4.Click on any image

Observed behavior: 
The image resolution is poor

Expected behavior: 
The image resolution should be good

Frequency: 100%

Additional comments: 
**Bisecting is in progress and will provide the Bisect info soon.

@ Images,logs: http://go/chrome-androidlogs1/6/720105

It's also reproducible in M59 and M60 too

Bisect Info:

Good build: 58.0.3012.4
Bad build: 58.0.3013.0
Regression range: https://chromium.googlesource.com/chromium/src/+log/58.0.3012.0..58.0.3013.0?pretty=fuller&n=10000

Good commit: 450469
Bad commit: 450470
Culprit CL: https://chromium.googlesource.com/chromium/src/+/5e009710e6cb606d60ba8a48963125a7775a25dc 

Only two CLs in that roll, so assigning to herb@ at random and CC'ing msarett@.

Tentatively marking as RB-Stable for M59 since it's a regression that is degrading images, which is a major use case; PTAL and let me know if you disagree, though.

I think there is a good chance that my CL is at fault.  I'll try to reproduce and figure out why we're losing resolution.

My guess is that I caused a behavior change around kARGB_4444_SkColorType.  I'd suggest that the browser not use 4444 if we want good resolution - but I'll need to dig into where we are calling into Skia.

I suspect the change in behavior is caused by the fact that conversion to 4444 no longer uses dithering.  Does this seem consistent with what you are seeing?

I'd like to fix this by using 8888 instead of 4444 at the client level.

> I suspect the change in behavior is caused by the fact that conversion to 4444 no longer uses dithering.  Does this seem consistent with what you are seeing?

Who are you asking here?  I have no idea how to answer that question as I don't know what dithering is, maybe TE does but seems like a stretch as well.  There are images in c#8, can you check those out?

I was asking the reporter as I've not been able to find a device to reproduce yet.

Thanks for the pointer to the images in #8!  This does appear to be caused by the fact that we no longer dither.

It wouldn't be hard to dither 4444 if you want.  It ought to be you just tack on a dither stage with a rate of 1/15.0f right before store_4444.

Arguably we should add dither any time we're down-bitting?

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/766f9cd5553ab091b85d287a79f47cbb91ad7989

commit 766f9cd5553ab091b85d287a79f47cbb91ad7989
Author: Mike Klein <mtklein@chromium.org>
Date: Sat May 20 16:37:12 2017

dither copies when decreasing precision

Still seeing the same 4444 diffs on copyTo4444 and all_bitmap_configs,
and now also 565 in all_bitmap_configs.

BUG= chromium:720105 

Change-Id: I19406f57aa6d2b2f98d98c093da302b004c7cd8b
Reviewed-on: https://skia-review.googlesource.com/17419
Commit-Queue: Mike Klein <mtklein@chromium.org>
Reviewed-by: Matt Sarett <msarett@google.com>

[modify] https://crrev.com/766f9cd5553ab091b85d287a79f47cbb91ad7989/src/core/SkConvertPixels.cpp

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/580d81fb336701dc815d79b0df028a31446c4b06

commit 580d81fb336701dc815d79b0df028a31446c4b06
Author: Mike Klein <mtklein@google.com>
Date: Sat May 20 17:27:26 2017

Revert "dither copies when decreasing precision"

This reverts commit 766f9cd5553ab091b85d287a79f47cbb91ad7989.

Reason for revert: unit test failures, I think on bots running portable code path

Original change's description:
> dither copies when decreasing precision
> 
> Still seeing the same 4444 diffs on copyTo4444 and all_bitmap_configs,
> and now also 565 in all_bitmap_configs.
> 
> BUG= chromium:720105 
> 
> Change-Id: I19406f57aa6d2b2f98d98c093da302b004c7cd8b
> Reviewed-on: https://skia-review.googlesource.com/17419
> Commit-Queue: Mike Klein <mtklein@chromium.org>
> Reviewed-by: Matt Sarett <msarett@google.com>
> 

TBR=mtklein@chromium.org,msarett@google.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:720105 

Change-Id: Ia9ece7dccef325233b870102ab38fbed2336b95d
Reviewed-on: https://skia-review.googlesource.com/17442
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>

[modify] https://crrev.com/580d81fb336701dc815d79b0df028a31446c4b06/src/core/SkConvertPixels.cpp

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

commit e294988760e65f29ca5e8ac8ae56f71fd8e514de
Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org>
Date: Sat May 20 18:05:29 2017

Roll src/third_party/skia/ 2f6878778..766f9cd55 (1 commit)

https://skia.googlesource.com/skia.git/+log/2f687877835b..766f9cd5553a

$ git log 2f6878778..766f9cd55 --date=short --no-merges --format='%ad %ae %s'
2017-05-19 mtklein dither copies when decreasing precision

Created with:
  roll-dep src/third_party/skia
BUG= 720105 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=egdaniel@chromium.org

Change-Id: I3cf00fe5b9cb0e8923d88166bbe89600cf360931
Reviewed-on: https://chromium-review.googlesource.com/509942
Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org>
Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#473443}
[modify] https://crrev.com/e294988760e65f29ca5e8ac8ae56f71fd8e514de/DEPS

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

commit 21e6d26f4a46684deafc43ff9294974bc1b4ed05
Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org>
Date: Sat May 20 19:23:53 2017

Roll src/third_party/skia/ 766f9cd55..5d7f2b530 (3 commits)

https://skia.googlesource.com/skia.git/+log/766f9cd5553a..5d7f2b5301f8

$ git log 766f9cd55..5d7f2b530 --date=short --no-merges --format='%ad %ae %s'
2017-05-20 mtklein tidy up dither stage
2017-05-20 mtklein streamline SkRasterPipeline::run()
2017-05-20 mtklein Revert "dither copies when decreasing precision"

Created with:
  roll-dep src/third_party/skia
BUG= 720105 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=egdaniel@chromium.org

Change-Id: Ib86a1f24e85a0316e559fa65b77b12ea86614a2f
Reviewed-on: https://chromium-review.googlesource.com/509966
Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org>
Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#473445}
[modify] https://crrev.com/21e6d26f4a46684deafc43ff9294974bc1b4ed05/DEPS

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/6ebba9f5b26d556b0a22131749da680716059702

commit 6ebba9f5b26d556b0a22131749da680716059702
Author: Mike Klein <mtklein@chromium.org>
Date: Tue May 23 19:45:46 2017

Dither copies when decreasing precision below 32-bit.

Same GM diffs as the last attempt in 8888.

CQ_INCLUDE_TRYBOTS=skia.primary:Test-Android-Clang-PixelC-CPU-TegraX1-arm64-Release-Android,Test-Win2k8-MSVC-GCE-CPU-AVX2-x86-Release,Test-Android-Clang-Nexus10-CPU-Exynos5250-arm-Release-Android

BUG= chromium:720105 

Still looking at why readpixels and readpixelspicture are so weird
(lots of black) when drawing into sRGB 8888.  Will follow up.

Change-Id: I223e3b74e567aea1acaffa8db6b24fbf22d98c97
Reviewed-on: https://skia-review.googlesource.com/17443
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Matt Sarett <msarett@google.com>

[modify] https://crrev.com/6ebba9f5b26d556b0a22131749da680716059702/src/core/SkConvertPixels.cpp

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

commit 4df202e0b38733d30c04f43408dae4a8c42608fc
Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org>
Date: Tue May 23 21:35:29 2017

Roll src/third_party/skia/ 5373609d9..ee92f131b (11 commits)

https://skia.googlesource.com/skia.git/+log/5373609d90d8..ee92f131b8bb

$ git log 5373609d9..ee92f131b --date=short --no-merges --format='%ad %ae %s'
2017-05-23 scroggo Fix alpha in webp bug introduced with animation
2017-05-23 egdaniel Really disable srgb on nexus player vulkan
2017-05-19 mtklein Dither copies when decreasing precision below 32-bit.
2017-05-23 bsalomon Revert "Add a flag to GrSurfaceFlags that requires the texture to be cleared upon creation. "
2017-05-23 reed add default arg so android can bulid
2017-05-23 mtklein Revert "add knob to turn off fancy SkJumper features"
2017-05-23 egdaniel Disable srgb on vulkan nexus player
2017-05-23 jvanverth Add Material Design shadow reference sample
2017-05-23 bsalomon Add a flag to GrSurfaceFlags that requires the texture to be cleared upon creation.
2017-05-22 ccameron Don't pass uniform arrays in GrGLNonlinearColorSpaceXformEffect
2017-05-23 mtklein move sk_memset?? to SkOpts

Created with:
  roll-dep src/third_party/skia
BUG= 720105 , 723133 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=brianosman@chromium.org

Change-Id: I1607acbbbd2f34a68ae36c38cbb046465c1924db
Reviewed-on: https://chromium-review.googlesource.com/513045
Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org>
Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474065}
[modify] https://crrev.com/4df202e0b38733d30c04f43408dae4a8c42608fc/DEPS

After the recent chrome beta update on 25/5/2017, it still has the same issue. My device is a Huawei Y530. I just google image search beds after each update to check if the issue has been fixed, but it hasnt.
Also, previous versions of chrome beta havent had the image issues on my device. Its only since i raised the issue.

Hi, i hope that this helps.

Ive just installed Chrome Canary (Unstable) version 60.0.3109.0, and that HAS'NT GOT image resolution issues at all.

I installed Chrome Dev version 60.0.3107.3, and that HAS the same image issues as Chrome Beta.

I also re-enabled Chrome on my device, version 34.0.1847.114 (built into device i think), and tht HAS'NT GOT image resolutiin issues. However when i updated it to the current version 58.0.3029.83, it HAS the image resolution issues.

I hope that this helps. 

Thank you for trying to help.

Thanks that's very helpful.  The fact that 60.0.3109 is fixed is likely due to the change in #21.

Just a little update. Todays Canary 60.0.3110.0 update, still works on my phone with no image issues.

phoneemailonly123@, that's expected.  Canary should be fine into the future, dev will be fixed next week, and beta / stable shortly after that.

msarett@, mtklein@ - is the fix in c#20 in a place to merge?  Will it be safe to merge with only one beta left for 59?

No, we cannot use this as a fix for M59.  There's code that it relies on in M60 that's not present in M59, and adding that support code would be a real dicey patch.

Do we have any safe mitigations for M59, like rolling back what applied the dithering in the first place in M58?  I've seen quite a few Play Store reviews due to this issue :\  I don't want to do anything risky at all, but if we can address this safely we should.

Matt and I think he can safely do a partial revert of that CL that took the feature away in the first place.

I think that there is a different, simple fix that I can write for M59.  The hard part will be reproducing the bug and verifying the fix.  When does this need to be ready for?

Why will reproduction be challenging?  Our test team can reliably reproduce this, we can test a fix if you send us a test APK pretty much right away.  Or maybe you can go and just buy one of the affected devices and have it shipped to you if we're really in a pinch.

Re: when this needs to be ready, at a minimum the fix would have to land on M59 branch by next Tuesday at 5 PM PT.  That said having it land Friday would be better to give us time to catch regressions if possible with internal testing.

That said I don't want to do anything that's not safe so if it looks risky let me know right away.  One thing I'm curious about here is the impacted population, can we estimate how many users are hitting this bug?  I saw 5-6 reports in the Play Store beta feedback alone but if we can narrow to a certain GPU or something else, we can perform a better analysis to make the right risk / reward tradeoff.

I'll write the CL today.  I think it is safe.

It's not clear to me what devices are affected - I'm guessing there is some heuristic around when to use 4444 to save memory.

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

commit ef6f9c65527412ec4057ea0551f2e051beb94d32
Author: Matt Sarett <msarett@google.com>
Date: Thu May 25 23:22:01 2017

[m59] dither when converting to 4444
NOTREECHECKS=true
NOTRY=true
NOPRESUBMIT=true
Bug:  720105 
Change-Id: I2a3b8a56795403077151ccebed978a94f2350def

Change-Id: I2a3b8a56795403077151ccebed978a94f2350def
Reviewed-on: https://skia-review.googlesource.com/18021
Reviewed-by: Mike Klein <mtklein@chromium.org>
Commit-Queue: Mike Klein <mtklein@chromium.org>

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

Thanks for the quick fix!  In the future please do wait for an explicit LGTM and merge approval from a TPM, though like I said I appreciate the expediency.

Just to confirm, this is super safe and won't cause any side effects?

My bad.  Yes, this is very very safe.  It's the same old code we used to run, and only triggers in this particular 8888 -> 4444 conversion case.

OK, let's mark this as fixed.  Thanks for getting this addressed!

We still see this issue in Chrome:59.0.3071.82 Device: Huawei Ascend Y520/4.4.2

msarett@, mtklein@, PTAL ASAP - we only have a couple days to take stable changes for M59.  Let us know what you need us to test to help expedite things.

@ #39

That is surprising.  You are seeing the exact same bad image from #18?

Yes we are seeing the same bad image as c#18 

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

commit d16084ffdc688bad6d0e9c04f3f98049b3412633
Author: Matt Sarett <msarett@google.com>
Date: Thu Jun 01 17:01:53 2017

Better image for copyTo4444 test

This one produces noticeable differences if we fail to dither.

Bug:720105
Change-Id: I208d0c8147f4cca1b484f2f55edc09ce1bef2dcb
Reviewed-on: https://skia-review.googlesource.com/18036
Commit-Queue: Mike Klein <mtklein@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>

[add] https://crrev.com/d16084ffdc688bad6d0e9c04f3f98049b3412633/resources/dog.jpg
[modify] https://crrev.com/d16084ffdc688bad6d0e9c04f3f98049b3412633/gm/copyTo4444.cpp

I figured out the bug is fixed with software raster but broken in GPU raster.  GPU raster is currently on finch trial for low-end devices: http://crbug.com/704716, so we would repro or not randomly based on whether the trial was active (although we didn't notice it in "variations" list for some reason... not sure how that works exactly).

That should be fixed by Skia-team moving forward before we can consider this closed.  But we can consider this fixed in M59 (59 stable will still be 100% software raster).

I'm going to retarget this to M60 (really, should be targeted to wherever we plan to ship h/w rasterization) so we don't lose track of it, but this doesn't need to block 59 as seen above.

+brianosman

Do we dither when converting to 4444 on GPU readPixels()?  Is that something we can implement?

FWIW, dithering may help, but I still think the real problem is that Clank is using 4444 and expecting it to look good.

Can anyone from Mobile>Webview help us understand the use of 4444 on Android devices?

We use 565 SurfaceView and 4444 tiles on Chrome and CCTs on 512MB phones.  In that configuration there is little enough RAM that OOM kills are very frequent, so Grace made the call that it's worthwhile to trade off some image quality to save ~10MB.  We don't use it on any other configuration (not on 1GB+ devices, and not in WebView even on 512MB devices).

There's no flag, but if you want to test the behavior on Chromium on a higher-end device you can hack base::SysInfo::AmountOfPhysicalMemoryMB() to return 512.

Agreed that we cannot expect perfect image quality in this mode, but we should at least maintain the previous quality bar that we've been shipping as we transition to GPU raster.

I'm going to take a stab at a solution for the remaining GPU raster issues.

I *think* that dithering images at decode time will cover most scenarios people care about (plus it saves us 50% of our image storage space, which is a great side benefit). I'm going to try this out (which should be a pretty easy patch, now that most images funnel through the CC image decode controller). If this proves to be good enough in the GPU raster case, we should consider moving to the same thing for SW raster, as it will both save 50% of our image storage as well as reducing SW raster overhead (no need to dither every tile).

Sounds good to me, thanks.  No reason to store the image as 8888 if we're just going to convert to 4444.

 Issue 726974  has been merged into this issue.

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/2c075e749d1f33dea06ad2710e15c9a1d60ebced

commit 2c075e749d1f33dea06ad2710e15c9a1d60ebced
Author: Eric Karl <ericrk@chromium.org>
Date: Wed Jun 07 20:19:46 2017

DeferredTextureImageData low-bit-depth/dithering support

Cause DeferredTextureImageData functionality to support low bit depth
(4444, 565) image formats (with dithering).

Bug:  720105 
Change-Id: Ie3b5768ebc393d9b0a5322461c722bf37c80b791
Reviewed-on: https://skia-review.googlesource.com/18945
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Eric Karl <ericrk@chromium.org>

[modify] https://crrev.com/2c075e749d1f33dea06ad2710e15c9a1d60ebced/src/core/SkAutoPixmapStorage.cpp
[modify] https://crrev.com/2c075e749d1f33dea06ad2710e15c9a1d60ebced/src/image/SkImage_Gpu.cpp
[modify] https://crrev.com/2c075e749d1f33dea06ad2710e15c9a1d60ebced/gm/deferredtextureimage.cpp
[modify] https://crrev.com/2c075e749d1f33dea06ad2710e15c9a1d60ebced/src/core/SkAutoPixmapStorage.h
[modify] https://crrev.com/2c075e749d1f33dea06ad2710e15c9a1d60ebced/include/core/SkImage.h
[modify] https://crrev.com/2c075e749d1f33dea06ad2710e15c9a1d60ebced/tests/ImageTest.cpp

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

commit 3e7226f51354c416d1bec1230b2a65c608581343
Author: Eric Karl <ericrk@chromium.org>
Date: Thu Jun 08 01:19:23 2017

Decode/Upload GPU images to tile color type (support 4444)

Makes use of new Skia logic which allows us to decode / upload GPU
images to dithered ARGB_4444 if desired. This looks much better on low
bit depth devices (low-end Android).

Bug:  720105 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I2cb73c3fcaaf8e98527c488db2c2a1cb55893e08
Reviewed-on: https://chromium-review.googlesource.com/527658
Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477847}
[modify] https://crrev.com/3e7226f51354c416d1bec1230b2a65c608581343/cc/tiles/gpu_image_decode_cache.cc

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

commit d7c681d6a7b154d276e275becc51685eac8705c2
Author: Brian Salomon <bsalomon@google.com>
Date: Thu Jun 08 13:03:50 2017

Revert "DeferredTextureImageData low-bit-depth/dithering support"

This reverts commit 2c075e749d1f33dea06ad2710e15c9a1d60ebced.

Reason for revert: Breaking tests. e.g.: https://chromium-swarm.appspot.com/task?id=369dc44f62ce9510&refresh=10

Original change's description:
> DeferredTextureImageData low-bit-depth/dithering support
> 
> Cause DeferredTextureImageData functionality to support low bit depth
> (4444, 565) image formats (with dithering).
> 
> Bug:  720105 
> Change-Id: Ie3b5768ebc393d9b0a5322461c722bf37c80b791
> Reviewed-on: https://skia-review.googlesource.com/18945
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Eric Karl <ericrk@chromium.org>

TBR=bsalomon@google.com,ericrk@chromium.org
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  720105 

Change-Id: I07aec722425efc62bc54f82cee9a19a9bf339f7b
Reviewed-on: https://skia-review.googlesource.com/19039
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/d7c681d6a7b154d276e275becc51685eac8705c2/src/core/SkAutoPixmapStorage.cpp
[modify] https://crrev.com/d7c681d6a7b154d276e275becc51685eac8705c2/src/image/SkImage_Gpu.cpp
[modify] https://crrev.com/d7c681d6a7b154d276e275becc51685eac8705c2/gm/deferredtextureimage.cpp
[modify] https://crrev.com/d7c681d6a7b154d276e275becc51685eac8705c2/src/core/SkAutoPixmapStorage.h
[modify] https://crrev.com/d7c681d6a7b154d276e275becc51685eac8705c2/include/core/SkImage.h
[modify] https://crrev.com/d7c681d6a7b154d276e275becc51685eac8705c2/tests/ImageTest.cpp

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/840ff8837bd93ee73c187890839b3f56e7c94fb7

commit 840ff8837bd93ee73c187890839b3f56e7c94fb7
Author: Brian Salomon <bsalomon@google.com>
Date: Thu Jun 08 13:32:30 2017

Revert "Revert "DeferredTextureImageData low-bit-depth/dithering support""

This reverts commit d7c681d6a7b154d276e275becc51685eac8705c2.

Reason for revert: New param already used in Chrome

Original change's description:
> Revert "DeferredTextureImageData low-bit-depth/dithering support"
> 
> This reverts commit 2c075e749d1f33dea06ad2710e15c9a1d60ebced.
> 
> Reason for revert: Breaking tests. e.g.: https://chromium-swarm.appspot.com/task?id=369dc44f62ce9510&refresh=10
> 
> Original change's description:
> > DeferredTextureImageData low-bit-depth/dithering support
> > 
> > Cause DeferredTextureImageData functionality to support low bit depth
> > (4444, 565) image formats (with dithering).
> > 
> > Bug:  720105 
> > Change-Id: Ie3b5768ebc393d9b0a5322461c722bf37c80b791
> > Reviewed-on: https://skia-review.googlesource.com/18945
> > Reviewed-by: Brian Salomon <bsalomon@google.com>
> > Commit-Queue: Eric Karl <ericrk@chromium.org>
> 
> TBR=bsalomon@google.com,ericrk@chromium.org
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  720105 
> 
> Change-Id: I07aec722425efc62bc54f82cee9a19a9bf339f7b
> Reviewed-on: https://skia-review.googlesource.com/19039
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Brian Salomon <bsalomon@google.com>

TBR=bsalomon@google.com,reviews@skia.org,ericrk@chromium.org
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  720105 

Change-Id: I91e690d0564f04209a2bd677de9ae9eb9c0f90d3
Reviewed-on: https://skia-review.googlesource.com/19041
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/840ff8837bd93ee73c187890839b3f56e7c94fb7/src/core/SkAutoPixmapStorage.cpp
[modify] https://crrev.com/840ff8837bd93ee73c187890839b3f56e7c94fb7/src/image/SkImage_Gpu.cpp
[modify] https://crrev.com/840ff8837bd93ee73c187890839b3f56e7c94fb7/gm/deferredtextureimage.cpp
[modify] https://crrev.com/840ff8837bd93ee73c187890839b3f56e7c94fb7/src/core/SkAutoPixmapStorage.h
[modify] https://crrev.com/840ff8837bd93ee73c187890839b3f56e7c94fb7/include/core/SkImage.h
[modify] https://crrev.com/840ff8837bd93ee73c187890839b3f56e7c94fb7/tests/ImageTest.cpp

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

commit b8edd52416f1e93bf2330543f5b888d2dadc9e14
Author: Brian Salomon <bsalomon@chromium.org>
Date: Thu Jun 08 14:01:35 2017

Revert "Decode/Upload GPU images to tile color type (support 4444)"

This reverts commit 3e7226f51354c416d1bec1230b2a65c608581343.

Reason for revert: Skia change this is dependent upon needs to be reverted.

Original change's description:
> Decode/Upload GPU images to tile color type (support 4444)
> 
> Makes use of new Skia logic which allows us to decode / upload GPU
> images to dithered ARGB_4444 if desired. This looks much better on low
> bit depth devices (low-end Android).
> 
> Bug:  720105 
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I2cb73c3fcaaf8e98527c488db2c2a1cb55893e08
> Reviewed-on: https://chromium-review.googlesource.com/527658
> Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
> Commit-Queue: Eric Karl <ericrk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#477847}

TBR=vmpstr@chromium.org,ericrk@chromium.org
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  720105 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel

Change-Id: I99978e01a9b744548fffb5befec92473ab3d8ab0
Reviewed-on: https://chromium-review.googlesource.com/527355
Reviewed-by: Brian Salomon <bsalomon@chromium.org>
Commit-Queue: Brian Salomon <bsalomon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477954}
[modify] https://crrev.com/b8edd52416f1e93bf2330543f5b888d2dadc9e14/cc/tiles/gpu_image_decode_cache.cc

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/7fbeb5acdd925f9d04ccb1316fbcd3490b3f07ec

commit 7fbeb5acdd925f9d04ccb1316fbcd3490b3f07ec
Author: Brian Salomon <bsalomon@google.com>
Date: Thu Jun 08 14:02:20 2017

Revert "Revert "Revert "DeferredTextureImageData low-bit-depth/dithering support"""

This reverts commit 840ff8837bd93ee73c187890839b3f56e7c94fb7.

Reason for revert: Not obvious to me how to fix, reverting the chrome CL that was dependent on this.

Original change's description:
> Revert "Revert "DeferredTextureImageData low-bit-depth/dithering support""
> 
> This reverts commit d7c681d6a7b154d276e275becc51685eac8705c2.
> 
> Reason for revert: New param already used in Chrome
> 
> Original change's description:
> > Revert "DeferredTextureImageData low-bit-depth/dithering support"
> > 
> > This reverts commit 2c075e749d1f33dea06ad2710e15c9a1d60ebced.
> > 
> > Reason for revert: Breaking tests. e.g.: https://chromium-swarm.appspot.com/task?id=369dc44f62ce9510&refresh=10
> > 
> > Original change's description:
> > > DeferredTextureImageData low-bit-depth/dithering support
> > > 
> > > Cause DeferredTextureImageData functionality to support low bit depth
> > > (4444, 565) image formats (with dithering).
> > > 
> > > Bug:  720105 
> > > Change-Id: Ie3b5768ebc393d9b0a5322461c722bf37c80b791
> > > Reviewed-on: https://skia-review.googlesource.com/18945
> > > Reviewed-by: Brian Salomon <bsalomon@google.com>
> > > Commit-Queue: Eric Karl <ericrk@chromium.org>
> > 
> > TBR=bsalomon@google.com,ericrk@chromium.org
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug:  720105 
> > 
> > Change-Id: I07aec722425efc62bc54f82cee9a19a9bf339f7b
> > Reviewed-on: https://skia-review.googlesource.com/19039
> > Reviewed-by: Brian Salomon <bsalomon@google.com>
> > Commit-Queue: Brian Salomon <bsalomon@google.com>
> 
> TBR=bsalomon@google.com,reviews@skia.org,ericrk@chromium.org
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  720105 
> 
> Change-Id: I91e690d0564f04209a2bd677de9ae9eb9c0f90d3
> Reviewed-on: https://skia-review.googlesource.com/19041
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Brian Salomon <bsalomon@google.com>

TBR=bsalomon@google.com,reviews@skia.org,ericrk@chromium.org
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  720105 

Change-Id: I63eb0051964ce99e187b4e4943ea79bc22d392d2
Reviewed-on: https://skia-review.googlesource.com/19046
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/7fbeb5acdd925f9d04ccb1316fbcd3490b3f07ec/src/core/SkAutoPixmapStorage.cpp
[modify] https://crrev.com/7fbeb5acdd925f9d04ccb1316fbcd3490b3f07ec/src/image/SkImage_Gpu.cpp
[modify] https://crrev.com/7fbeb5acdd925f9d04ccb1316fbcd3490b3f07ec/gm/deferredtextureimage.cpp
[modify] https://crrev.com/7fbeb5acdd925f9d04ccb1316fbcd3490b3f07ec/src/core/SkAutoPixmapStorage.h
[modify] https://crrev.com/7fbeb5acdd925f9d04ccb1316fbcd3490b3f07ec/include/core/SkImage.h
[modify] https://crrev.com/7fbeb5acdd925f9d04ccb1316fbcd3490b3f07ec/tests/ImageTest.cpp

Hi, just to let yous know, ive just updated to 60.0.3112.20 and im still experiencing the same image resolution issue. 

My change will likely address this for Beta/Canary - it was reverted due to a small issue, and I'm trying to re-land it today.

I will try to get this in for the next Beta (next week). Thanks!

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/7a8c84c6c92565842aeea27d4971fbd78d523f7a

commit 7a8c84c6c92565842aeea27d4971fbd78d523f7a
Author: Eric Karl <ericrk@chromium.org>
Date: Mon Jun 12 18:27:48 2017

Reland DeferredTextureImageData low-bit-depth/dithering support

Cause DeferredTextureImageData functionality to support low bit depth
(4444, 565) image formats (with dithering).

Updated to handle colorspace + 4444 colortype correctly.

Bug:  720105 
Change-Id: Ib7e14d937849f4f6b08fda6992a240bb203d0089
Reviewed-on: https://skia-review.googlesource.com/19094
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/7a8c84c6c92565842aeea27d4971fbd78d523f7a/tests/ImageTest.cpp
[modify] https://crrev.com/7a8c84c6c92565842aeea27d4971fbd78d523f7a/src/image/SkImage_Gpu.cpp
[modify] https://crrev.com/7a8c84c6c92565842aeea27d4971fbd78d523f7a/src/image/SkImage.cpp
[modify] https://crrev.com/7a8c84c6c92565842aeea27d4971fbd78d523f7a/gm/deferredtextureimage.cpp
[modify] https://crrev.com/7a8c84c6c92565842aeea27d4971fbd78d523f7a/src/core/SkAutoPixmapStorage.h
[modify] https://crrev.com/7a8c84c6c92565842aeea27d4971fbd78d523f7a/include/core/SkImage.h
[modify] https://crrev.com/7a8c84c6c92565842aeea27d4971fbd78d523f7a/src/core/SkAutoPixmapStorage.cpp

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

commit adf17082e30e12ff45657e22d5072a37d3bd83ff
Author: Eric Karl <ericrk@chromium.org>
Date: Tue Jun 13 18:00:24 2017

Reland: Decode/Upload GPU images to tile color type (support 4444)

Makes use of new Skia logic which allows us to decode / upload GPU
images to dithered ARGB_4444 if desired. This looks much better on low
bit depth devices (low-end Android).

Bug:  720105 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I0da734a46473e675af476545cd2e5b23a0b6f6c8
Reviewed-on: https://chromium-review.googlesource.com/531629
Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479064}
[modify] https://crrev.com/adf17082e30e12ff45657e22d5072a37d3bd83ff/cc/tiles/gpu_image_decode_cache.cc

Requesting merge of #60 and #61 (pair of Skia/Chrome changes) to M61.

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

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

Correction, #62 should have said "requesting merge... to M60"

I'm concerned that we did not detect the bug until users had to send us feedback repeatedly.  Do we have high confidence that there are not other regressions lurking for H/W rasterization on low end devices?  Are there any other known defects that are still pending fixes / merges?

I'm moving this to Merge-TBD pending answers to the questions above, please move back to Merge-Review-60 once answered.

Re #65 - this wasn't a H/W raster specific regression - in fact, the same regression was present in SW in M58. From what I understand, this regression made it to M58 Stable for SW raster, and was fixed for SW raster in comment #34 for M59. During the period when GPU raster was being added to low end Android (M58/M59 timeframe) GPU raster matched SW raster, so this issue was missed.

GPU raster hasn't shipped on low-end Android yet (it's at 50% in Beta), so we didn't need to rush to get a GPU raster fix into M59. Instead, this change is landing a fix for M60.

The raster differences between Normal Android and Low-End Android are small (low-bit depth, seen here, being the main one), so I don't expect additional issues like this. There are no other GPU raster on low-end Android defects pending fix/merge (and I believe that this has been the only low-end-Android specific issue reported so far). Finally, we plan to let GPU raster soak at 50% in Beta for the remainder of M60.

In the changes above, Skia tests have been landed to ensure that we maintain dithering in both the SW and GPU paths in the future.

Let me know if this makes sense. Thanks!

removing RBS as the SW raster issue which was fixed in M59 is the only release-blocking part.

Given what I said in #66, I decided to take another deep look at this area of code and work on some additional test cases, to make sure I hadn't missed anything. Unfortunately, I believe I've found a second related (although less severe) issue in the same area of code.

amineer@, let me know how you'd like to proceed. If possible, I'd still like to merge this fix.

Thanks for the details and reviewing related code, appreciate it. Approved for M60 branch 3112.

Hi, just to let yous know. Ive just updated to 60.0.3112.33 and im still having the same issue.
I know yous are really trying hard to fix it, so thank you for your efforts.

Thanks phoneemailonly123@ - beta is not expected to have the fix this week, but the version we release next should have it.  Let me know if the next beta does not work, it should come out sometime next week.

Thank you for providing us feedback, we appreciate it!

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

commit bb20c118c615565e31f1bd0bff0f6c9387f3d0cf
Author: Eric Karl <ericrk@chromium.org>
Date: Fri Jun 16 17:03:09 2017

Reland DeferredTextureImageData low-bit-depth/dithering support

Merge to M60

Cause DeferredTextureImageData functionality to support low bit depth
(4444, 565) image formats (with dithering).

Updated to handle colorspace + 4444 colortype correctly.

No-Tree-Checks: true
No-Try: true
No-Presubmit: true
Bug:  720105 
Change-Id: Ib7e14d937849f4f6b08fda6992a240bb203d0089
Reviewed-On: https://skia-review.googlesource.com/19094
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-By: Brian Salomon <bsalomon@google.com>
Reviewed-on: https://skia-review.googlesource.com/20128

[modify] https://crrev.com/bb20c118c615565e31f1bd0bff0f6c9387f3d0cf/tests/ImageTest.cpp
[modify] https://crrev.com/bb20c118c615565e31f1bd0bff0f6c9387f3d0cf/src/image/SkImage_Gpu.cpp
[modify] https://crrev.com/bb20c118c615565e31f1bd0bff0f6c9387f3d0cf/src/image/SkImage.cpp
[modify] https://crrev.com/bb20c118c615565e31f1bd0bff0f6c9387f3d0cf/gm/deferredtextureimage.cpp
[modify] https://crrev.com/bb20c118c615565e31f1bd0bff0f6c9387f3d0cf/src/core/SkAutoPixmapStorage.h
[modify] https://crrev.com/bb20c118c615565e31f1bd0bff0f6c9387f3d0cf/include/core/SkImage.h
[modify] https://crrev.com/bb20c118c615565e31f1bd0bff0f6c9387f3d0cf/src/core/SkAutoPixmapStorage.cpp

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

commit c6432a860679782fee787af285ba7a7aa8ca9ab9
Author: Eric Karl <ericrk@chromium.org>
Date: Fri Jun 16 18:32:10 2017

Reland: Decode/Upload GPU images to tile color type (support 4444)

Makes use of new Skia logic which allows us to decode / upload GPU
images to dithered ARGB_4444 if desired. This looks much better on low
bit depth devices (low-end Android).

Bug:  720105 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I0da734a46473e675af476545cd2e5b23a0b6f6c8
Reviewed-on: https://chromium-review.googlesource.com/531629
Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#479064}
Review-Url: https://codereview.chromium.org/2939413002 .
Cr-Commit-Position: refs/branch-heads/3112@{#362}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/c6432a860679782fee787af285ba7a7aa8ca9ab9/cc/tiles/gpu_image_decode_cache.cc

This issue has been merged to Beta - should be in next week's beta (check for a version >60.0.3112.32).

Verified on Chrome:60.0.3112.43 Device: Huawei Ascend Y520/4.4.2

Thank You for fixing the problem, ive just updated to 60.0.3112.43, and the images are fine.