|
|
Created:
3 years, 10 months ago by tommi (sloooow) - chröme Modified:
3 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histogram to track down errors when initializing the audio client.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2712483006
Cr-Commit-Position: refs/heads/master@{#452599}
Committed: https://chromium.googlesource.com/chromium/src/+/a55e42d9d9805d0ef86b82370ed3fe054f1401bf
Patch Set 1 #
Total comments: 3
Patch Set 2 : Switch HRESULT to Hresult #Patch Set 3 : Merge Hresult and D3DHresult, update 'HRESULT' use in a few places to use 'enum' #
Messages
Total messages: 24 (8 generated)
Description was changed from ========== Add histogram to track down errors when initializing the audio client. BUG= ========== to ========== Add histogram to track down errors when initializing the audio client. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
tommi@chromium.org changed reviewers: + asvitkine@chromium.org, henrika@chromium.org
henrika - main reviewer asvitkine - histogram owner
LGTM https://codereview.chromium.org/2712483006/diff/1/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2712483006/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:718: UMA_HISTOGRAM_SPARSE_SLOWLY("Media.Audio.Capture.Win.InitError", hr); Just FYI. Mac specific stats are callled e.g. Media.Audio.InputBufferSizeWasChangedMac. Like your style better.
https://codereview.chromium.org/2712483006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2712483006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:24602: +<histogram name="Media.Audio.Capture.Win.InitError" units="HRESULT"> I think we have an enum for HRESULT. Can you specify it?
https://codereview.chromium.org/2712483006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2712483006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:24602: +<histogram name="Media.Audio.Capture.Win.InitError" units="HRESULT"> On 2017/02/23 15:29:49, Alexei Svitkine (slow) wrote: > I think we have an enum for HRESULT. Can you specify it? It's a 32 bit generic error value. I see we have WinGetLastError, which is a small subset of HRESULT (via HRESULT_FROM_WIN32). Is it OK with you if I start adding a definition when we know more about what errors we will be getting?
Yeah, re-using WinGetLastError and expanding it sgtm. On Thu, Feb 23, 2017 at 11:20 AM, <tommi@chromium.org> wrote: > > https://codereview.chromium.org/2712483006/diff/1/tools/ > metrics/histograms/histograms.xml > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2712483006/diff/1/tools/ > metrics/histograms/histograms.xml#newcode24602 > tools/metrics/histograms/histograms.xml:24602: +<histogram > name="Media.Audio.Capture.Win.InitError" units="HRESULT"> > On 2017/02/23 15:29:49, Alexei Svitkine (slow) wrote: > > I think we have an enum for HRESULT. Can you specify it? > > It's a 32 bit generic error value. I see we have WinGetLastError, which > is a small subset of HRESULT (via HRESULT_FROM_WIN32). > Is it OK with you if I start adding a definition when we know more about > what errors we will be getting? > > https://codereview.chromium.org/2712483006/ > -- 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 2017/02/23 16:31:46, Alexei Svitkine (slow) wrote: > Yeah, re-using WinGetLastError and expanding it sgtm. Unfortunately I think it might be hard to reuse it (but there might be a trick?), since GLE constants are just one 'facility' (or domain) in HRESULT errors. For example HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) == 0x80070002. > On Thu, Feb 23, 2017 at 11:20 AM, <mailto:tommi@chromium.org> wrote: > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > metrics/histograms/histograms.xml > > File tools/metrics/histograms/histograms.xml (right): > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > metrics/histograms/histograms.xml#newcode24602 > > tools/metrics/histograms/histograms.xml:24602: +<histogram > > name="Media.Audio.Capture.Win.InitError" units="HRESULT"> > > On 2017/02/23 15:29:49, Alexei Svitkine (slow) wrote: > > > I think we have an enum for HRESULT. Can you specify it? > > > > It's a 32 bit generic error value. I see we have WinGetLastError, which > > is a small subset of HRESULT (via HRESULT_FROM_WIN32). > > Is it OK with you if I start adding a definition when we know more about > > what errors we will be getting? > > > > https://codereview.chromium.org/2712483006/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
On 2017/02/23 16:34:18, tommi (krómíum) wrote: > On 2017/02/23 16:31:46, Alexei Svitkine (slow) wrote: > > Yeah, re-using WinGetLastError and expanding it sgtm. > > Unfortunately I think it might be hard to reuse it (but there might be a > trick?), since GLE constants are just one 'facility' (or domain) in HRESULT > errors. For example HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) == 0x80070002. Copy paste works though :) However, I think we can be pragmatic about populating an HRESULT enum and define the errors we actually see. As is, it's not likely that we'll see HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) for example. > > > On Thu, Feb 23, 2017 at 11:20 AM, <mailto:tommi@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > metrics/histograms/histograms.xml > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > metrics/histograms/histograms.xml#newcode24602 > > > tools/metrics/histograms/histograms.xml:24602: +<histogram > > > name="Media.Audio.Capture.Win.InitError" units="HRESULT"> > > > On 2017/02/23 15:29:49, Alexei Svitkine (slow) wrote: > > > > I think we have an enum for HRESULT. Can you specify it? > > > > > > It's a 32 bit generic error value. I see we have WinGetLastError, which > > > is a small subset of HRESULT (via HRESULT_FROM_WIN32). > > > Is it OK with you if I start adding a definition when we know more about > > > what errors we will be getting? > > > > > > https://codereview.chromium.org/2712483006/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
Defining a separate HRESULT enum and populating it as we see codes sgtm if it's not the same one as used by WinGetLastError. On Thu, Feb 23, 2017 at 11:36 AM, <tommi@chromium.org> wrote: > On 2017/02/23 16:34:18, tommi (krómíum) wrote: > > On 2017/02/23 16:31:46, Alexei Svitkine (slow) wrote: > > > Yeah, re-using WinGetLastError and expanding it sgtm. > > > > Unfortunately I think it might be hard to reuse it (but there might be a > > trick?), since GLE constants are just one 'facility' (or domain) in > HRESULT > > errors. For example HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) == > 0x80070002. > > Copy paste works though :) However, I think we can be pragmatic about > populating an HRESULT enum and define the errors we actually see. As is, > it's > not likely that we'll see HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) for > example. > > > > > > On Thu, Feb 23, 2017 at 11:20 AM, <mailto:tommi@chromium.org> wrote: > > > > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > > metrics/histograms/histograms.xml > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > > metrics/histograms/histograms.xml#newcode24602 > > > > tools/metrics/histograms/histograms.xml:24602: +<histogram > > > > name="Media.Audio.Capture.Win.InitError" units="HRESULT"> > > > > On 2017/02/23 15:29:49, Alexei Svitkine (slow) wrote: > > > > > I think we have an enum for HRESULT. Can you specify it? > > > > > > > > It's a 32 bit generic error value. I see we have WinGetLastError, > which > > > > is a small subset of HRESULT (via HRESULT_FROM_WIN32). > > > > Is it OK with you if I start adding a definition when we know more > about > > > > what errors we will be getting? > > > > > > > > https://codereview.chromium.org/2712483006/ > > > > > > > > > > -- > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2712483006/ > -- 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.
Switch HRESULT to Hresult
On 2017/02/23 16:49:09, Alexei Svitkine (slow) wrote: > Defining a separate HRESULT enum and populating it as we see codes sgtm if > it's not the same one as used by WinGetLastError. > > On Thu, Feb 23, 2017 at 11:36 AM, <mailto:tommi@chromium.org> wrote: > > > On 2017/02/23 16:34:18, tommi (krómíum) wrote: > > > On 2017/02/23 16:31:46, Alexei Svitkine (slow) wrote: > > > > Yeah, re-using WinGetLastError and expanding it sgtm. > > > > > > Unfortunately I think it might be hard to reuse it (but there might be a > > > trick?), since GLE constants are just one 'facility' (or domain) in > > HRESULT > > > errors. For example HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) == > > 0x80070002. > > > > Copy paste works though :) However, I think we can be pragmatic about > > populating an HRESULT enum and define the errors we actually see. As is, > > it's > > not likely that we'll see HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) for > > example. > > > > > > > > > On Thu, Feb 23, 2017 at 11:20 AM, <mailto:tommi@chromium.org> wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > > > metrics/histograms/histograms.xml > > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > > > metrics/histograms/histograms.xml#newcode24602 > > > > > tools/metrics/histograms/histograms.xml:24602: +<histogram > > > > > name="Media.Audio.Capture.Win.InitError" units="HRESULT"> > > > > > On 2017/02/23 15:29:49, Alexei Svitkine (slow) wrote: > > > > > > I think we have an enum for HRESULT. Can you specify it? > > > > > > > > > > It's a 32 bit generic error value. I see we have WinGetLastError, > > which > > > > > is a small subset of HRESULT (via HRESULT_FROM_WIN32). > > > > > Is it OK with you if I start adding a definition when we know more > > about > > > > > what errors we will be getting? > > > > > > > > > > https://codereview.chromium.org/2712483006/ > > > > > > > > > > > > > -- > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > https://codereview.chromium.org/2712483006/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. Hey Alexei - I think I misread your comment earlier. I somehow read it as us *not* having an HRESULT (and I didn't find it before). I started adding it, but the found that we do have an enum "Hresult", and I've switched to using that for now. Btw, I also noticed that we have "D3DHresult" which could actually be merged with "Hresult".
The CQ bit was checked by tommi@chromium.org to run a CQ dry run
Do you mind merging the two hresult enums then in this CL? Also, you need to specify enum= rather than units= for it to be associated with the enum. On Thu, Feb 23, 2017 at 1:07 PM, <tommi@chromium.org> wrote: > On 2017/02/23 16:49:09, Alexei Svitkine (slow) wrote: > > Defining a separate HRESULT enum and populating it as we see codes sgtm > if > > it's not the same one as used by WinGetLastError. > > > > On Thu, Feb 23, 2017 at 11:36 AM, <mailto:tommi@chromium.org> wrote: > > > > > On 2017/02/23 16:34:18, tommi (krómíum) wrote: > > > > On 2017/02/23 16:31:46, Alexei Svitkine (slow) wrote: > > > > > Yeah, re-using WinGetLastError and expanding it sgtm. > > > > > > > > Unfortunately I think it might be hard to reuse it (but there might > be a > > > > trick?), since GLE constants are just one 'facility' (or domain) in > > > HRESULT > > > > errors. For example HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) == > > > 0x80070002. > > > > > > Copy paste works though :) However, I think we can be pragmatic about > > > populating an HRESULT enum and define the errors we actually see. As > is, > > > it's > > > not likely that we'll see HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) for > > > example. > > > > > > > > > > > > On Thu, Feb 23, 2017 at 11:20 AM, <mailto:tommi@chromium.org> > wrote: > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > > > > metrics/histograms/histograms.xml > > > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > > > > metrics/histograms/histograms.xml#newcode24602 > > > > > > tools/metrics/histograms/histograms.xml:24602: +<histogram > > > > > > name="Media.Audio.Capture.Win.InitError" units="HRESULT"> > > > > > > On 2017/02/23 15:29:49, Alexei Svitkine (slow) wrote: > > > > > > > I think we have an enum for HRESULT. Can you specify it? > > > > > > > > > > > > It's a 32 bit generic error value. I see we have WinGetLastError, > > > which > > > > > > is a small subset of HRESULT (via HRESULT_FROM_WIN32). > > > > > > Is it OK with you if I start adding a definition when we know > more > > > about > > > > > > what errors we will be getting? > > > > > > > > > > > > https://codereview.chromium.org/2712483006/ > > > > > > > > > > > > > > > > -- > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > > > https://codereview.chromium.org/2712483006/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > Hey Alexei - I think I misread your comment earlier. I somehow read it as > us > *not* having an HRESULT (and I didn't find it before). I started adding > it, but > the found that we do have an enum "Hresult", and I've switched to using > that for > now. > > Btw, I also noticed that we have "D3DHresult" which could actually be > merged > with "Hresult". > > https://codereview.chromium.org/2712483006/ > -- 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.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Merge Hresult and D3DHresult, update 'HRESULT' use in a few places to use 'enum'
Done On Thu, Feb 23, 2017 at 7:09 PM Alexei Svitkine <asvitkine@chromium.org> wrote: > Do you mind merging the two hresult enums then in this CL? > > Also, you need to specify enum= rather than units= for it to be associated > with the enum. > > On Thu, Feb 23, 2017 at 1:07 PM, <tommi@chromium.org> wrote: > > On 2017/02/23 16:49:09, Alexei Svitkine (slow) wrote: > > Defining a separate HRESULT enum and populating it as we see codes sgtm > if > > it's not the same one as used by WinGetLastError. > > > > On Thu, Feb 23, 2017 at 11:36 AM, <mailto:tommi@chromium.org> wrote: > > > > > On 2017/02/23 16:34:18, tommi (krómíum) wrote: > > > > On 2017/02/23 16:31:46, Alexei Svitkine (slow) wrote: > > > > > Yeah, re-using WinGetLastError and expanding it sgtm. > > > > > > > > Unfortunately I think it might be hard to reuse it (but there might > be a > > > > trick?), since GLE constants are just one 'facility' (or domain) in > > > HRESULT > > > > errors. For example HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) == > > > 0x80070002. > > > > > > Copy paste works though :) However, I think we can be pragmatic about > > > populating an HRESULT enum and define the errors we actually see. As > is, > > > it's > > > not likely that we'll see HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) for > > > example. > > > > > > > > > > > > On Thu, Feb 23, 2017 at 11:20 AM, <mailto:tommi@chromium.org> > wrote: > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > > > > metrics/histograms/histograms.xml > > > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > https://codereview.chromium.org/2712483006/diff/1/tools/ > > > > > > metrics/histograms/histograms.xml#newcode24602 > > > > > > tools/metrics/histograms/histograms.xml:24602: +<histogram > > > > > > name="Media.Audio.Capture.Win.InitError" units="HRESULT"> > > > > > > On 2017/02/23 15:29:49, Alexei Svitkine (slow) wrote: > > > > > > > I think we have an enum for HRESULT. Can you specify it? > > > > > > > > > > > > It's a 32 bit generic error value. I see we have WinGetLastError, > > > which > > > > > > is a small subset of HRESULT (via HRESULT_FROM_WIN32). > > > > > > Is it OK with you if I start adding a definition when we know > more > > > about > > > > > > what errors we will be getting? > > > > > > > > > > > > https://codereview.chromium.org/2712483006/ > > > > > > > > > > > > > > > > -- > > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > > > https://codereview.chromium.org/2712483006/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > Hey Alexei - I think I misread your comment earlier. I somehow read it as > us > *not* having an HRESULT (and I didn't find it before). I started adding > it, but > the found that we do have an enum "Hresult", and I've switched to using > that for > now. > > Btw, I also noticed that we have "D3DHresult" which could actually be > merged > with "Hresult". > > https://codereview.chromium.org/2712483006/ > > > -- > 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. > -- 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.
lgtm - thanks for the clean up!
The CQ bit was checked by tommi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrika@chromium.org Link to the patchset: https://codereview.chromium.org/2712483006/#ps40001 (title: "Merge Hresult and D3DHresult, update 'HRESULT' use in a few places to use 'enum'")
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": 40001, "attempt_start_ts": 1487874141390570, "parent_rev": "8a213f053a3a7cbb2b09cc6a51bc26faf3e1da51", "commit_rev": "a55e42d9d9805d0ef86b82370ed3fe054f1401bf"}
Message was sent while issue was closed.
Description was changed from ========== Add histogram to track down errors when initializing the audio client. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add histogram to track down errors when initializing the audio client. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2712483006 Cr-Commit-Position: refs/heads/master@{#452599} Committed: https://chromium.googlesource.com/chromium/src/+/a55e42d9d9805d0ef86b82370ed3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a55e42d9d9805d0ef86b82370ed3... |