Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(277)

Issue 1716173002: Histograms for H264EncoderImpl/H264DecoderImpl initialization and errors (Closed)

Created:
4 years, 10 months ago by hbos
Modified:
4 years, 9 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Histograms for H264EncoderImpl/H264DecoderImpl initialization and errors. The stats are counts using enumeration, an instance of H264EncoderImpl/H264DecoderImpl will report at most 1 Init and 1 Error for its entire lifetime. This is to avoid spamming reports if initialization or coding fails and it retries in a loop. The Init stats will give us an idea of usage counts for the encoder/decoder. The Error stats will give us an idea of how many of these usages encounters some type of problem, such as encode or decode errors. - WebRTC.Video.H264EncoderImpl.Event: * kH264EncoderEventInit: Occurs at InitEncode. * kH264EncoderEventError: Occurs if any type of error occurs during initialization or encoding. - WebRTC.Video.H264DecoderImpl.Event: * kH264DecoderEventInit: Occurs at InitDecode. * kH264DecoderEventError: Occurs if any type of error occurs during initialization, AVGetBuffer2 or decoding. Chromium sibling CL: https://codereview.chromium.org/1719273002/ BUG=chromium:500605, chromium:468365 Committed: https://crrev.com/12f4cda0867b7264f0ad65b2b00e592de9a2a753 Cr-Commit-Position: refs/heads/master@{#11736}

Patch Set 1 #

Total comments: 8

Patch Set 2 : RTC_HISTOGRAM_ENUMERATION with State as Init or Error instead of counters #

Patch Set 3 : Renamed "...State" enums to "...Event" #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+114 lines, -10 lines) Patch
M webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h View 1 1 chunk +7 lines, -0 lines 0 comments Download
M webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc View 1 2 11 chunks +48 lines, -4 lines 4 comments Download
M webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h View 1 1 chunk +7 lines, -0 lines 0 comments Download
M webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc View 1 2 11 chunks +52 lines, -6 lines 0 comments Download

Messages

