Make ImageDecodeBench build once again

third_party/WebKit/Source/web/ImageDecodeBench.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Provides a minimal wrapping of the Blink image decoders. Used to perform
 // a non-threaded, memory-to-memory image decode using micro second accuracy
 // clocks to measure image decode time. Optionally applies color correction
 // during image decoding on supported platforms (default off). Usage:
 //
 //   % ninja -C /out/Release image_decode_bench &&
 //      ./out/Release/image_decode_bench file [iterations]
 //
 // TODO(noel): Consider adding md5 checksum support to WTF. Use it to compute
 // the decoded image frame md5 and output that value.
 //
 // TODO(noel): Consider integrating this tool in Chrome telemetry for realz,
 // using the image corpii used to assess Blink image decode performance. Refer
 // to http://crbug.com/398235#c103 and http://crbug.com/258324#c5
 
 #include "platform/SharedBuffer.h"
 #include "platform/image-decoders/ImageDecoder.h"
 #include "platform/testing/TestingPlatformSupport.h"
 #include "public/platform/Platform.h"
 #include "public/web/WebKit.h"
 #include "wtf/OwnPtr.h"
 #include "wtf/PassRefPtr.h"
 
 #if defined(_WIN32)
 #if defined(WIN32_LEAN_AND_MEAN)
 #error Fix: WIN32_LEAN_AND_MEAN disables timeBeginPeriod/TimeEndPeriod.

[Noel Gordon, 2016/04/26 14:09:59]

We will probably need to remove this #if #else clause.  At least I had to remove
it to get this CL compiled on Windows.

[scroggo_chromium, 2016/04/29 13:44:30]

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).

[Noel Gordon, 2016/05/02 12:31:09]

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

 #endif
 #include <mmsystem.h>
 #include <sys/stat.h>
 #include <time.h>
 #define stat(x,y) _stat(x,y)
 typedef struct _stat sttype;
 #else
 #include <sys/stat.h>
 #include <sys/time.h>
 typedef struct stat sttype;
@@ skipping 232 matching lines @@
         int frameCount = decoder->frameCount();
         for (int i = 0; i < frameCount; ++i) {
             if (!decoder->frameBufferAtIndex(i))
                 return false;
         }
 
         return !decoder->failed();
     }
 
     RefPtr<SharedBuffer> packetData = SharedBuffer::create();
-    unsigned position = 0;
+    size_t position = 0;
     while (true) {
         const char* packet;
-        unsigned length = data->getSomeData(packet, position);
+        size_t length = data->getSomeData(packet, position);
 
         length = std::min(static_cast<size_t>(length), packetSize);
         packetData->append(packet, length);
         position += length;
 
         bool allDataReceived = position == data->size();
         decoder->setData(packetData.get(), allDataReceived);
 
         int frameCount = decoder->frameCount();
         for (int i = 0; i < frameCount; ++i) {
             if (!decoder->frameBufferAtIndex(i))
                 break;
         }
 
         if (allDataReceived || decoder->failed())
             break;
     }
 
     return !decoder->failed();
 }
 
 int main(int argc, char* argv[])
 {
     char* name = argv[0];

[Noel Gordon, 2016/04/26 14:09:59]

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?

 
     // If the platform supports color correction, allow it to be controlled.
 
     bool applyColorCorrection = false;
 
 #if USE(QCMSLIB)
     if (argc >= 2 && strcmp(argv[1], "--color-correct") == 0)
         applyColorCorrection = (--argc, ++argv, true);
 
     if (argc < 2) {
@@ skipping 27 matching lines @@
         if (*end != '\0') {
             fprintf(stderr, "Third argument should be packet size. Default is "
                 "0, meaning to decode the entire image in one packet. You "
                 "supplied %s\n", argv[3]);
             exit(1);
         }
     }
 
     // Create a web platform without V8.
 
     class WebPlatform : public TestingPlatformSupport {

[Noel Gordon, 2016/04/26 14:09:59]

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?

[scroggo_chromium, 2016/04/29 13:44:30]

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).

[Noel Gordon, 2016/05/02 12:31:09]

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?
Yeap.
IC, then that's the remaining thing to fix.

     public:
         void screenColorProfile(WebVector<char>* profile) override
         {
             getScreenColorProfile(profile); // Returns a whacked color profile.
         }
     };
 
     Platform::initialize(new WebPlatform());
 
     // Set image decoding Platform options.
 

[Noel Gordon, 2016/04/26 14:09:59]

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?

[scroggo_chromium, 2016/04/29 13:44:30]

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...)

[Noel Gordon, 2016/05/02 12:31:09]

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.

 #if USE(QCMSLIB)
-    ImageDecoder::qcmsOutputDeviceProfile(); // Initialize screen colorProfile.
+    // FIXME: This method has been removed, but it is important.

[ccameron, 2016/04/21 07:30:42]

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.

[scroggo_chromium, 2016/04/29 13:44:30]

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.
Done.

[Noel Gordon, 2016/05/02 12:31:09]

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.

+    // ImageDecoder::qcmsOutputDeviceProfile(); // Initialize screen colorProfile.
 #endif
 
     // Read entire file content to data.
 
     RefPtr<SharedBuffer> data = readFile(argv[1]);
     if (!data.get() || !data->size()) {
         fprintf(stderr, "Error reading image data from [%s]\n", argv[1]);
         exit(2);
     }
 
@@ skipping 14 matching lines @@
             exit(3);
         }
     }
 
     // Results to stdout.
 
     double averageTime = totalTime / static_cast<double>(iterations);
     printf("%f %f\n", totalTime, averageTime);
     return 0;
 }