Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1879133002:
Make ImageDecodeBench build once again (Closed)
|
Created:
4 years, 8 months ago by scroggo_chromium Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ImageDecodeBench build once again
Use size_t instead of position for SharedBuffer::getSomeData.
Inherit from blink::Platform instead of TestingPlatformSupport.
Remove missing function from ImageDecoder. Instead, decode once before the
loop to do any initialization.
Call base::CommandLine::Init
Remove unnecessary windows error.
BUG=601198
Patch Set 1 #
Total comments: 17
Patch Set 2 : Rebase preemptively #Patch Set 3 : Respond to comments #Patch Set 4 : Remove TestingPlatformSupport changes, no longer needed #
Total comments: 3
Messages
Total messages: 16 (3 generated)
scroggo@chromium.org changed reviewers: + noel@chromium.org
Noel, I'll defer to you on how to handle qcmsOutputDeviceProfile(). This builds, but I have little confidence it will last unless we automatically build it.
On 2016/04/12 19:34:34, scroggo_chromium wrote: > Noel, I'll defer to you on how to handle qcmsOutputDeviceProfile(). Yeah not sure why that code was removed. +ccameron@ to advise us. Any reasons for that? > This builds, but I have little confidence it will last unless we automatically build it. Agree, we have a bug for the auto-build, right (this bug)? Mostly, here to get cblume@ going so he can go ahead and measure GIF decode perf. Patch looks fine to me.
noel@chromium.org changed reviewers: + ccameron@chromium.org
https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:371: // FIXME: This method has been removed, but it is important. Could you give a bit more detail about what is "important"? I suspect this impacts the benchmark because the first call to qcmsOutputDeviceProfile() will do a blocking IPC to the browser process, which is an artifact that you don't want to appear in your timing data. If so, a couple of options: First, below, in the for (size_t i = 0; i < iterations; ++i) { you could change that to go through iterations+1 and just toss out the i=0 result. This is probably something you'll want to do anyway. Second, we had some discussion earlier about adding a more predictable and controllable mechanism for communicating device color profiles. We could continue in that direction, and specify the output color profile to a specific color profile there: https://codereview.chromium.org/1796293003/ We should probably do both of these. The first to lower variance. The second because we probably don't want the machine's system settings (its monitor color profile) to be impacting this benchmark.
On 2016/04/21 07:30:42, ccameron wrote: > https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right): > > https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/ImageDecodeBench.cpp:371: // FIXME: This method > has been removed, but it is important. > Could you give a bit more detail about what is "important"? For Blink>Image, we typically test a image changes using an image corpus to assess perf. leon@, do you have an example where you had to do that? > I suspect this impacts the benchmark because the first call to > qcmsOutputDeviceProfile() will do a blocking IPC to the browser process, which > is an artifact that you don't want to appear in your timing data. The tool is a stand-alone executable: there's no browser process, and no IPC. Instead, the tool provides the Platform interface which is used to provide the color profile (see class WebPlatform). Note the color profile is initialized outside of the iteration loop, so that cost is not included in the benchmark results. > Second, we had some discussion earlier about adding a more predictable and > controllable mechanism for communicating device color profiles. We could > continue in that direction, and specify the output color profile to a specific > color profile there: https://codereview.chromium.org/1796293003/ > > We should probably do both of these. The first to lower variance. As mention above, there is no variance. > The second > because we probably don't want the machine's system settings (its monitor color > profile) to be impacting this benchmark. Because the tool is stand-alone, the machines settings are ignored, a Whacked color profile is always used. Does that make sense?
Leon@ I patched this in and found a few issues compiling and running on Mac and Win. https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:30: #error Fix: WIN32_LEAN_AND_MEAN disables timeBeginPeriod/TimeEndPeriod. We will probably need to remove this #if #else clause. At least I had to remove it to get this CL compiled on Windows. https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:310: char* name = argv[0]; blink::Platform increasingly depends on src/base in its initialization. To avoid a crash at start-up, I had to add prefix the line with ... base::CommandLine::Init(argc, argv); Could you try that? https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:358: class WebPlatform : public TestingPlatformSupport { Could you try changing this to class WebPlatform : public blink::Platform { to remove the dependency on TestingPlatformSupport, and see if you can compile and run the tool? https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:369: Maybe a DeferredImageDecoder::setEnabled(false); is needed here to disable deferred decoding? An alternative option might be to just preroll the SkImage of a frame? https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/web.gyp (right): https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/web.gyp:208: '../config.gyp:config', To compile on Mac OSX with a component build, I had to add an explicit dependency on src/base in this group. '<(DEPTH)/base/base.gyp:base', Not sure why you did not need to. https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/web.gyp:221: '../platform/testing/TestingPlatformSupport.h', As mentioned in review of ImageDecodeBench.cpp, we might be able to remove these if we can define / use class WebPlatform as class WebPlatform : public blink:Platform Could you try that?
On 2016/04/26 14:09:59, noel gordon wrote: > Leon@ I patched this in and found a few issues compiling and running on Mac and > Win. > > https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right): > > https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/ImageDecodeBench.cpp:30: #error Fix: > WIN32_LEAN_AND_MEAN disables timeBeginPeriod/TimeEndPeriod. > We will probably need to remove this #if #else clause. At least I had to remove > it to get this CL compiled on Windows. Re-checked this. Include <mmsystem.h> is all that's needed, and we already include it. So yes, delete the #if #else clause. > https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/web.gyp:208: '../config.gyp:config', > To compile on Mac OSX with a component build, I had to add an explicit > dependency on src/base in this group. > > '<(DEPTH)/base/base.gyp:base', > > Not sure why you did not need to. Re-checked this too: not needed. Once I defined class WebPlatform : public blink:Platform { } the tool compiles on win / mac fine without my suggested gyp change. HTH.
Description was changed from ========== Make ImageDecodeBench build once again Update the gyp file to include TestingPlatformSupport. Use size_t instead of position for SharedBuffer::getSomeData. Add missing includes to TestingPlatformSupport. Comment out missing function from ImageDecoder. BUG=601198 ========== to ========== Make ImageDecodeBench build once again Use size_t instead of position for SharedBuffer::getSomeData. Inherit from blink::Platform instead of TestingPlatformSupport. Remove missing function from ImageDecoder. Instead, decode once before the loop to do any initialization. Call base::CommandLine::Init Remove unnecessary windows error. BUG=601198 ==========
https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:30: #error Fix: WIN32_LEAN_AND_MEAN disables timeBeginPeriod/TimeEndPeriod. On 2016/04/26 14:09:59, noel gordon wrote: > We will probably need to remove this #if #else clause. At least I had to remove > it to get this CL compiled on Windows. I'm guessing you mean the #if #error clause, and not the larger #if WIN32 #else clause? I do not have a windows machine, so I cannot test (and I don't think we currently have a trybot that runs this). https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:358: class WebPlatform : public TestingPlatformSupport { On 2016/04/26 14:09:59, noel gordon wrote: > Could you try changing this to > > class WebPlatform : public blink::Platform { > > to remove the dependency on TestingPlatformSupport, and see if you can compile > and run the tool? Yes, that works. It seems a little weird to me that we're switching from a test version to the real thing though. This also requires calling base::CommandLine::Init, which in turn requires including "base/command_line.h". This all works, but the presubmit complains that I've violated the checksdeps rules (should not include base from third_party). https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:369: On 2016/04/26 14:09:59, noel gordon wrote: > Maybe a DeferredImageDecoder::setEnabled(false); is needed here to disable > deferred decoding? An alternative option might be to just preroll the SkImage > of a frame? I don't see how this is necessary. That only affects using DeferredImageDecoder, which this program does not use. It uses ImageDecoder directly, which will never accidentally use deferred. (That line used to be in here, but you removed it in https://chromium.googlesource.com/chromium/src/+/d6e97a9d13f0adc2a31432136f83...) https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:371: // FIXME: This method has been removed, but it is important. On 2016/04/26 13:56:43, noel gordon wrote: > On 2016/04/21 07:30:42, ccameron wrote: > > > https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right): > > > > > https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/web/ImageDecodeBench.cpp:371: // FIXME: This method > > has been removed, but it is important. > > Could you give a bit more detail about what is "important"? > > For Blink>Image, we typically test a image changes using an image corpus to > assess perf. leon@, do you have an example where you had to do that? As I understand it, the question is not whether the tool is important, but whether this call is important. noel@, you know more about this method than I do, but based on the comment, it looks like it did some initialization outside of the loop, so we don't factor it into the time. > > > I suspect this impacts the benchmark because the first call to > > qcmsOutputDeviceProfile() will do a blocking IPC to the browser process, which > > is an artifact that you don't want to appear in your timing data. > > The tool is a stand-alone executable: there's no browser process, and no IPC. > Instead, the tool provides the Platform interface which is used to provide the > color profile (see class WebPlatform). > > Note the color profile is initialized outside of the iteration loop, so that > cost is not included in the benchmark results. > > First, below, in the > for (size_t i = 0; i < iterations; ++i) { > you could change that to go through iterations+1 and just toss out the i=0 > result. This is probably something you'll want to do anyway. Done. > > Second, we had some discussion earlier about adding a more predictable and > > controllable mechanism for communicating device color profiles. We could > > continue in that direction, and specify the output color profile to a specific > > color profile there: https://codereview.chromium.org/1796293003/ > > > > We should probably do both of these. The first to lower variance. > > As mention above, there is no variance. > > > The second > > because we probably don't want the machine's system settings (its monitor > color > > profile) to be impacting this benchmark. > > Because the tool is stand-alone, the machines settings are ignored, a Whacked > color profile is always used. Does that make sense? https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/web.gyp (right): https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/web.gyp:208: '../config.gyp:config', > > To compile on Mac OSX with a component build, I had to add an explicit > > dependency on src/base in this group. > > > > '<(DEPTH)/base/base.gyp:base', > > > > Not sure why you did not need to. > > Re-checked this too: not needed. Once I defined > > class WebPlatform : public blink:Platform { } > > the tool compiles on win / mac fine without my suggested gyp change. > > HTH. Per comment over email, left this out. https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/web.gyp:221: '../platform/testing/TestingPlatformSupport.h', On 2016/04/26 14:09:59, noel gordon wrote: > As mentioned in review of ImageDecodeBench.cpp, we might be able to remove these > if we can define / use class WebPlatform as > > class WebPlatform : public blink:Platform > > Could you try that? Done.
https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:30: #error Fix: WIN32_LEAN_AND_MEAN disables timeBeginPeriod/TimeEndPeriod. On 2016/04/29 13:44:30, scroggo_chromium wrote: > On 2016/04/26 14:09:59, noel gordon wrote: > > We will probably need to remove this #if #else clause. At least I had to > remove > > it to get this CL compiled on Windows. > > I'm guessing you mean the #if #error clause, and not the larger #if WIN32 #else > clause? Correct, just these 3 lines need to be removed. - #if defined(WIN32_LEAN_AND_MEAN) - #error Fix: WIN32_LEAN_AND_MEAN disables timeBeginPeriod/TimeEndPeriod. - #endif https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:358: class WebPlatform : public TestingPlatformSupport { On 2016/04/29 13:44:30, scroggo_chromium wrote: > On 2016/04/26 14:09:59, noel gordon wrote: > > Could you try changing this to > > > > class WebPlatform : public blink::Platform { > > > > to remove the dependency on TestingPlatformSupport, and see if you can compile > > and run the tool? > > Yes, that works. It seems a little weird to me that we're switching from a test > version to the real thing though. TestingWebPlatform provides very few overrides, and none we use, for blink::Platform. Also, since TestingWebPlatform : public blink::Platform, we did initialize the real thing via the later code Platform::initialize(new WebPlatform()); so maybe ok? > This also requires calling base::CommandLine::Init, which in turn requires > including "base/command_line.h". Yeap. > This all works, but the presubmit complains > that I've violated the checksdeps rules (should not include base from > third_party). IC, then that's the remaining thing to fix. https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:369: On 2016/04/29 13:44:30, scroggo_chromium wrote: > On 2016/04/26 14:09:59, noel gordon wrote: > > Maybe a DeferredImageDecoder::setEnabled(false); is needed here to disable > > deferred decoding? An alternative option might be to just preroll the SkImage > > of a frame? > > I don't see how this is necessary. That only affects using DeferredImageDecoder, > which this program does not use. It uses ImageDecoder directly, which will never > accidentally use deferred. I recall in testing the older code last week, that the color profile initializer in ImageFrame was being called twice, but called only once if I called DeferredImageDecoder::setEnabled(false) during setup. Maybe doesn't matter anymore now you have done an initial decode and thrown the result away. https://codereview.chromium.org/1879133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ImageDecodeBench.cpp:371: // FIXME: This method has been removed, but it is important. On 2016/04/29 13:44:30, scroggo_chromium wrote: > On 2016/04/26 13:56:43, noel gordon wrote: > As I understand it, the question is not whether the tool is important, but > whether this call is important. noel@, you know more about this method than I > do, but based on the comment, it looks like it did some initialization outside > of the loop, so we don't factor it into the time. Yeap, that's right, to remove profile initialization effects from the results. Doing an initial decode outside the loop, as you have done now, should remove any initialization effects too, I expect.
https://codereview.chromium.org/1879133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ImageDecodeBench.cpp (left): https://codereview.chromium.org/1879133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ImageDecodeBench.cpp:31: #endif Yeap, that looks good. https://codereview.chromium.org/1879133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ImageDecodeBench.cpp:288: length = std::min(static_cast<size_t>(length), packetSize); nit: static_cast<size_t>(length) -> length https://codereview.chromium.org/1879133002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/1879133002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ImageDecodeBench.cpp:307: base::CommandLine::Init(argc, argv); As you noted, this is the thing we need to fix (due to the presubmit warning you mentioned).
On 2016/05/02 12:34:55, noel gordon wrote: > https://codereview.chromium.org/1879133002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/ImageDecodeBench.cpp:307: > base::CommandLine::Init(argc, argv); > As you noted, this is the thing we need to fix (due to the presubmit warning you > mentioned). Searching around, platform/RunAllTests.cpp ... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Seems like one option might be to move ImageDecodeBench into one of the platform gyp (gn) files, where call src/base is permitted. blink_platform.gyp and blink_platform_tests.gyp are possible candidates.
On 2016/05/04 03:46:21, noel gordon wrote: > Seems like one option might be to move ImageDecodeBench into one of the platform > gyp (gn) files, where call src/base is permitted. blink_platform.gyp and > blink_platform_tests.gyp are possible candidates. on try blink_platform_tests.gyp: https://codereview.chromium.org/1954673002 and that compiles for me on win, no crash at start-up. Will that work for you?
On 2016/05/05 09:44:07, noel gordon wrote: > On 2016/05/04 03:46:21, noel gordon wrote: > > > Seems like one option might be to move ImageDecodeBench into one of the > platform > > gyp (gn) files, where call src/base is permitted. blink_platform.gyp and > > blink_platform_tests.gyp are possible candidates. > > on try blink_platform_tests.gyp: > > https://codereview.chromium.org/1954673002 > > and that compiles for me on win, no crash at start-up. Will that work for you? Yep. That works for me. Thanks! Closing this one. |
