(responding to https://crbug.com/768612#c9): This appears to be a paint-time regression on a benchmark that has no images. I... can't explain that, but two bots agreed that the patch was the cause. Could you try confirming locally? Could there be static initializers in the new code?

I ran locally 3 times each (with and without this patch).


With patch:
avg 380.04750000000007 ms
median 371.5600000000004 ms
stdev 27.24759839060199 ms
min 356.22000000000116 ms
max 444.77499999999964 ms

avg 393.60049999999995 ms
median 403.1725000000006 ms
stdev 22.39951939588568 ms
min 352.2999999999993 ms
max 417.96500000000015 ms

avg 375.2895000000001 ms
median 367.1300000000001 ms
stdev 32.865209353255466 ms
min 341.1800000000003 ms
max 426.90499999999884 ms


Without patch:
avg 348.2884999999999 ms
median 343.8049999999994 ms
stdev 9.86970196837007 ms
min 340.7500000000009 ms
max 366.1849999999995 ms

avg 347.5215000000001 ms
median 336.08999999999924 ms
stdev 20.16123219476217 ms
min 333.0549999999994 ms
max 394.4200000000001 ms

avg 360.5690000000002 ms
median 355.625 ms
stdev 14.05078819457786 ms
min 349.0750000000007 ms
max 398.375 ms


It does indeed appear slower after this patch.
The average and median are both higher.
The min seems unchanged.
The max seems higher but fluctuates wildly.


Seeing no images in the test, I'm also pretty unclear on this. There shouldn't be any new static initializers.

There are a few static initializers in SkCodec; mostly simple values, but there is an array of function pointers. https://skia-review.googlesource.com/c/skia/+/54101 converts to using constexpr for these.

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

commit 862c19675edb26ed7cba56ae6ca9f98c1e4cbef1
Author: Leon Scroggins III <scroggo@google.com>
Date: Tue Oct 03 12:35:58 2017

Remove static initializers in SkCodec

Bug: 768878

Switch const declarations to constexpr where appropriate. Speculative
fix for crbug.com/768878.

Change-Id: I7fc356e623ce7a0f2b87e92e9a8ed95d5c423d79
Reviewed-on: https://skia-review.googlesource.com/54101
Commit-Queue: Leon Scroggins <scroggo@google.com>
Reviewed-by: Chris Blume <cblume@chromium.org>

[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkBmpCodec.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkIcoCodec.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkMasks.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkAndroidCodec.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkJpegCodec.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkBmpCodec.h
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkCodec.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkPngCodec.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkGifCodec.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkBmpRLECodec.cpp
[modify] https://crrev.com/862c19675edb26ed7cba56ae6ca9f98c1e4cbef1/src/codec/SkSwizzler.cpp

Sadly I think the regression persists :'(

https://chromeperf.appspot.com/report?sid=2327871bf661d3218fe59001b6dc67d4d9860084d4256fa787cb502fd5322bc8&start_rev=494327&end_rev=506499

Chris, do you have a Windows build? I am trying to set one up to run the static_initializers executable (as in https://codereview.chromium.org/2942273002), but it's slow going. (FWIW, I'm currently stuck on installing "Debugging Tools for Windows".)

Re #8: Still no luck... (It's been a while since I've used a Windows machine, and Win 10 seems to be actively working against me!)

But I did manage to run the static_initializers tool on a (somewhat old - April, I think) Skia build, and it turned up nothing in SkCodec.

I think we should roll out this change (for https://crbug.com/768612) to get numbers on dev channel and see if this was the cause of the Blink.Compositing.UpdateTime regression. cblume@, would you be willing to do that?

Just to make sure I understand: "roll out" means revert? That sounds like the right approach to me. From [1] and the fact that the test has no images, it sounds like the problem is simply due to building SkCodec. If that is the case, we'll also need to revert https://chromium-review.googlesource.com/668408, which uses SkCodec elsewhere.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=768612#c14

Oh, one more important point - reverting https://chromium-review.googlesource.com/668408 will re-break  issue 758459 .

That bug could alternatively be fixed by making the code that attempts to encode to PNG (referenced in [1]) fail (like it used to) and instead serialize the raw pixels.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=758459#c21

Oops, I'm sorry. Comment 11 should have said "Blink.Paint.UpdateTime". I think this patch could be responsible for Blink.Paint.UpdateTime but it's unlikely to have affected Blink.Compositing.UpdateTime.

I just checked with the release folks and M63 has branched so I think we should revert this (and dependencies) and let that roll to dev channel for some numbers. If Blink.Paint.UpdateTime does not improve, we can re-land.

I'm happy to revert GIFImageDecoder. Any change in numbers will confirm where we should be looking.

But if it is something like function order causing cache misses we'll need to re-land in order to start fixing it. So far, I haven't found any initializers. So I'm actually leaning towards the order.

Feel free to revert https://chromium-review.googlesource.com/668408 if you think it might help. It's a visual issue (and may have been made "obsolete" by a different fix already.)

I've already reverted GIFImageDecoder locally. I've also reverted https://chromium-review.googlesource.com/668408, but I wanted to see if I could make sure I don't re-break  issue 758459 

fs@, is there a different fix you have in mind when you suggest in #17 that the fix may have been made obsolete? That seems to be the case - when I reverted your CL, the test case still works as expected.

Revert is at https://chromium-review.googlesource.com/c/chromium/src/+/718918/2

I believe that a custom pixel serializer was hooked up in the encoding path (disabling it), I don't have it at hand I'm afraid.

Re #21: That's okay. Just wanted to make sure it wasn't a fluke. Thanks!

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

commit fd493b4bc80363d51a4c142ddf287d38e86fc1f5
Author: Leon Scroggins III <scroggo@google.com>
Date: Tue Oct 17 22:25:47 2017

Stop building SkCodec

Bug: 768878

Building SkCodec seems to have caused a paint regression on a webpage
without any images. This leads us to suspect "some minor compiler
optimization tickling". Stop building it to confirm. Two CLs rely on
SkCodec:

"Use SkCodec internally in GIFImageDecoder"
4fed3346549a90c0de40c02f6388e19e8151e92a. This introduced building
SkCodec.

"Enable Skia's SkImageGenerator implementation"
f5eb27c2b897f206b275fd862e874b64159cc15e. This used SkCodec to fix
 crbug.com/758459 , but that seems to have been fixed in another way.

In addition, this corrects some formatting in the old code (as
commanded by presubmit), and makes some other minor changes (no more
PassRefPtr, FrameDurationAtIndex now returns a TimeDelta).

Change-Id: Ic2bdd87740da0232c9c07e27eed6049efc26d76c
Reviewed-on: https://chromium-review.googlesource.com/718918
Commit-Queue: Leon Scroggins <scroggo@chromium.org>
Reviewed-by: Chris Blume <cblume@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509570}
[modify] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/skia/BUILD.gn
[modify] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
[modify] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
[modify] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
[delete] https://crrev.com/26e025feb74e49f320a1ca9cdafdfbd04827c723/third_party/WebKit/Source/platform/image-decoders/SegmentStream.cpp
[delete] https://crrev.com/26e025feb74e49f320a1ca9cdafdfbd04827c723/third_party/WebKit/Source/platform/image-decoders/SegmentStream.h
[delete] https://crrev.com/26e025feb74e49f320a1ca9cdafdfbd04827c723/third_party/WebKit/Source/platform/image-decoders/SegmentStreamTest.cpp
[modify] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
[modify] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
[modify] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp
[add] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
[add] https://crrev.com/fd493b4bc80363d51a4c142ddf287d38e86fc1f5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.h

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

commit 445fcc902e78722f0c675c2ef2d598ad44915ab1
Author: Reilly Grant <reillyg@chromium.org>
Date: Tue Oct 17 23:04:40 2017

Revert "Stop building SkCodec"

This reverts commit fd493b4bc80363d51a4c142ddf287d38e86fc1f5.

Reason for revert: Broken build: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Builder%20%28dbg%29/builds/114259

Original change's description:
> Stop building SkCodec
> 
> Bug: 768878
> 
> Building SkCodec seems to have caused a paint regression on a webpage
> without any images. This leads us to suspect "some minor compiler
> optimization tickling". Stop building it to confirm. Two CLs rely on
> SkCodec:
> 
> "Use SkCodec internally in GIFImageDecoder"
> 4fed3346549a90c0de40c02f6388e19e8151e92a. This introduced building
> SkCodec.
> 
> "Enable Skia's SkImageGenerator implementation"
> f5eb27c2b897f206b275fd862e874b64159cc15e. This used SkCodec to fix
>  crbug.com/758459 , but that seems to have been fixed in another way.
> 
> In addition, this corrects some formatting in the old code (as
> commanded by presubmit), and makes some other minor changes (no more
> PassRefPtr, FrameDurationAtIndex now returns a TimeDelta).
> 
> Change-Id: Ic2bdd87740da0232c9c07e27eed6049efc26d76c
> Reviewed-on: https://chromium-review.googlesource.com/718918
> Commit-Queue: Leon Scroggins <scroggo@chromium.org>
> Reviewed-by: Chris Blume <cblume@chromium.org>
> Reviewed-by: Fredrik Söderquist <fs@opera.com>
> Reviewed-by: Leon Scroggins <scroggo@chromium.org>
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#509570}

TBR=scroggo@chromium.org,pdr@chromium.org,fs@opera.com,cblume@chromium.org

Change-Id: Ib4c5be2e885f483d881ca65689cb5e9f3dc755df
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 768878
Reviewed-on: https://chromium-review.googlesource.com/724359
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509577}
[modify] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/skia/BUILD.gn
[modify] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
[modify] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
[modify] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
[add] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/SegmentStream.cpp
[add] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/SegmentStream.h
[add] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/SegmentStreamTest.cpp
[modify] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
[modify] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
[modify] https://crrev.com/445fcc902e78722f0c675c2ef2d598ad44915ab1/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp
[delete] https://crrev.com/d36ebac42d7bb1c0dd070e768b6c821215adb58a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
[delete] https://crrev.com/d36ebac42d7bb1c0dd070e768b6c821215adb58a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.h

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

commit 4f11c563cb7e664469e8199232815002ed419af8
Author: ccameron chromium <ccameron@chromium.org>
Date: Tue Oct 17 23:35:59 2017

Revert "Stop building SkCodec"

This reverts commit fd493b4bc80363d51a4c142ddf287d38e86fc1f5.

Reason for revert: Causing compile failures, unresolved externals
SkCodec::getPixels
SkCodec::startIncrementalDecode
SkCodec::startScanlineDecode


Original change's description:
> Stop building SkCodec
> 
> Bug: 768878
> 
> Building SkCodec seems to have caused a paint regression on a webpage
> without any images. This leads us to suspect "some minor compiler
> optimization tickling". Stop building it to confirm. Two CLs rely on
> SkCodec:
> 
> "Use SkCodec internally in GIFImageDecoder"
> 4fed3346549a90c0de40c02f6388e19e8151e92a. This introduced building
> SkCodec.
> 
> "Enable Skia's SkImageGenerator implementation"
> f5eb27c2b897f206b275fd862e874b64159cc15e. This used SkCodec to fix
>  crbug.com/758459 , but that seems to have been fixed in another way.
> 
> In addition, this corrects some formatting in the old code (as
> commanded by presubmit), and makes some other minor changes (no more
> PassRefPtr, FrameDurationAtIndex now returns a TimeDelta).
> 
> Change-Id: Ic2bdd87740da0232c9c07e27eed6049efc26d76c
> Reviewed-on: https://chromium-review.googlesource.com/718918
> Commit-Queue: Leon Scroggins <scroggo@chromium.org>
> Reviewed-by: Chris Blume <cblume@chromium.org>
> Reviewed-by: Fredrik Söderquist <fs@opera.com>
> Reviewed-by: Leon Scroggins <scroggo@chromium.org>
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#509570}

TBR=scroggo@chromium.org,pdr@chromium.org,fs@opera.com,cblume@chromium.org

Change-Id: I7871671628746680fd727708aed8736402d75ada
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 768878
Reviewed-on: https://chromium-review.googlesource.com/724381
Reviewed-by: ccameron chromium <ccameron@chromium.org>
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509595}

I think the build problem (that required a revert) is because SkPixmap now depends on SkCodec for an enum. https://skia-review.googlesource.com/c/skia/+/60721 moves that enum to its own file, so once that rolls into Chromium I'll try to revert again, and select the builder that failed.

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

commit b6ab10f34b407d9db9efb69b31849cef010328e3
Author: Leon Scroggins III <scroggo@google.com>
Date: Wed Oct 18 19:08:16 2017

Move SkCodec::Origin into its own file

It is now used by SkPixmap and will soon be in SkJpegEncoder. No need
for those to depend on SkCodec.

Bug: 768878
TBR=reed@google.com
(reed@ already approved the API change in
https://skia-review.googlesource.com/60721)
Change-Id: If1a6e1d5b60a7a3d8c97818e15a48d28ba804668
Reviewed-on: https://skia-review.googlesource.com/61680
Reviewed-by: Leon Scroggins <scroggo@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>

[add] https://crrev.com/b6ab10f34b407d9db9efb69b31849cef010328e3/include/codec/SkEncodedOrigin.h
[modify] https://crrev.com/b6ab10f34b407d9db9efb69b31849cef010328e3/tests/ExifTest.cpp
[modify] https://crrev.com/b6ab10f34b407d9db9efb69b31849cef010328e3/src/codec/SkCodecImageGenerator.cpp
[modify] https://crrev.com/b6ab10f34b407d9db9efb69b31849cef010328e3/include/codec/SkCodec.h
[modify] https://crrev.com/b6ab10f34b407d9db9efb69b31849cef010328e3/src/core/SkPixmapPriv.h
[modify] https://crrev.com/b6ab10f34b407d9db9efb69b31849cef010328e3/src/codec/SkJpegCodec.h
[modify] https://crrev.com/b6ab10f34b407d9db9efb69b31849cef010328e3/src/codec/SkCodec.cpp
[modify] https://crrev.com/b6ab10f34b407d9db9efb69b31849cef010328e3/src/codec/SkJpegCodec.cpp

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

commit 8bf6a886aad1c42210fd16f372097236db34c162
Author: Leon Scroggins III <scroggo@google.com>
Date: Mon Oct 23 18:04:39 2017

Reland "Stop building SkCodec"

This is a reland of fd493b4bc80363d51a4c142ddf287d38e86fc1f5
Original change's description:
> Stop building SkCodec
> 
> Bug: 768878
> 
> Building SkCodec seems to have caused a paint regression on a webpage
> without any images. This leads us to suspect "some minor compiler
> optimization tickling". Stop building it to confirm. Two CLs rely on
> SkCodec:
> 
> "Use SkCodec internally in GIFImageDecoder"
> 4fed3346549a90c0de40c02f6388e19e8151e92a. This introduced building
> SkCodec.
> 
> "Enable Skia's SkImageGenerator implementation"
> f5eb27c2b897f206b275fd862e874b64159cc15e. This used SkCodec to fix
>  crbug.com/758459 , but that seems to have been fixed in another way.
> 
> In addition, this corrects some formatting in the old code (as
> commanded by presubmit), and makes some other minor changes (no more
> PassRefPtr, FrameDurationAtIndex now returns a TimeDelta).
> 
> Change-Id: Ic2bdd87740da0232c9c07e27eed6049efc26d76c
> Reviewed-on: https://chromium-review.googlesource.com/718918
> Commit-Queue: Leon Scroggins <scroggo@chromium.org>
> Reviewed-by: Chris Blume <cblume@chromium.org>
> Reviewed-by: Fredrik Söderquist <fs@opera.com>
> Reviewed-by: Leon Scroggins <scroggo@chromium.org>
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#509570}

Bug: 768878
Change-Id: I18ce8032a1154f222d7392ac5a48b4cd5ec31672
Reviewed-on: https://chromium-review.googlesource.com/730484
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Commit-Queue: Leon Scroggins <scroggo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510847}
[modify] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/skia/BUILD.gn
[modify] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
[modify] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
[modify] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
[delete] https://crrev.com/703e9e62cd9fda43c2b4c9924fc3d41977d621fe/third_party/WebKit/Source/platform/image-decoders/SegmentStream.cpp
[delete] https://crrev.com/703e9e62cd9fda43c2b4c9924fc3d41977d621fe/third_party/WebKit/Source/platform/image-decoders/SegmentStream.h
[delete] https://crrev.com/703e9e62cd9fda43c2b4c9924fc3d41977d621fe/third_party/WebKit/Source/platform/image-decoders/SegmentStreamTest.cpp
[modify] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
[modify] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
[modify] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp
[add] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
[add] https://crrev.com/8bf6a886aad1c42210fd16f372097236db34c162/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.h

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

commit a57e28529a8dd63261cb59fe9f21139cb3a0d5e5
Author: Leon Scroggins III <scroggo@google.com>
Date: Wed Nov 01 12:07:49 2017

Cherry-pick 'Reland "Stop building SkCodec"'

Bug:  779261 

This is a reland of fd493b4bc80363d51a4c142ddf287d38e86fc1f5
Original change's description:
> Stop building SkCodec
>
> Bug: 768878
>
> Building SkCodec seems to have caused a paint regression on a webpage
> without any images. This leads us to suspect "some minor compiler
> optimization tickling". Stop building it to confirm. Two CLs rely on
> SkCodec:
>
> "Use SkCodec internally in GIFImageDecoder"
> 4fed3346549a90c0de40c02f6388e19e8151e92a. This introduced building
> SkCodec.
>
> "Enable Skia's SkImageGenerator implementation"
> f5eb27c2b897f206b275fd862e874b64159cc15e. This used SkCodec to fix
>  crbug.com/758459 , but that seems to have been fixed in another way.
>
> In addition, this corrects some formatting in the old code (as
> commanded by presubmit), and makes some other minor changes (no more
> PassRefPtr, FrameDurationAtIndex now returns a TimeDelta).
>
> Change-Id: Ic2bdd87740da0232c9c07e27eed6049efc26d76c
> Reviewed-on: https://chromium-review.googlesource.com/718918
> Commit-Queue: Leon Scroggins <scroggo@chromium.org>
> Reviewed-by: Chris Blume <cblume@chromium.org>
> Reviewed-by: Fredrik Söderquist <fs@opera.com>
> Reviewed-by: Leon Scroggins <scroggo@chromium.org>
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#509570}

TBR=scroggo@google.com

(cherry picked from commit 8bf6a886aad1c42210fd16f372097236db34c162)

Bug: 768878
Change-Id: I18ce8032a1154f222d7392ac5a48b4cd5ec31672
Reviewed-on: https://chromium-review.googlesource.com/730484
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Commit-Queue: Leon Scroggins <scroggo@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#510847}
Reviewed-on: https://chromium-review.googlesource.com/743691
Cr-Commit-Position: refs/branch-heads/3239@{#332}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/skia/BUILD.gn
[modify] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
[modify] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
[modify] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
[delete] https://crrev.com/52360f276aaddb9da6087ca6a62e61e60c1ac5a6/third_party/WebKit/Source/platform/image-decoders/SegmentStream.cpp
[delete] https://crrev.com/52360f276aaddb9da6087ca6a62e61e60c1ac5a6/third_party/WebKit/Source/platform/image-decoders/SegmentStream.h
[delete] https://crrev.com/52360f276aaddb9da6087ca6a62e61e60c1ac5a6/third_party/WebKit/Source/platform/image-decoders/SegmentStreamTest.cpp
[modify] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
[modify] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
[modify] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp
[add] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
[add] https://crrev.com/a57e28529a8dd63261cb59fe9f21139cb3a0d5e5/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.h

tl;dr -- Are we ready to re-land SkGifCodec? I think yes.

I think this graph might be stable enough that we can consider re-landing SkGifCodec and see if that was indeed the culprit [1]. There was a noticeable drop from ~2.1 ms to ~1.5 ms on 2017-09-28T08:09:51.000Z. And there was another drop from that ~1.5 ms to ~1.3 ms on 2017-10-23T19:01:51.000Z

If SkGifCodec is indeed the culprit, it seems like the graph might bump back up to ~1.5 ms.

Note that we're also seeing a 58 ms speedup per decoded frame by using SkGifCodec in a separate perf test. So this 0.2 ms loss may end up being worth it even if we can't find the cause. (Obviously, that depends on frequency of each event and scheduling pressures.)

[1] https://chromeperf.appspot.com/report?sid=daab5768b66b0688733524bc0d961bf48c03862a98eb34397d6110c1c4ce49ed&start_rev=504328&end_rev=514470

I do think we should re-land this.

Please monitor Blink.Paint.UpdateTime as this rolls out:
https://uma.googleplex.com/timeline_v2/?sid=ac2d3e1e0b615b9ac0cb44dac7e3e5db

Would you be willing to do a simple test of UpdateTime using printf or tracing of a simple gif page with and without the patch? Something with enough gif work that UpdateTime is affected.

I think this can be re-landed now. It looks like the other patch was responsible for the bulk of the regression (see: crbug.com/768612).

Ping.  Was the change ever relanded?

Chris is working on relanding this in https://chromium-review.googlesource.com/c/chromium/src/+/783647

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

commit 0ffa5c7b260f5ac3290bad107d5b74faba967ceb
Author: Chris Blume <cblume@chromium.org>
Date: Wed Sep 19 22:58:02 2018

Reland SkGifCodec

SkGifCodec work was reverted as a suspected cause of a paint regression.
But it may end up being unrelated.

This is a revert of crrev.com/c/718918

Bug: 768878, 778164 , 773548 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ifb72574cf2012d417c223193cbd14b52ec644b9a
Reviewed-on: https://chromium-review.googlesource.com/783647
Commit-Queue: Chris Blume <cblume@chromium.org>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592590}
[modify] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/skia/BUILD.gn
[modify] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/BUILD.gn
[modify] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/gif/gif_image_decoder.cc
[modify] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/gif/gif_image_decoder.h
[modify] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/gif/gif_image_decoder_test.cc
[delete] https://crrev.com/72aa1d1634061e0de286f2dc3291d7791d7f1b40/third_party/blink/renderer/platform/image-decoders/gif/gif_image_reader.cc
[delete] https://crrev.com/72aa1d1634061e0de286f2dc3291d7791d7f1b40/third_party/blink/renderer/platform/image-decoders/gif/gif_image_reader.h
[modify] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/image_decoder.h
[modify] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/image_frame.cc
[modify] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/image_frame.h
[add] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/segment_stream.cc
[add] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/segment_stream.h
[add] https://crrev.com/0ffa5c7b260f5ac3290bad107d5b74faba967ceb/third_party/blink/renderer/platform/image-decoders/segment_stream_test.cc