Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2506363003:
Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms (Closed)
DescriptionAdd Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms
This enum result is recorded for verifying HW codec is used on ARC++,
or if there is any initialization error.
BUG=661864
TEST=test it on Elm and make sure the flag shows correct value in
chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult
Committed: https://crrev.com/9448e050e5d0ba0862b23438b8ebcd61c756dac2
Cr-Commit-Position: refs/heads/master@{#434880}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add Media.ArcGpuVideoDecodeAccelerator.InitializeSuccess to histograms #
Total comments: 4
Patch Set 3 : Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms #
Total comments: 6
Patch Set 4 : Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms #
Messages
Total messages: 38 (15 generated)
johnylin@chromium.org changed reviewers: + henryhsu@chromium.org, owenlin@chromium.org, posciak@chromium.org, wuchengli@chromium.org
wuchengli@chromium.org changed reviewers: - henryhsu@chromium.org
Owen. Please help review this.
https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.cc:74: ArcVideoAccelerator::Result result = InitializeTask(config, client); auto result = ? https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.h:39: ArcVideoAccelerator::Result InitializeTask( Put in private.
https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.cc:80: ArcVideoAccelerator::Result ArcGpuVideoDecodeAccelerator::InitializeTask( Do we need a separate method?
Description was changed from ========== Add Media.ArcGpuVideoDecodeAccelerator.InitializeSuccess to histograms This flag is for verifying HW codec is used on ARC++. BUG=661864 TEST=test it on Elm and make sure the flag shows correct value in chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeSuccess ========== to ========== Add Media.ArcGpuVideoDecodeAccelerator.InitializeSuccess to histograms This flag is for verifying HW codec is used on ARC++. BUG=661864 TEST=test it on Elm and make sure the flag shows correct value in chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeSuccess ==========
wuchengli@chromium.org changed reviewers: - wuchengli@chromium.org
https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.cc:80: ArcVideoAccelerator::Result ArcGpuVideoDecodeAccelerator::InitializeTask( On 2016/11/17 08:45:31, Pawel Osciak wrote: > Do we need a separate method? I also wondered if the separate is good or bad. Since we also want to record (result != SUCCESS) case, in original Initialize(), we need to add "UMA_HISTOGRAM_BOOLEAN(..., false)" before every "return NON-SUCCESS". I worried that would be kind of code redundant.
https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.cc:74: ArcVideoAccelerator::Result result = InitializeTask(config, client); On 2016/11/17 08:28:28, Owen Lin wrote: > auto result = ? Done. https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/2506363003/diff/1/chrome/gpu/arc_gpu_video_de... chrome/gpu/arc_gpu_video_decode_accelerator.h:39: ArcVideoAccelerator::Result InitializeTask( On 2016/11/17 08:28:28, Owen Lin wrote: > Put in private. Done.
lgtm
https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:76: result == SUCCESS); Should we instead record the full result status enum, not just SUCCESS or not? For example, it may be interesting to know about INSUFFICIENT_RESOURCES. https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:108: // Intialization process. Perhaps we would be able to have a more descriptive sentence, e.g. "a helper method to simplify reporting of the status returned to UMA."
https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.cc:76: result == SUCCESS); On 2016/11/21 09:02:37, Pawel Osciak wrote: > Should we instead record the full result status enum, not just SUCCESS or not? > For example, it may be interesting to know about INSUFFICIENT_RESOURCES. Sounds good. Done. https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... chrome/gpu/arc_gpu_video_decode_accelerator.h:108: // Intialization process. On 2016/11/21 09:02:37, Pawel Osciak wrote: > Perhaps we would be able to have a more descriptive sentence, e.g. "a helper > method to simplify reporting of the status returned to UMA." Done.
On 2016/11/21 15:39:58, johnylin1 wrote: > https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... > File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... > chrome/gpu/arc_gpu_video_decode_accelerator.cc:76: result == SUCCESS); > On 2016/11/21 09:02:37, Pawel Osciak wrote: > > Should we instead record the full result status enum, not just SUCCESS or not? > > For example, it may be interesting to know about INSUFFICIENT_RESOURCES. > > Sounds good. Done. > > https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... > File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): > > https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... > chrome/gpu/arc_gpu_video_decode_accelerator.h:108: // Intialization process. > On 2016/11/21 09:02:37, Pawel Osciak wrote: > > Perhaps we would be able to have a more descriptive sentence, e.g. "a helper > > method to simplify reporting of the status returned to UMA." > > Done. You may like to update the title of the issue.
Description was changed from
==========
Add Media.ArcGpuVideoDecodeAccelerator.InitializeSuccess to histograms
This flag is for verifying HW codec is used on ARC++.
BUG=661864
TEST=test it on Elm and make sure the flag shows correct value in
chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeSuccess
==========
to
==========
Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms
This enum result is recorded for verifying HW codec is used on ARC++,
or if there is any initialization error.
BUG=661864
TEST=test it on Elm and make sure the flag shows correct value in
chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult
==========
On 2016/11/22 02:30:44, Owen Lin wrote: > On 2016/11/21 15:39:58, johnylin1 wrote: > > > https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... > > File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right): > > > > > https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... > > chrome/gpu/arc_gpu_video_decode_accelerator.cc:76: result == SUCCESS); > > On 2016/11/21 09:02:37, Pawel Osciak wrote: > > > Should we instead record the full result status enum, not just SUCCESS or > not? > > > For example, it may be interesting to know about INSUFFICIENT_RESOURCES. > > > > Sounds good. Done. > > > > > https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... > > File chrome/gpu/arc_gpu_video_decode_accelerator.h (right): > > > > > https://codereview.chromium.org/2506363003/diff/20001/chrome/gpu/arc_gpu_vide... > > chrome/gpu/arc_gpu_video_decode_accelerator.h:108: // Intialization process. > > On 2016/11/21 09:02:37, Pawel Osciak wrote: > > > Perhaps we would be able to have a more descriptive sentence, e.g. "a helper > > > method to simplify reporting of the status returned to UMA." > > > > Done. > > You may like to update the title of the issue. Thanks, done.
lgtm % nits https://codereview.chromium.org/2506363003/diff/40001/chrome/gpu/arc_video_ac... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/2506363003/diff/40001/chrome/gpu/arc_video_ac... chrome/gpu/arc_video_accelerator.h:58: enum Result { Please add a comment: "Note: this enum is used for UMA reporting. The existing values should not be rearranged, reused or removed and any additions should include updates to UMA reporting and histograms.xml." https://codereview.chromium.org/2506363003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2506363003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23062: + Records the return value of initializing arc gpu video decode accelerator Counts of status values returned from calls to ArcGpuVideoDecodeAccelerator::Initialize(). https://codereview.chromium.org/2506363003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74441: + <summary>Defines Arc Video Accelerator initialization status</summary> s/Arc Video Accelerator/ArcVideoAccelerator/
https://codereview.chromium.org/2506363003/diff/40001/chrome/gpu/arc_video_ac... File chrome/gpu/arc_video_accelerator.h (right): https://codereview.chromium.org/2506363003/diff/40001/chrome/gpu/arc_video_ac... chrome/gpu/arc_video_accelerator.h:58: enum Result { On 2016/11/22 04:04:48, Pawel Osciak wrote: > Please add a comment: > > "Note: this enum is used for UMA reporting. The existing values should not be > rearranged, reused or removed and any additions should include updates to UMA > reporting and histograms.xml." Done. https://codereview.chromium.org/2506363003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2506363003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23062: + Records the return value of initializing arc gpu video decode accelerator On 2016/11/22 04:04:48, Pawel Osciak wrote: > Counts of status values returned from calls to > ArcGpuVideoDecodeAccelerator::Initialize(). Done. https://codereview.chromium.org/2506363003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:74441: + <summary>Defines Arc Video Accelerator initialization status</summary> On 2016/11/22 04:04:49, Pawel Osciak wrote: > s/Arc Video Accelerator/ArcVideoAccelerator/ Done.
The CQ bit was checked by johnylin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2506363003/#ps60001 (title: "Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
johnylin@chromium.org changed reviewers: + asvitkine@chromium.org
johnylin@chromium.org changed reviewers: + isherman@chromium.org - asvitkine@chromium.org
johnylin@chromium.org changed reviewers: + asvitkine@chromium.org
On 2016/11/28 09:14:36, johnylin1 wrote: Hi Ilya, Please help to review histograms.xml, thanks.
metrics lgtm
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480387777140350,
"parent_rev": "3a73b38a5337cca4e13b5089ea07a023559c0028", "commit_rev":
"88e94ceb2fa06a367c0efa8483f9608403ef4beb"}
Message was sent while issue was closed.
Description was changed from
==========
Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms
This enum result is recorded for verifying HW codec is used on ARC++,
or if there is any initialization error.
BUG=661864
TEST=test it on Elm and make sure the flag shows correct value in
chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult
==========
to
==========
Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms
This enum result is recorded for verifying HW codec is used on ARC++,
or if there is any initialization error.
BUG=661864
TEST=test it on Elm and make sure the flag shows correct value in
chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms
This enum result is recorded for verifying HW codec is used on ARC++,
or if there is any initialization error.
BUG=661864
TEST=test it on Elm and make sure the flag shows correct value in
chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult
==========
to
==========
Add Media.ArcGpuVideoDecodeAccelerator.InitializeResult to histograms
This enum result is recorded for verifying HW codec is used on ARC++,
or if there is any initialization error.
BUG=661864
TEST=test it on Elm and make sure the flag shows correct value in
chrome://histograms/Media.ArcGpuVideoDecodeAccelerator.InitializeResult
Committed: https://crrev.com/9448e050e5d0ba0862b23438b8ebcd61c756dac2
Cr-Commit-Position: refs/heads/master@{#434880}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9448e050e5d0ba0862b23438b8ebcd61c756dac2 Cr-Commit-Position: refs/heads/master@{#434880} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
