Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1242263011:
Record UMA stats for Blink decoded image types (Closed)
|
Created:
5 years, 5 months ago by urvang Modified:
5 years, 4 months ago Reviewers:
Noel Gordon CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRecord UMA stats for Blink decoded image types
Note: We avoid counting trivial 1x1 images.
These can be seen locally at chrome://histograms/Blink
Histogram was added here: http://crrev.com/1249273004
BUG=513523
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200090
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Moved logic to BitmapImage to avoid double counting for ICO/CUR #
Total comments: 8
Patch Set 3 : hasVisibleImageSize() helper #Patch Set 4 : move enum to source file #Messages
Total messages: 27 (9 generated)
urvang@chromium.org changed reviewers: + noel@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
@Noel: Pls review this now that the other patch has landed.
https://codereview.chromium.org/1242263011/diff/100001/Source/platform/image-... File Source/platform/image-decoders/ico/ICOImageDecoder.h (right): https://codereview.chromium.org/1242263011/diff/100001/Source/platform/image-... Source/platform/image-decoders/ico/ICOImageDecoder.h:85: reportStats(ImageICO); Considering that each ICO decoder has one or more child decoders, either PNG or BMP, and those child decoders will reportStats() as well, this approach over-counts. Would it be possible do the counting in BitmapImage, or maybe ImageSource, to avoid over-counting? BitmapImage has an ImageSource which has an m_decoder, and thus an m_decoder->filenameExtension(). Perhaps that could be used to centralize where reportStats() is done, and not over-count?
https://codereview.chromium.org/1242263011/diff/100001/Source/platform/image-... File Source/platform/image-decoders/ico/ICOImageDecoder.h (right): https://codereview.chromium.org/1242263011/diff/100001/Source/platform/image-... Source/platform/image-decoders/ico/ICOImageDecoder.h:85: reportStats(ImageICO); On 2015/07/30 18:11:47, noel gordon wrote: > Considering that each ICO decoder has one or more child decoders, either PNG or > BMP, and those child decoders will reportStats() as well, this approach > over-counts. Ah good point, thanks! > > Would it be possible do the counting in BitmapImage, or maybe ImageSource, to > avoid over-counting? > > BitmapImage has an ImageSource which has an m_decoder, and thus an > m_decoder->filenameExtension(). Perhaps that could be used to centralize where > reportStats() is done, and not over-count? But would be have the image width and height at that point for sure? They are needed to ignore the 1x1 images. My initial idea was to reportStats() in ImageDecoder::Create() itself, but we don't have width/height at that point: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/07/30 18:52:37, urvang wrote: > https://codereview.chromium.org/1242263011/diff/100001/Source/platform/image-... > File Source/platform/image-decoders/ico/ICOImageDecoder.h (right): > > https://codereview.chromium.org/1242263011/diff/100001/Source/platform/image-... > Source/platform/image-decoders/ico/ICOImageDecoder.h:85: reportStats(ImageICO); > On 2015/07/30 18:11:47, noel gordon wrote: > > Considering that each ICO decoder has one or more child decoders, either PNG > or > > BMP, and those child decoders will reportStats() as well, this approach > > over-counts. > > Ah good point, thanks! > > > > > Would it be possible do the counting in BitmapImage, or maybe ImageSource, to > > avoid over-counting? > > > > BitmapImage has an ImageSource which has an m_decoder, and thus an > > m_decoder->filenameExtension(). Perhaps that could be used to centralize > where > > reportStats() is done, and not over-count? > > But would be have the image width and height at that point for sure? They are > needed to ignore the 1x1 images. Blink creates a BitmapImage when it needs an image. When Blink wants to use / draw the image, it must know the image size for layout. Blink calls BitmapImage->isSizeAvailable(). That calls down thru ImageSource toward its m_decoder, and invokes m_decoder::ImageDecoder::isSizeAvailable(). https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Therein we see decodeSize() called if we don't know the size. Each image decoder overrides decodeSize() (see your current patch) and calls decode(true) or similar to pump enough data into decoder to decode the image size (aka decode image headers, aka a sizeOnly decode). So when a BitmapImage / ImageSource report true to isSizeAvailable(), you 1) have the image size and 2) know that Blink will use / draw the image. If you can detect that state change, isSizeAvailable() false -> true, that'd be the time to report the stat.
TL:DR; you change should probably happen around [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/07/30 22:28:09, noel gordon wrote: > TL:DR; you change should probably happen around [1] > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Indeed, this is a good place for the reporting. Moved the logic there, PTAL.
https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... File Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... Source/platform/graphics/BitmapImage.cpp:343: if (m_sizeAvailable && (size().width() > 1 || size().height() > 1)) { Yes better. Let's define a static inline hasVisibleImageSize(size()) helper to document the visible size thing. In that helper, should it be the conjunction, viz., size().width() > N _&&_ size().height() > N ? N=1 is obvious I suppose, but what value of N should we use to declare an image visible? N=6,8,10,...? Consider the smallest box that could be visible _and_ meaningful to a user. https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... File Source/platform/graphics/BitmapImage.h (right): https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... Source/platform/graphics/BitmapImage.h:45: // These values are histogrammed over time; do not change their ordinal We should move this into its own file, similar to platform/image-decoder/ImageAnimation.h and platform/graphics/ImageAnimimationPolicy.h Please add FIXME: to do that in a follow-up patch. https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... Source/platform/graphics/BitmapImage.h:49: // tools/metrics/histograms/histograms.xml tools/metrics/histograms/histograms.xml -> src/tools/metrics/histograms/histograms.xml https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... Source/platform/graphics/BitmapImage.h:58: DecodedImageTypeMax = ImageBMP // Must equal the last "real" image type above. nit: abbv DecodedImageTypeMax. Rename it to LastDecodedImageType, drop the comment here, amend the main comment where it refers to DecodedImageTypeMax.
Thanks for the comments! PTAL. https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... File Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... Source/platform/graphics/BitmapImage.cpp:343: if (m_sizeAvailable && (size().width() > 1 || size().height() > 1)) { On 2015/08/01 02:33:06, noel gordon wrote: > Yes better. Let's define a static inline hasVisibleImageSize(size()) helper to > document the visible size thing. Done. > > In that helper, should it be the conjunction, viz., size().width() > N _&&_ > size().height() > N ? No, we require || operator because 1x100 as well as 100x1 sized images are visible, for example. > > N=1 is obvious I suppose, but what value of N should we use to declare an image > visible? N=6,8,10,...? Consider the smallest box that could be visible _and_ > meaningful to a user. Any image larger than 1x1 would be visible. So, N=1 is appropriate. https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... File Source/platform/graphics/BitmapImage.h (right): https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... Source/platform/graphics/BitmapImage.h:45: // These values are histogrammed over time; do not change their ordinal On 2015/08/01 02:33:06, noel gordon wrote: > We should move this into its own file, similar to > platform/image-decoder/ImageAnimation.h and > platform/graphics/ImageAnimimationPolicy.h > > Please add FIXME: to do that in a follow-up patch. Can you explain why this is necessary? - Other enums in chrome://histograms/Blink don't seem to be in their own files. See: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... for example. - For ImageAnimationPolicy, a separate file made sense as it's used at multiple places. The same doesn't apply to this enum. https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... Source/platform/graphics/BitmapImage.h:49: // tools/metrics/histograms/histograms.xml On 2015/08/01 02:33:06, noel gordon wrote: > tools/metrics/histograms/histograms.xml -> > src/tools/metrics/histograms/histograms.xml In code search, "tools/metrics/histograms/histograms.xml" returns 52 results, while "src/tools/metrics/histograms/histograms.xml" returns 6 results. So former seems to be the norm. But I'm OK either way. Edited. https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... Source/platform/graphics/BitmapImage.h:58: DecodedImageTypeMax = ImageBMP // Must equal the last "real" image type above. On 2015/08/01 02:33:06, noel gordon wrote: > nit: abbv DecodedImageTypeMax. Rename it to LastDecodedImageType, drop the > comment here, amend the main comment where it refers to DecodedImageTypeMax. Done.
> > In that helper, should it be the conjunction, viz., size().width() > N _&&_ > > size().height() > N ? > > No, we require || operator because 1x100 as well as 100x1 sized images are > visible, for example. I suppose we don't care about 0 x 100, since maybe it doesn't happen. A 1x100 is not very meaningful as an image, but I don't really have a better suggestion of how we'd implement "meaningful" at this time.
On 2015/08/04 20:21:17, urvang wrote: >> N=1 is obvious I suppose, but what value of N should we use to declare an >> image visible? N=6,8,10,...? Consider the smallest box that could be >> visible _and_ meaningful to a user. > Any image larger than 1x1 would be visible. So, N=1 is appropriate. Dunno about you, but I'd find a 2x2 hard to see.
On 2015/08/04 20:21:17, urvang wrote: > https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... > Source/platform/graphics/BitmapImage.h:49: // > tools/metrics/histograms/histograms.xml > On 2015/08/01 02:33:06, noel gordon wrote: > > tools/metrics/histograms/histograms.xml -> > > src/tools/metrics/histograms/histograms.xml > > In code search, "tools/metrics/histograms/histograms.xml" returns 52 results, > while "src/tools/metrics/histograms/histograms.xml" returns 6 results. So former > seems to be the norm. > But I'm OK either way. Understood. The "src" leader was to indicate that the code referred to was in chromium, not blink.
On 2015/08/04 20:21:17, urvang wrote: > Thanks for the comments! PTAL. > > https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... > File Source/platform/graphics/BitmapImage.cpp (right): > > https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... > Source/platform/graphics/BitmapImage.cpp:343: if (m_sizeAvailable && > (size().width() > 1 || size().height() > 1)) { > On 2015/08/01 02:33:06, noel gordon wrote: > > Yes better. Let's define a static inline hasVisibleImageSize(size()) helper > to > > document the visible size thing. > > Done. > > > > > In that helper, should it be the conjunction, viz., size().width() > N _&&_ > > size().height() > N ? > > No, we require || operator because 1x100 as well as 100x1 sized images are > visible, for example. > > > > > N=1 is obvious I suppose, but what value of N should we use to declare an > image > > visible? N=6,8,10,...? Consider the smallest box that could be visible _and_ > > meaningful to a user. > > Any image larger than 1x1 would be visible. So, N=1 is appropriate. > > https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... > File Source/platform/graphics/BitmapImage.h (right): > > https://codereview.chromium.org/1242263011/diff/120001/Source/platform/graphi... > Source/platform/graphics/BitmapImage.h:45: // These values are histogrammed over > time; do not change their ordinal > On 2015/08/01 02:33:06, noel gordon wrote: > > We should move this into its own file, similar to > > platform/image-decoder/ImageAnimation.h and > > platform/graphics/ImageAnimimationPolicy.h > > > > Please add FIXME: to do that in a follow-up patch. > > Can you explain why this is necessary? I guess my point was these values are an internal implementation detail of BitmapImage, so I'd like to get them out of the the .h file. > - Other enums in chrome://histograms/Blink don't seem to be in their own files. > See: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > for example. That's a nice example. > - For ImageAnimationPolicy, a separate file made sense as it's used at multiple > places. The same doesn't apply to this enum. Ack. How about we do it like media does it, viz., define the enum in the .cpp near where used?
On 2015/08/05 02:15:27, noel gordon wrote: > > I guess my point was these values are an internal implementation detail of > BitmapImage, so I'd like to get them out of the the .h file. > > > - Other enums in chrome://histograms/Blink don't seem to be in their own > files. > > See: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > for example. > > That's a nice example. > > > - For ImageAnimationPolicy, a separate file made sense as it's used at > multiple > > places. The same doesn't apply to this enum. > > Ack. How about we do it like media does it, viz., define the enum in the .cpp > near where used? Good idea! Done.
On 2015/08/05 20:17:15, urvang wrote: > Good idea! Done. LGTM.
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/1242263011/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1242263011/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/65210)
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1242263011/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1242263011/160001
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200090 |
