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

Issue 601198 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

performance tests for ImageDecoder

Project Member Reported by scroggo@chromium.org, Apr 6 2016

Issue description

We already have a benchmark: ImageDecodeBench [1], but it has bit-rotted, due to the fact that it is not built on any bots. We need to do the following:

- get the benchmark building again
- make it get built in the waterfall so it continues to build (I'm new to Chromium's performance tests; where should this go? [2] recommends using Telemetry, but this is different, since it does not use a webpage/browser. Does Blink have its own performance benchmarks elsewhere?)
- support it with gn (I had to use gyp to get it to build)

Follow-up steps:
- Actually run it on the waterfall? Which files?

It looks like there is a high bar for automation (from [3]: "Whenever possible, avoid creating new benchmarks"), but I think it's reasonable to at least keep it from bit-rotting so developers can use it locally. Noel and I have used it a lot, and others have expressed interest in a benchmark that just times image decodes.

[1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/ImageDecodeBench.cpp&q=imagedecodebench&sq=package:chromium&l=1
[2] http://www.chromium.org/developers/testing/adding-performance-tests
[3] https://docs.google.com/document/d/1bBKyYCW3VlUUPDpQE4xvrMFdA6tovQMZoqO9KCcmqqQ/preview
 

Comment 1 by noel@chromium.org, Apr 9 2016

These seems like good first steps to begin with

  - get the benchmark building again
  - support it with gn
      (I had to use gyp to get it to build)

You have a change to get it building with gyp again?


Had. See https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/web/web.gyp

That may have been a superset of what was needed (since I was also modifying it to time deferred decoding), but those changes (at the time) made it work again.

Today, it won't build at least because of ImageDecoder::qcmsOutputDeviceProfile (which is called in ImageDecodeBench, but has been removed from ImageDecoder). I haven't dug further than that, though. I believe over email you said we need to bring that method back?
Owner: scroggo@chromium.org
Status: Started (was: Untriaged)
Uploaded https://codereview.chromium.org/1879133002/ for review. I commented out ImageDecoder::qcmsOutputDeviceProfile(), which no longer exists on ImageDecoder. I suppose it won't work properly when testing for color profiles, but if not (e.g. if ! USE(QCMSLIB)) it should work as expected.

Comment 4 by cblume@google.com, Apr 12 2016

Once we get to it, I think it would be benefitial to have a separate test for different types of images.

For example, within the gif decoder we could have tests for:
still, animated w/ only references to previous frame, animated w/ references to distant frames, animated w/ only global palette, animated w/ separate palettes

Comment 5 by noel@chromium.org, Apr 13 2016

>Today, it won't build at least because of ImageDecoder::qcmsOutputDeviceProfile (which is called in ImageDecodeBench, but has been removed from ImageDecoder). I haven't dug further than that, though. I believe over email you said we need to bring that method back?

Yeap, we should bring it back.
Project Member

Comment 6 by bugdroid1@chromium.org, May 5 2016

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

commit cab42628d6fe53b18f9d119d3999746c79b33ba5
Author: noel <noel@chromium.org>
Date: Thu May 05 16:59:48 2016

Make ImageDecodeBench build again

Remove unnecessary windows #error re LEAN_AND_MEAN, including
<mmsystem.h> is enough.

Use size_t instead of position for SharedBuffer::getSomeData.

Call base::CommandLine::Init() to avoid a SEGFAULT at start-up
when Platform heap initializes and trys to read CommmandLine.

Inherit from blink::Platform, not TestingPlatformSupport.

Remove missing function from ImageDecoder. Instead, decode once
before the bench loop to initialize Platform statics.

Build: move the gyp into blink_platform_tests.gyp and the .cpp
to platform/testing to allow the call to base::CommandLine per
the rules in platform/testing/DEPS.

This patch is based on previous patch by scroggo@chromium.org,
refer to https://codereview.chromium.org/1879133002

TBR=jochen@chromium.org
BUG=601198

Review-Url: https://codereview.chromium.org/1954673002
Cr-Commit-Position: refs/heads/master@{#391831}

[modify] https://crrev.com/cab42628d6fe53b18f9d119d3999746c79b33ba5/third_party/WebKit/Source/platform/blink_platform_tests.gyp
[rename] https://crrev.com/cab42628d6fe53b18f9d119d3999746c79b33ba5/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp
[modify] https://crrev.com/cab42628d6fe53b18f9d119d3999746c79b33ba5/third_party/WebKit/Source/web/web.gyp

Comment 7 by hcm@chromium.org, Jul 14 2016

Components: Internals>Images>Codecs

Comment 8 by noel@chromium.org, Dec 5 2016

Anything more that needs to happen here?
https://codereview.chromium.org/2483243003/, which recently landed, fights bit-rot and builds with GN.

Next step is to make sure we build it so that it will not bit-rot further.
Project Member

Comment 10 by bugdroid1@chromium.org, Feb 12 2018

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

commit b8644b02a35599e95cf14c05a9e71f02cfb6350e
Author: Noel Gordon <noel@chromium.org>
Date: Mon Feb 12 21:48:12 2018

[image_decode_bench] Remove color-correction option

Remove --color-correction option now that color correction is done by
Skia (which has its own color correction performance tests).

Other cleanup: FrameCount returns size_t, use |index| for frame index
access consistently whether packet mode, or not. Rename iterations to
decode_iterations. Remove blink:: from Platform because we're already
in the blink namespace. total_time is a double so casting the divisor
when computing the average decode time is not needed (due to C++ type
promotion rules). Rename Main to ImageDecodeBenchMain.

Bug: 601198
Change-Id: I2651771e470664f78d54fb78522a98a329d25ed9
Reviewed-on: https://chromium-review.googlesource.com/913028
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536202}
[modify] https://crrev.com/b8644b02a35599e95cf14c05a9e71f02cfb6350e/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Feb 13 2018

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

commit 0342258175d72113ae72c0ce56d79a3784ad2dd0
Author: Noel Gordon <noel@chromium.org>
Date: Tue Feb 13 00:46:55 2018

[image_decode_bench] Change BUILD gn to build an executable

Currently built as a test fixture, which makes it painful to use when
testing on Android: a special script is needed for Android [1], which
makes Android a special snowflake when image benching.

Simpler worlds can be had even for Android: build/create image decode
bench binary executable. Android developers can 'adb push' the binary
to their Android test device, and run it from adb shell (and no fancy
scripts required). For everyone else (mac/linux...), nothing changes,
just run out/Release/image_decode_bench as usual.

[1]  https://crbug.com/762564#c44 

Bug: 601198,  762564 
Change-Id: I7c229f686ea5f6672b2d4d972a010f2bfbccd40c
Reviewed-on: https://chromium-review.googlesource.com/912988
Reviewed-by: Adenilson Cavalcanti <cavalcantii@chromium.org>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536249}
[modify] https://crrev.com/0342258175d72113ae72c0ce56d79a3784ad2dd0/third_party/WebKit/Source/platform/BUILD.gn

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 13 2018

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