Total messages: 34 (12 generated)
hbos
Please take a look, stefan & hlundin. I looked at other examples of histograms in ...
4 years, 10 months ago (2016-02-22 13:52:13 UTC) #3
hbos
+rkaplow for finch ambassador-ness (note: this is the webrtc repo with webrtc macros for the ...
4 years, 10 months ago (2016-02-22 20:30:19 UTC) #5
stefan-webrtc
My understanding of RTC_HISTOGRAM_COUNTS suggests that what you are doing is correct. Is your plan ...
4 years, 10 months ago (2016-02-22 20:54:54 UTC) #6
hbos
On 2016/02/22 20:54:54, stefan-webrtc (holmer) wrote: > My understanding of RTC_HISTOGRAM_COUNTS suggests that what you ...
4 years, 10 months ago (2016-02-22 21:10:31 UTC) #8
hbos
PTAL stefan, hlundin. Want this in before branch cut (i.e. before roll on thursday).
4 years, 10 months ago (2016-02-23 08:26:13 UTC) #9
stefan-webrtc
On 2016/02/22 21:10:31, hbos wrote: > On 2016/02/22 20:54:54, stefan-webrtc (holmer) wrote: > > My ...
4 years, 10 months ago (2016-02-23 08:38:51 UTC) #10
hbos
Sweet. Looksies, Other Henrik? :}
4 years, 10 months ago (2016-02-23 08:46:31 UTC) #11
hbos
Oh btw, do you know if the sibling CL that modifies chromium's histograms.xml (https://codereview.chromium.org/1719273002/) has ...
4 years, 10 months ago (2016-02-23 09:48:20 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1716173002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1716173002/1
4 years, 10 months ago (2016-02-23 09:48:57 UTC) #14
stefan-webrtc
On 2016/02/23 09:48:20, hbos wrote: > Oh btw, do you know if the sibling CL ...
4 years, 10 months ago (2016-02-23 10:05:08 UTC) #15
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
4 years, 10 months ago (2016-02-23 11:49:12 UTC) #17
hlundin-webrtc
https://codereview.webrtc.org/1716173002/diff/1/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1716173002/diff/1/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode374 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:374: RTC_HISTOGRAM_COUNTS("WebRTC.Video.H264DecoderImpl.Init", Did you consider using RTC_HISTOGRAM_ENUMERATION instead? It seems ...
4 years, 10 months ago (2016-02-23 13:55:54 UTC) #18
hbos
Thanks, changed to RTC_HISTOGRAM_ENUMRATION instead. Please take another look. https://codereview.webrtc.org/1716173002/diff/1/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1716173002/diff/1/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode374 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:374: ...
4 years, 10 months ago (2016-02-23 14:43:26 UTC) #20
hbos
On 2016/02/23 14:43:26, hbos wrote: > Thanks, changed to RTC_HISTOGRAM_ENUMRATION instead. Please take another look. ...
4 years, 10 months ago (2016-02-23 14:45:53 UTC) #21
hbos
On 2016/02/23 14:45:53, hbos wrote: > On 2016/02/23 14:43:26, hbos wrote: > > Thanks, changed ...
4 years, 10 months ago (2016-02-23 14:52:19 UTC) #23
hbos
https://codereview.webrtc.org/1716173002/diff/40001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1716173002/diff/40001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode42 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:42: kH264DecoderEventMax = 16, The example I looked at was ...
4 years, 10 months ago (2016-02-23 15:01:22 UTC) #24
hlundin-webrtc
lgtm https://codereview.webrtc.org/1716173002/diff/40001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1716173002/diff/40001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode42 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:42: kH264DecoderEventMax = 16, On 2016/02/23 15:01:22, hbos wrote: ...
4 years, 10 months ago (2016-02-24 09:17:57 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1716173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1716173002/40001
4 years, 10 months ago (2016-02-24 09:55:04 UTC) #28
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 9 months ago (2016-02-24 11:03:09 UTC) #30
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/12f4cda0867b7264f0ad65b2b00e592de9a2a753 Cr-Commit-Position: refs/heads/master@{#11736}
4 years, 9 months ago (2016-02-24 11:03:16 UTC) #32
hbos
https://codereview.webrtc.org/1716173002/diff/40001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1716173002/diff/40001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode42 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:42: kH264DecoderEventMax = 16, On 2016/02/24 09:17:57, hlundin-webrtc wrote: > ...
4 years, 9 months ago (2016-02-24 11:11:25 UTC) #33
rkaplow
4 years, 9 months ago (2016-02-24 16:10:08 UTC) #34
Message was sent while issue was closed.
lgtm

sorry for delay

https://codereview.webrtc.org/1716173002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1716173002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:42:
kH264DecoderEventMax = 16,
On 2016/02/24 11:11:25, hbos wrote:
> On 2016/02/24 09:17:57, hlundin-webrtc wrote:
> > On 2016/02/23 15:01:22, hbos wrote:
> > > The example I looked at was WebRTC.Video.Encoder.CodecType which had a
much
> > > higher "Max" than the number of enum entries (64). I thought the margin
was
> in
> > > case more entries are added in the future and also used a higher value.
But
> > > perhaps this is not necessary, and max should be either 1 or 2 depending
on
> if
> > > its inclusive or exclusive. Do any of you know why a higher Max value was
> used
> > > in that case and if it makes sense in this case?
> > 
> > I don't know the history behind CodecType. The description in metrics.h says
> > "|boundary| should be above the max enumerator sample" which is interpret as
> > being exclusive. I don't know what happens if you later on want to add more
> enum
> > values and have to raise the boundary.
> 
> Ok. Landed as-is. rkaplow if you know I could adjust "Max" in a follow-up if
> necessary?

usually people don't even set the numbers here manually.
It should be fine to adjust the max value, yes.

Powered by Google App Engine
This is Rietveld 408576698