Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1413593012:
Record UMA stats for Blink decoded image orientation (Closed)
|
Created:
5 years, 1 month ago by rwlbuis Modified:
5 years, 1 month ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, slimming-paint-reviews_chromium.org, dshwang, jchaffraix+rendering, blink-reviews-paint_chromium.org, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord UMA stats for Blink decoded image orientation
Record UMA stats for Blink decoded bitmap image orientation values.
BUG=158753
Committed: https://crrev.com/7a103b24dadf42f27e914b024977e58ae263a836
Cr-Commit-Position: refs/heads/master@{#360400}
Patch Set 1 #Patch Set 2 : Fix build #Patch Set 3 : Different approach #
Total comments: 1
Patch Set 4 : Address review comments #Patch Set 5 : Rebase patch #
Total comments: 3
Patch Set 6 : Patch for landing #Patch Set 7 : Fix histograms.xml #
Messages
Total messages: 37 (17 generated)
Description was changed from ========== 498233 WIP BUG= ========== to ========== Add UseCounter for image-orientation Add a UseCounter for image-orientation to measure how much content would be affected if we'd turn image-orientation on by default. BUG=498233 ==========
Description was changed from ========== Add UseCounter for image-orientation Add a UseCounter for image-orientation to measure how much content would be affected if we'd turn image-orientation on by default. BUG=498233 ========== to ========== Add UseCounter for image-orientation Add a UseCounter for image-orientation to measure how much content would be affected if we'd turn image-orientation on by default. BUG=498233 ==========
rob.buis@samsung.com changed reviewers: + noel@chromium.org
PTAL. I was looking for a place to measure this at creation instead of paint time, any idea? It seems the actual image is only referenced at paint time...
On 2015/11/05 21:38:06, rwlbuis wrote: > PTAL. I was looking for a place to measure this at creation > instead of paint time, any idea? It seems the actual image is only referenced at > paint time... Yes tricky that it is a paint-time effect. One idea would be to add a UMA for it instead, to measure all the images we paint, and how many of those would get orientated. To do that, maybe add the UMA to BitmapImage. We already UMA count the number and type of images we decode there, see https://crbug.com/513523 for reference. https://crbug.com/513523#10 - defined the UMA histogram values. https://crbug.com/513523#17 - record the histogram in Platform::BitmapImage.
You might not be ale to see bug 513512, so the relevant code reviews were: https://codereview.chromium.org/1249273004 https://codereview.chromium.org/1242263011 Let me know if you can't see them.
On 2015/11/06 00:45:06, noel gordon wrote: > On 2015/11/05 21:38:06, rwlbuis wrote: > > PTAL. I was looking for a place to measure this at creation > > instead of paint time, any idea? It seems the actual image is only referenced > at > > paint time... > > Yes tricky that it is a paint-time effect. One idea would be to add a UMA for it > instead, to measure all the images we paint, and how many of those would get > orientated. > > To do that, maybe add the UMA to BitmapImage. We already UMA count the number > and type of images we decode there, see https://crbug.com/513523 for reference. > > https://crbug.com/513523#10 - defined the UMA histogram values. > https://crbug.com/513523#17 - record the histogram in Platform::BitmapImage. Great idea, I added the histogram change here: https://codereview.chromium.org/1413903010/
Description was changed from ========== Add UseCounter for image-orientation Add a UseCounter for image-orientation to measure how much content would be affected if we'd turn image-orientation on by default. BUG=498233 ========== to ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. BUG=498233 ==========
Description was changed from ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. BUG=498233 ========== to ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. BUG=158753 ==========
On 2015/11/09 16:47:28, rwlbuis wrote: > Great idea, I added the histogram change here: > https://codereview.chromium.org/1413903010/ And I updated the CL to use above histogram.
https://codereview.chromium.org/1413593012/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1413593012/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:170: Platform::current()->histogramEnumeration("Blink.DecodedImageOrientation", m_frames[index].m_orientation.orientation(), OriginLeftBottom + 1); The actual image orientation is not known until isSizeAvailable. Do it the way urvang@ did it when recording UMA for the decoded image type.
Description was changed from ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. BUG=158753 ========== to ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. Historgram added here: https://codereview.chromium.org/1413903010/ BUG=158753 ==========
Description was changed from ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. Historgram added here: https://codereview.chromium.org/1413903010/ BUG=158753 ========== to ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. Historgram added here: https://codereview.chromium.org/1413903010/ BUG=158753 ==========
Description was changed from ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. Historgram added here: https://codereview.chromium.org/1413903010/ BUG=158753 ========== to ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. Histogram added here: https://codereview.chromium.org/1413903010/ BUG=158753 ==========
Description was changed from ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. Histogram added here: https://codereview.chromium.org/1413903010/ BUG=158753 ========== to ========== Record UMA stats for Blink decoded image orientation Record UMA stats for Blink decoded bitmap image orientation values. BUG=158753 ==========
Sorry, for the delay, was at BlinkOn :) On 2015/11/10 01:14:56, noel gordon wrote: > https://codereview.chromium.org/1413593012/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): > > https://codereview.chromium.org/1413593012/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:170: > Platform::current()->histogramEnumeration("Blink.DecodedImageOrientation", > m_frames[index].m_orientation.orientation(), OriginLeftBottom + 1); > The actual image orientation is not known until isSizeAvailable. Do it the way > urvang@ did it when recording UMA for the decoded image type. Done! PTAL.
LGTM - blink side seems fine to me. On 2015/11/16 22:02:10, rwlbuis wrote: > Sorry, for the delay, was at BlinkOn :) Yes, I was wondering what happened to you!
rob.buis@samsung.com changed reviewers: + asvitkine@chromium.org
+asvitkine@chromium.org for histograms.xml review.
lgtm % nits https://codereview.chromium.org/1413593012/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1413593012/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:371: Platform::current()->histogramEnumeration("Blink.DecodedImageOrientation", m_source.orientationAtIndex(0).orientation(), OriginLeftBottom + 1); Nit: Can you add an entry to the enum with OriginLeftBottom + 1 as it's value? That way, its definition is next to the enum and not here. https://codereview.chromium.org/1413593012/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1413593012/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2917: +<histogram name="Blink.DecodedImageOrientation" enum="DecodedImageOrientation"> Nit: Suggest adding another . in the name: Blink.DecodedImage.Orientation This makes it slightly more readable and will group things together in case new histograms about the decoded image are added in the future. (If you change it, don't forgot to change both here and in the code.) https://codereview.chromium.org/1413593012/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56846: + <int value="0" label="kImageOrientationUnknown"/> Nit: Make these names human readable, e.g. "Top Left", "Top Right", etc. Since they'll be shown on the dashboard.
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, noel@chromium.org Link to the patchset: https://codereview.chromium.org/1413593012/#ps100001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413593012/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413593012/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
** Presubmit ERRORS ** histograms.xml is not formatted correctly; run pretty_print.py to fix On Wed, Nov 18, 2015 at 11:35 AM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub... > ) > > https://codereview.chromium.org/1413593012/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
** Presubmit ERRORS ** histograms.xml is not formatted correctly; run pretty_print.py to fix On Wed, Nov 18, 2015 at 11:35 AM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub... > ) > > https://codereview.chromium.org/1413593012/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/18 16:39:18, Alexei Svitkine (slow) wrote: > ** Presubmit ERRORS ** > histograms.xml is not formatted correctly; run pretty_print.py to fix Thanks, will fix. I ignored it initially since I thought it was caused by existing parts of the xml.
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, noel@chromium.org Link to the patchset: https://codereview.chromium.org/1413593012/#ps120001 (title: "Fix histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413593012/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413593012/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413593012/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413593012/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7a103b24dadf42f27e914b024977e58ae263a836 Cr-Commit-Position: refs/heads/master@{#360400} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
