Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1249273004:
Add a histogram for Blink decoded image type (Closed)
DescriptionAdd a histogram for Blink decoded image type
Add a histogram to record decoded images in Blink by type.
Notes:
(1) Both ICO and CUR images are categorized as ICO type in
the histogram since both use the Blink::ICOImageDecoder,
which does not discriminate by type.
(2) We only count visible images. In other words, 1x1 images
aren't counted.
Recording of stats implemented here: http://crrev.com/1242263011
BUG=513523
Committed: https://crrev.com/2d3227d9478a93d92ad215914968bc6b6edc80fc
Cr-Commit-Position: refs/heads/master@{#341069}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Naming tweaks #Patch Set 3 : #
Total comments: 2
Patch Set 4 : nit #
Total comments: 2
Messages
Total messages: 28 (6 generated)
urvang@chromium.org changed reviewers: + jar@chromium.org, noel@chromium.org
On 2015/07/28 18:47:14, urvang wrote: > mailto:urvang@chromium.org changed reviewers: > + mailto:jar@chromium.org, mailto:noel@chromium.org Need review from Noel and OWNER's LGTM from jar@. Thanks!
https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2605: + <summary>Image codec inferred during decode.</summary> Since this is for decoded images, could we call it blink.DecodedImageType? (anything other but codec). https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59403: + <int value="0" label="kUnknownImage"/> kImageUnknown? https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59408: + <int value="5" label="kImageICO"/> Can blink discriminate between ICO and CUR types? Their file types are decoded with the same image decoder. Should we just lump them together as kImageICO?
https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2605: + <summary>Image codec inferred during decode.</summary> On 2015/07/28 22:49:24, noel gordon wrote: > Since this is for decoded images, could we call it blink.DecodedImageType? > (anything other but codec). Done. https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59403: + <int value="0" label="kUnknownImage"/> On 2015/07/28 22:49:24, noel gordon wrote: > kImageUnknown? Done. https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59408: + <int value="5" label="kImageICO"/> On 2015/07/28 22:49:24, noel gordon wrote: > Can blink discriminate between ICO and CUR types? Their file types are decoded > with the same image decoder. Should we just lump them together as kImageICO? Ah my bad, I had already combined them into kImageICOorCUR in the Blink side patch. Did the same here now. Thanks!
https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2605: + <summary>Image codec inferred during decode.</summary> On 2015/07/28 23:24:34, urvang wrote: > On 2015/07/28 22:49:24, noel gordon wrote: > > Since this is for decoded images, could we call it blink.DecodedImageType? > > (anything other but codec). > > Done. Ok good. You might need to re-title this CL, and I see you tried, thank you. Maybe "Add a histogram for Blink decoded image type". https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59408: + <int value="5" label="kImageICO"/> On 2015/07/28 23:24:34, urvang wrote: > On 2015/07/28 22:49:24, noel gordon wrote: > > Can blink discriminate between ICO and CUR types? Their file types are > decoded > > with the same image decoder. Should we just lump them together as kImageICO? > > Ah my bad, I had already combined them into kImageICOorCUR in the Blink side > patch. > Did the same here now. Thanks! I guess my questions were unclear, since I was asking them from two different POV... That fact that Blink can't discriminate b/w ICO/CUR is its problem, its limitation. I think it's unwise to reflect that limitation in this histogram enum. Here's what I'd do. Define kImageICO only here. If Blink decides in future that it is important to discriminate, then we can add a kImageCUR entry here at that time. I'd describe why things are this way _briefly_ in the change description. On that point, the change description is a little empty for my tastes, but I suppose we can fill it in with ~2 sentences now we know more details about this change.
With ^^^ those changes, LGTM.
https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:2605: + <summary>Image codec inferred during decode.</summary> On 2015/07/29 00:04:57, noel gordon wrote: > On 2015/07/28 23:24:34, urvang wrote: > > On 2015/07/28 22:49:24, noel gordon wrote: > > > Since this is for decoded images, could we call it blink.DecodedImageType? > > > (anything other but codec). > > > > Done. > > Ok good. You might need to re-title this CL, and I see you tried, thank you. > Maybe "Add a histogram for Blink decoded image type". Done. https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59408: + <int value="5" label="kImageICO"/> On 2015/07/29 00:04:57, noel gordon wrote: > On 2015/07/28 23:24:34, urvang wrote: > > On 2015/07/28 22:49:24, noel gordon wrote: > > > Can blink discriminate between ICO and CUR types? Their file types are > > decoded > > > with the same image decoder. Should we just lump them together as > kImageICO? > > > > Ah my bad, I had already combined them into kImageICOorCUR in the Blink side > > patch. > > Did the same here now. Thanks! > > I guess my questions were unclear, since I was asking them from two different > POV... > > That fact that Blink can't discriminate b/w ICO/CUR is its problem, its > limitation. I think it's unwise to reflect that limitation in this histogram > enum. To be precise: Blink *can* descriminate between ICO and CUR by signature: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But, as we need the width/height of the image while reporting the stats, we add to the histogram inside the decoder. And because ICO and CUR have a common decoder, we can't discriminate between the two at that point. Given this, I'm OK with naming it either "ICOorCUR" or just "ICO". What do you prefer?
On 2015/07/29 00:23:20, urvang wrote: > https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:2605: + <summary>Image codec inferred > during decode.</summary> > On 2015/07/29 00:04:57, noel gordon wrote: > > On 2015/07/28 23:24:34, urvang wrote: > > > On 2015/07/28 22:49:24, noel gordon wrote: > > > > Since this is for decoded images, could we call it blink.DecodedImageType? > > > > (anything other but codec). > > > > > > Done. > > > > Ok good. You might need to re-title this CL, and I see you tried, thank you. > > Maybe "Add a histogram for Blink decoded image type". > > Done. > > https://codereview.chromium.org/1249273004/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:59408: + <int value="5" > label="kImageICO"/> > On 2015/07/29 00:04:57, noel gordon wrote: > > On 2015/07/28 23:24:34, urvang wrote: > > > On 2015/07/28 22:49:24, noel gordon wrote: > > > > Can blink discriminate between ICO and CUR types? Their file types are > > > decoded > > > > with the same image decoder. Should we just lump them together as > > kImageICO? > > > > > > Ah my bad, I had already combined them into kImageICOorCUR in the Blink side > > > patch. > > > Did the same here now. Thanks! > > > > I guess my questions were unclear, since I was asking them from two different > > POV... > > > > That fact that Blink can't discriminate b/w ICO/CUR is its problem, its > > limitation. I think it's unwise to reflect that limitation in this histogram > > enum. > > > To be precise: Blink *can* descriminate between ICO and CUR by signature: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > But, as we need the width/height of the image while reporting the stats, we add > to the histogram inside the decoder. And because ICO and CUR have a common > decoder, we can't discriminate between the two at that point. > > Given this, I'm OK with naming it either "ICOorCUR" or just "ICO". What do you > prefer? Let's just call is kImageICO for now in this enum, and have Blink lump everything decoded by ICOImageDecoder into that bin.
> Let's just call is kImageICO for now in this enum, and have Blink lump > everything decoded by ICOImageDecoder into that bin. Done.
On 2015/07/29 00:30:59, noel gordon wrote: > > To be precise: Blink *can* descriminate between ICO and CUR by signature: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Agree, and also the ICODecoder internally knows m_filetype ... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... but it's not publicly exposed at this time. That could be the subject of a future bug if discrimination becomes important. I'm pretty sure it is not important today.
On 2015/07/29 00:35:50, urvang wrote: > > Let's just call is kImageICO for now in this enum, and have Blink lump > > everything decoded by ICOImageDecoder into that bin. > > Done. Right, now fix you change description (re-read #5) and away you go. LGTM.
urvang@chromium.org changed reviewers: + asvitkine@chromium.org - jar@chromium.org
@asvitkine: can you pls give OWNER's LGTM?
https://codereview.chromium.org/1249273004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1249273004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2604: +<histogram name="blink.DecodedImageType" enum="DecodedImageType"> Nit: Capitalize Blink to be consistent with the other histograms. Can you link to the CL that implements this?
https://codereview.chromium.org/1249273004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1249273004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2604: +<histogram name="blink.DecodedImageType" enum="DecodedImageType"> On 2015/07/29 14:11:51, Alexei Svitkine wrote: > Nit: Capitalize Blink to be consistent with the other histograms. Done > Can you link to the CL that implements this? Done
lgtm https://codereview.chromium.org/1249273004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1249273004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2605: + <summary>Image codec inferred during decode.</summary> Nit: Mention that this ignores trivial 1x1 images, as you do in your corresponding CL.
On 2015/07/29 15:54:08, Alexei Svitkine wrote: > Nit: Mention that this ignores trivial 1x1 images, as you do in your > corresponding CL. Mind, we might change that definition to be "visible" images, and that might be larger than 1x1, TBD.
https://codereview.chromium.org/1249273004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1249273004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2605: + <summary>Image codec inferred during decode.</summary> On 2015/07/29 15:54:08, Alexei Svitkine wrote: > Nit: Mention that this ignores trivial 1x1 images, as you do in your > corresponding CL. Done.
On 2015/07/29 15:57:25, noel gordon wrote: > On 2015/07/29 15:54:08, Alexei Svitkine wrote: > > > Nit: Mention that this ignores trivial 1x1 images, as you do in your > > corresponding CL. > > Mind, we might change that definition to be "visible" images, and that might be > larger than 1x1, TBD. Ack.
The CQ bit was checked by urvang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from noel@chromium.org Link to the patchset: https://codereview.chromium.org/1249273004/#ps60001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1249273004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1249273004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by urvang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1249273004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1249273004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2d3227d9478a93d92ad215914968bc6b6edc80fc Cr-Commit-Position: refs/heads/master@{#341069} |