commit 9f16a173c9f23cce777a78db5a278d21bf26258b
Author: Noel Gordon <noel@chromium.org>
Date: Tue Feb 13 21:03:24 2018

[image_decode_bench] Introduce ImageMeta data

Define and use an ImageMeta structure: to hold meta data about images
we decode and, in particular, the image decode time. Use that time to
accumlate and report the image decode bench result. Note: the decoder
creation time cost is now excluded from the result.

Also add a DecodeFailure helper: to simplify decode failure handling,
and throw more code away. Errors in the bench are fatal so the helper
reports a failure reason and exits.

Bug: 601198
Change-Id: Iee7e578d91e705eb3289aac45a2e0b0d75807257
Reviewed-on: https://chromium-review.googlesource.com/914215
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536470}
[modify] https://crrev.com/9f16a173c9f23cce777a78db5a278d21bf26258b/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp

Comment 13 by noel@chromium.org, Mar 7 2018

From OP > "Whenever possible, avoid creating new benchmarks"), but I think it's reasonable to at least keep it from bit-rotting so developers can use it locally.

Yes.

> Noel and I have used it a lot, and others have expressed interest in a benchmark that just times image decodes."

In the recently zlib work, bench marking with the new zlib_bench [1] added in  issue 798943  has been very good way for us to vet optimizations locally, and also for external contributors to do the same.

[1] https://cs.chromium.org/search/?q=zlib_bench.cc&sq=package:chromium&type=cs

It is a rate-based bench mark tool, with statistics over kRuns.  As more developers use the bench tools, we wanna be sure they are measuring correctly (low variance results, aka low jitter) [2].

I re-wrote image_decode_bench during this zlib work to capture variance also and I now propose that we move toward that type of bench marker, and remove the old code [iterations] [packeSize]

[2] I have examples of where this was not the case: patches have been allowed to travel to stable channel with regressions in PNG.
Project Member

Comment 14 by bugdroid1@chromium.org, Mar 7 2018

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

commit 355e21674ff6172240118a37d7419fe44c0ca7de
Author: Noel Gordon <noel@chromium.org>
Date: Wed Mar 07 22:36:15 2018

[image_decode_bench] Remove [packetSize] option

Patch to prepare to for the removal of [iterations] option, so we can
replace it with a rate-based bench marker tool similar to zlib_bench.
First step: remove the [packetSize] option.

Bug: 601198
Change-Id: If5fabb8597209dac88b134a71d648a950910c86d
Reviewed-on: https://chromium-review.googlesource.com/952603
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541616}
[modify] https://crrev.com/355e21674ff6172240118a37d7419fe44c0ca7de/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp

Project Member

Comment 15 by bugdroid1@chromium.org, Mar 9 2018

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

commit b8e15ca8e63e539beeeb1d67c14b5d6de590c1d6
Author: Noel Gordon <noel@chromium.org>
Date: Fri Mar 09 14:34:38 2018

[image_decode_bench] Remove use of SharedBuffer::Data()

SharedBuffer::Data() is deprecated, and was being used to consolidate
the buffer into one contiguous piece of memory. This is now redundant
since the ShardBuffer is created in ReadFile() by adopting the Vector
of image data (which is a contiguous piece of memory under the hood).

Remove the SharedBuffer::Data() call, move the related error handling
for ReadFile() back into that routine.

Bug: 601198
Change-Id: Ic0eab97ead78a967180d1ee6bdcb708e3cc3a4e5
Reviewed-on: https://chromium-review.googlesource.com/956644
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542102}
[modify] https://crrev.com/b8e15ca8e63e539beeeb1d67c14b5d6de590c1d6/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp

Sign in to add a comment