Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2483243003:
Re-enable image_decode_bench target (Closed)
|
Created:
4 years, 1 month ago by Simon Hosie Modified:
4 years, 1 month ago CC:
chromium-reviews, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-enable image_decode_bench target
Committed: https://crrev.com/2f3fe66204307bf90c5aa458207c7d5ed315384f
Cr-Commit-Position: refs/heads/master@{#433351}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Pass colour profile to image decoder. #
Total comments: 8
Patch Set 3 : Re-enable image_decode_bench target #
Total comments: 1
Messages
Total messages: 38 (20 generated)
The CQ bit was checked by simon.hosie@arm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
scroggo@chromium.org changed reviewers: + msarett@chromium.org, scroggo@chromium.org
https://codereview.chromium.org/2483243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (left): https://codereview.chromium.org/2483243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:356: getScreenColorProfile(profile); // Returns a color spin color profile. msarett@, do you know the right way to set this now? I'm guessing we still want to put in some sort of dummy profile.
https://codereview.chromium.org/2483243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (left): https://codereview.chromium.org/2483243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:356: getScreenColorProfile(profile); // Returns a color spin color profile. On 2016/11/09 14:16:59, scroggo_chromium wrote: > msarett@, do you know the right way to set this now? I'm guessing we still want > to put in some sort of dummy profile. I'm not aware that this has changed. The profile is still ICC data at this point. Why is this deleted?
https://codereview.chromium.org/2483243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (left): https://codereview.chromium.org/2483243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:356: getScreenColorProfile(profile); // Returns a color spin color profile. On 2016/11/09 15:01:58, msarett1 wrote: > On 2016/11/09 14:16:59, scroggo_chromium wrote: > > msarett@, do you know the right way to set this now? I'm guessing we still > want > > to put in some sort of dummy profile. > > I'm not aware that this has changed. The profile is still ICC data at this > point. > > Why is this deleted? > The method it overrides was deleted in https://chromium.googlesource.com/chromium/src/+/6fc2fd3b6c99c97632b7fe43bdbf... I wasn't sure how to reconnect things properly.
https://codereview.chromium.org/2483243003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (left): https://codereview.chromium.org/2483243003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:356: getScreenColorProfile(profile); // Returns a color spin color profile. On 2016/11/09 17:29:13, Simon Hosie wrote: > On 2016/11/09 15:01:58, msarett1 wrote: > > On 2016/11/09 14:16:59, scroggo_chromium wrote: > > > msarett@, do you know the right way to set this now? I'm guessing we still > > want > > > to put in some sort of dummy profile. > > > > I'm not aware that this has changed. The profile is still ICC data at this > > point. > > > > Why is this deleted? > > > > The method it overrides was deleted in > https://chromium.googlesource.com/chromium/src/+/6fc2fd3b6c99c97632b7fe43bdbf... > > I wasn't sure how to reconnect things properly. Ahh thanks! It looks to me like now instead we can call static void setTargetColorProfile(const WebVector<char>&); directly on the ImageDecoder.
The CQ bit was checked by cavalcantii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/BUILD.gn:1832: visibility = [ "*" ] The `visibility` value was copied from a unittest target. I don't know what the significance of it is or if it's correct this way. https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:268: decoder->setTargetColorProfile(profile); So far as I could see `profile` doesn't need to live beyond this point. Hope I didn't miss anything. Seems to work.
https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:268: decoder->setTargetColorProfile(profile); On 2016/11/15 01:50:00, Simon Hosie wrote: > So far as I could see `profile` doesn't need to live beyond this point. Hope I > didn't miss anything. Seems to work. Agreed. setTargetColorProfile will store an object that this profile represents [1]. This method is static, though, so you can call ImageDecoder::setTargetColorProfile() once at the beginning of main(). [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:356: // Create a web platform without V8. This comment appears to no longer apply. When this code was first added (here [1]), we called blink::initializeWithoutV8. That method no longer exists, though. It looks like Platform::initialize() is minimal though [2]. [1] https://chromium.googlesource.com/chromium/src/+/b54402e4c1766963e82c35393e9c... [2] https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/Platf... https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:358: class WebPlatform : public blink::Platform {}; Without overriding anything, I don't think you need this class anymore. Can you just do: Platform::initialize(new Platform()); ? Edit - now I see - Platform's constructor is protected, so you cannot new Platform(). Maybe add a comment explaining why it's necessary?
The CQ bit was checked by cavalcantii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:268: decoder->setTargetColorProfile(profile); On 2016/11/15 14:54:42, scroggo_chromium wrote: > On 2016/11/15 01:50:00, Simon Hosie wrote: > > So far as I could see `profile` doesn't need to live beyond this point. Hope > I > > didn't miss anything. Seems to work. > > Agreed. setTargetColorProfile will store an object that this profile represents > [1]. > > This method is static, though, so you can call > ImageDecoder::setTargetColorProfile() once at the beginning of main(). > > [1] > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... Done. https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:356: // Create a web platform without V8. On 2016/11/15 14:54:42, scroggo_chromium wrote: > This comment appears to no longer apply. When this code was first added (here > [1]), we called blink::initializeWithoutV8. That method no longer exists, > though. It looks like Platform::initialize() is minimal though [2]. > > [1] > https://chromium.googlesource.com/chromium/src/+/b54402e4c1766963e82c35393e9c... > [2] > https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/Platf... Done. https://codereview.chromium.org/2483243003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:358: class WebPlatform : public blink::Platform {}; On 2016/11/15 14:54:42, scroggo_chromium wrote: > Without overriding anything, I don't think you need this class anymore. Can you > just do: > > Platform::initialize(new Platform()); > > ? > > Edit - now I see - Platform's constructor is protected, so you cannot new > Platform(). Maybe add a comment explaining why it's necessary? Done.
Changes to ImageDecodeBench.cpp lgtm (although I'm not an OWNER in that directory, so you'll still need an OWNER approval there).
simon.hosie@arm.com changed reviewers: + eae@chromium.org
https://codereview.chromium.org/2483243003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/2483243003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/BUILD.gn:1840: visibility = [ "*" ] I'm not certain about this line. I don't know what visibility does but did wonder if it might imply adding itself as a target for builds that don't want it and might fail with it. I'm also not sure about the rest of this file. I just copied all the lines that looked appropriate from the unit test above based on what I could see in the old .gyp file. It does build and run on Linux and Chromium, though. GYP file for reference: https://chromium.googlesource.com/chromium/src/+/980562f70d53ff313d4f5cd885c7...
LGTM (owner approval for Source/platform)
As it was lgtm-ed, I'm adding it in the CQ. Good job, Simon.
The CQ bit was checked by cavalcantii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by cavalcantii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by cavalcantii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Re-enable image_decode_bench target ========== to ========== Re-enable image_decode_bench target Committed: https://crrev.com/2f3fe66204307bf90c5aa458207c7d5ed315384f Cr-Commit-Position: refs/heads/master@{#433351} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2f3fe66204307bf90c5aa458207c7d5ed315384f Cr-Commit-Position: refs/heads/master@{#433351} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
