Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18107003:
Add a command line driven image decoding tool (Closed)
|
Created:
7 years, 5 months ago by Noel Gordon Modified:
6 years, 9 months ago CC:
blink-reviews, dglazkov+blink, eae+blinkwatch Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd a command line driven image decoding tool
Usage: image_decode_bench my-image-file.png [iterations] to decode the
given image some number of times (iterations: default is 1000). Output
the total and average image decode times in seconds to stdout.
BUG=258324
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 3
Messages
Total messages: 20 (0 generated)
asked the gardener to mail troopers about the "webkit_python_tests webkit_python_tests failed" foo.
Not sure about the time function but James will probably have a better idea. https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... File Source/WebKit/chromium/src/ImageDecodeBench.cpp (right): https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:31: // % ninja -C out/Release image_decode_bench There's no need to add comments about building it. https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:49: using namespace std; You're not using <iostream> you can get rid of this. https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:199: unsigned char* buffer = new unsigned char[fileSize]; delete [] buffer after use. https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:239: class DummyWebKitPlatformSupport : public WebKit::Platform { Class definition like this looks kind of weird. I don't have strong object to it though.
https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... File Source/WebKit/chromium/src/ImageDecodeBench.cpp (right): https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:56: // http://trac.webkit.org/browser/trunk/Source/WTF/wtf/CurrentTime.cpp?rev=152438 nack, just use WTF::monotonicallyIncreasingTime(). It should be fine.
https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... File Source/WebKit/chromium/src/ImageDecodeBench.cpp (right): https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:31: // % ninja -C out/Release image_decode_bench removed - there is no build item for image_decode_bench in VS2010 with this change, so I left this here so the intrepid would at least know what to do. https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:49: using namespace std; On 2013/07/10 18:51:07, Alpha wrote: > You're not using <iostream> you can get rid of this. Done. https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:56: // http://trac.webkit.org/browser/trunk/Source/WTF/wtf/CurrentTime.cpp?rev=152438 On 2013/07/10 19:20:33, jamesr wrote: > nack, just use WTF::monotonicallyIncreasingTime(). It should be fine. Ah if only we had it. There's no real platform support here since we are using a dummy platform to initialize webkit. So there is no currentTime or monotonicallyIncreasingTime either; we're rolling our own. Adjusted the comment to say that for now. Not sure how to access a complete platform implementation to plug in, or if it'd be worth it. https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:199: unsigned char* buffer = new unsigned char[fileSize]; used OwnArrayPtr. https://codereview.chromium.org/18107003/diff/1/Source/WebKit/chromium/src/Im... Source/WebKit/chromium/src/ImageDecodeBench.cpp:239: class DummyWebKitPlatformSupport : public WebKit::Platform { Me either, and the style presubmit is mute about it too.
LGTM if James is happy with the time functions.
Hmm, it's unfortunate to add this timing stuff in blink when it looks very redundant with chromium code. Why can't you write this tool on the chromium side of the repo? It looks like the WebKit::WebImageDecoder interface provides access to the decoder functionality you need through the WebKit API, no?
On 2013/07/11 19:14:30, jamesr wrote: > Hmm, it's unfortunate to add this timing stuff in blink when it looks very > redundant with chromium code. Why can't you write this tool on the chromium > side of the repo? It looks like the WebKit::WebImageDecoder interface provides > access to the decoder functionality you need through the WebKit API, no? That's a very good suggestion I didn't think of. Moving this to Chromium code allow us to use profiler as well.
On 2013/07/11 19:19:39, Alpha wrote: > That's a very good suggestion I didn't think of. Moving this to Chromium code > allow us to use profiler as well. What profiler?
On 2013/07/11 19:14:30, jamesr wrote: > Hmm, it's unfortunate to add this timing stuff in blink when it looks very > redundant with chromium code. Won't it be redundant when src/Blink happens? Then we could just call into base or something. > Why can't you write this tool on the chromium > side of the repo? It looks like the WebKit::WebImageDecoder interface provides > access to the decoder functionality you need through the WebKit API, no? Why do that? And WebKit::WebImageDecoder doesn't handle JPEG/PNG/GIF ...
On Jul 15, 2013 1:12 AM, <noel@chromium.org> wrote: > > On 2013/07/11 19:14:30, jamesr wrote: >> >> Hmm, it's unfortunate to add this timing stuff in blink when it looks very >> redundant with chromium code. > > > Won't it be redundant when src/Blink happens? Then we could just call into base > or something. The repo merge is at least many months away. > > >> Why can't you write this tool on the chromium >> side of the repo? It looks like the WebKit::WebImageDecoder interface > > provides >> >> access to the decoder functionality you need through the WebKit API, no? > > > Why do that? And WebKit::WebImageDecoder doesn't handle JPEG/PNG/GIF ... > Sure it does. Did you even try it? > > https://codereview.chromium.org/18107003/
On 2013/07/15 15:54:08, jamesr wrote: > > Why do that? And WebKit::WebImageDecoder doesn't handle JPEG/PNG/GIF ... > Sure it does. Did you even try it? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... WebImageDecoder::init(Type type) is called form the constructor, seems it only creates BMP or ICO. What am I missing?
WebImage::fromData() should work for JPEG, PNG, GIF, ICO, WebP and BMP and is used in this way in several places in chromium. WebImageDecoder appears to only be called by tests, I don't really know why it exists.
On 2013/07/16 01:10:52, jamesr wrote: > WebImage::fromData() should work for JPEG, PNG, GIF, ICO, WebP and BMP and is > used in this way in several places in chromium. .. to decode a single-frame of an image only -- won't work for GIF decode benchmarking. > WebImageDecoder appears to only be called by tests, I don't really know why it exists. For testing ICO and BMP images it seems.
On 2013/07/16 06:14:48, Noel Gordon (Google) wrote: > For testing ICO and BMP images it seems. https://code.google.com/p/chromium/issues/detail?id=184276
https://codereview.chromium.org/18107003/diff/9001/Source/web/ImageDecodeBenc... File Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/18107003/diff/9001/Source/web/ImageDecodeBenc... Source/web/ImageDecodeBench.cpp:218: int main(int argc, char* argv[]) This file should probably be in Source/testing or in Tools somewhere. It's a client of the Blink API rather than part of the implementation.
https://codereview.chromium.org/18107003/diff/9001/Source/WebKit/chromium/Web... File Source/WebKit/chromium/WebKit.gyp (right): https://codereview.chromium.org/18107003/diff/9001/Source/WebKit/chromium/Web... Source/WebKit/chromium/WebKit.gyp:533: 'WEBKIT_IMPLEMENTATION=1', This isn't correct. We shouldn't have an executable be linked as if it were inside the DLL. It should be outside the DLL.
The more I think about this CL, the more I think this target should be in Chromium and consume Blink via its API.
https://codereview.chromium.org/18107003/diff/9001/Source/web/ImageDecodeBenc... File Source/web/ImageDecodeBench.cpp (right): https://codereview.chromium.org/18107003/diff/9001/Source/web/ImageDecodeBenc... Source/web/ImageDecodeBench.cpp:210: for (int i = 0; i < frameCount; ++i) { If we always start decoding from the first frame, the efficiency of seeking wouldn't be tested. How about starting from the middle frame and then decoding all frames from there (in the order: middle, middle + 1, ..., last, 1, 2, ..., middle - 1)?
To be more explicit, not lgtm. I don't think Blink should contain this target, especially not inside the API. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
