Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
45053003:
Add UMA metrics for ffmpeg color ranges. (Closed)
|
Created:
7 years, 1 month ago by rileya (GONE FROM CHROMIUM) Modified:
7 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd UMA metrics for ffmpeg color ranges.
BUG=310272
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232875
Patch Set 1 #Patch Set 2 : Add enum for colorrange #
Total comments: 1
Patch Set 3 : Log VideoFrame::Format, instead of ffmpeg's AvPixelFormat. #
Total comments: 1
Patch Set 4 : Remove unnecessary + 1 in histogram macro. #Patch Set 5 : Rename FORMAT_MAX to NUM_FORMATS. #
Total comments: 1
Patch Set 6 : Rename format histogram max enum, add cases for it in several switches to clear up compile errors. #Patch Set 7 : Add period to comment. #
Total comments: 1
Patch Set 8 : Drop the 'FORMAT_'. Just 'HISTOGRAM_MAX', it's cleaner. #
Total comments: 5
Patch Set 9 : Note when histograms are emitted. #Patch Set 10 : Remove gaps in VideoFrame::Format #Patch Set 11 : Remove VideoFrame::Format stuff. #Patch Set 12 : #Patch Set 13 : Attempt to merge #Patch Set 14 : Merge, correctly this time? #Messages
Total messages: 39 (0 generated)
On 2013/10/28 16:54:38, rileya wrote: On John's suggestion I was looking into adding the enum names to histograms.xml, and looking at the ffmpeg AVPixelFormat enum (http://ffmpeg.org/doxygen/trunk/pixfmt_8h_source.html), I'm not quite sure how to handle it. There's a bunch of #ifdefs whose values I'm not sure of (and if they're consistent between platforms/etc) that would alter the enumerations and I'm not sure how often formats are added, and if that's done in such a way as to preserve the same enumerations between releases...
https://codereview.chromium.org/45053003/diff/50001/media/ffmpeg/ffmpeg_commo... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/45053003/diff/50001/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.cc:375: "Media.VideoPixelFormat", stream->codec->pix_fmt, PIX_FMT_NB + 1); you're correct that since these values aren't guaranteed to be stable, we shouldn't use them for UMA let's use VideoFrame::Format and add a comment to that enum that values shouldn't change, similar to other enums: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/pipelin...
On 2013/10/28 21:16:38, scherkus wrote: > https://codereview.chromium.org/45053003/diff/50001/media/ffmpeg/ffmpeg_commo... > File media/ffmpeg/ffmpeg_common.cc (right): > > https://codereview.chromium.org/45053003/diff/50001/media/ffmpeg/ffmpeg_commo... > media/ffmpeg/ffmpeg_common.cc:375: "Media.VideoPixelFormat", > stream->codec->pix_fmt, PIX_FMT_NB + 1); > you're correct that since these values aren't guaranteed to be stable, we > shouldn't use them for UMA > > let's use VideoFrame::Format and add a comment to that enum that values > shouldn't change, similar to other enums: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/pipelin... Okay, it now logs the VideoFrame::Format (which now has a note about being logged). It still logs the ffmpeg AVColorRange, because (unlike the pixel format) it appears it will remain reasonably stable, and should give us useful data about MPEG vs JPEG color range.
https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.cc:376: AVCOL_RANGE_NB + 1); should this be AVCOL_RANGE_NB as opposed to AVCOL_RANGE_NB + 1? IIUC, AVCOL_RANGE_NB is equivalent to your FORMAT_MAX and not actually a valid value
On 2013/10/29 00:36:30, scherkus wrote: > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > File media/ffmpeg/ffmpeg_common.cc (right): > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > media/ffmpeg/ffmpeg_common.cc:376: AVCOL_RANGE_NB + 1); > should this be AVCOL_RANGE_NB as opposed to AVCOL_RANGE_NB + 1? > > IIUC, AVCOL_RANGE_NB is equivalent to your FORMAT_MAX and not actually a valid > value Makes sense, I also renamed FORMAT_MAX to NUM_FORMATS because that describes it better and better matches conventions elsewhere.
On 2013/10/29 00:52:07, rileya wrote: > On 2013/10/29 00:36:30, scherkus wrote: > > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > > File media/ffmpeg/ffmpeg_common.cc (right): > > > > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > > media/ffmpeg/ffmpeg_common.cc:376: AVCOL_RANGE_NB + 1); > > should this be AVCOL_RANGE_NB as opposed to AVCOL_RANGE_NB + 1? > > > > IIUC, AVCOL_RANGE_NB is equivalent to your FORMAT_MAX and not actually a valid > > value > > Makes sense, I also renamed FORMAT_MAX to NUM_FORMATS because that describes it > better and better matches conventions elsewhere. Where did you see that? I believe media code has more FOO_MAX instead of NUM_FOOS. This isn't perfect grep but it's close: $ git grep "_MAX\b" -- *media/*h content/common/gpu/media/vaapi_h264_decoder.h: VAVDA_H264_DECODER_FAILURES_MAX, media/base/channel_layout.h: CHANNEL_LAYOUT_MAX // Must always be last! media/base/channel_layout.h: CHANNELS_MAX media/base/channel_layout.h:// from 0 to CHANNELS_MAX - 1. media/base/container_names.h:// done at the end of the list (before CONTAINER_MAX). This list must be kept in media/base/container_names.h: CONTAINER_MAX // Must be last media/base/pipeline_status.h: PIPELINE_STATUS_MAX, // Must be greater than all other values logged. media/base/video_decoder_config.h: H264PROFILE_MAX = H264PROFILE_MULTIVIEWHIGH, media/base/video_decoder_config.h: VP8PROFILE_MAX = VP8PROFILE_MAIN, media/base/video_decoder_config.h: VP9PROFILE_MAX = VP9PROFILE_MAIN, media/base/video_decoder_config.h: VIDEO_CODEC_PROFILE_MAX = VP9PROFILE_MAX, $ git grep "\bNUM_" -- *media/*h content/browser/renderer_host/media/media_stream_manager.h: int active_enumeration_ref_count_[NUM_MEDIA_TYPES]; content/common/media/media_stream_messages.h: content::NUM_MEDIA_TYPES - 1) content/common/media/media_stream_messages.h: content::NUM_MEDIA_VIDEO_FACING_MODE - 1) media/audio/win/audio_unified_win.h: NUM_FRAMES_IN_FIFO, media/base/demuxer_stream.h: NUM_TYPES, // Always keep this entry as the last one! If these enums were in the global namespace we'd also be forced to go with FOO_MAX because enums in the global namespace share a common prefix e.g., ChannelLayout https://code.google.com/p/chromium/codesearch#chromium/src/media/base/channel...
On 2013/10/29 01:00:23, scherkus wrote: > On 2013/10/29 00:52:07, rileya wrote: > > On 2013/10/29 00:36:30, scherkus wrote: > > > > > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > > > File media/ffmpeg/ffmpeg_common.cc (right): > > > > > > > > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > > > media/ffmpeg/ffmpeg_common.cc:376: AVCOL_RANGE_NB + 1); > > > should this be AVCOL_RANGE_NB as opposed to AVCOL_RANGE_NB + 1? > > > > > > IIUC, AVCOL_RANGE_NB is equivalent to your FORMAT_MAX and not actually a > valid > > > value > > > > Makes sense, I also renamed FORMAT_MAX to NUM_FORMATS because that describes > it > > better and better matches conventions elsewhere. > > Where did you see that? I believe media code has more FOO_MAX instead of > NUM_FOOS. This isn't perfect grep but it's close: > > $ git grep "_MAX\b" -- *media/*h > content/common/gpu/media/vaapi_h264_decoder.h: > VAVDA_H264_DECODER_FAILURES_MAX, > media/base/channel_layout.h: CHANNEL_LAYOUT_MAX // Must always be last! > media/base/channel_layout.h: CHANNELS_MAX > media/base/channel_layout.h:// from 0 to CHANNELS_MAX - 1. > media/base/container_names.h:// done at the end of the list (before > CONTAINER_MAX). This list must be kept in > media/base/container_names.h: CONTAINER_MAX // Must be last > media/base/pipeline_status.h: PIPELINE_STATUS_MAX, // Must be greater than all > other values logged. > media/base/video_decoder_config.h: H264PROFILE_MAX = H264PROFILE_MULTIVIEWHIGH, > media/base/video_decoder_config.h: VP8PROFILE_MAX = VP8PROFILE_MAIN, > media/base/video_decoder_config.h: VP9PROFILE_MAX = VP9PROFILE_MAIN, > media/base/video_decoder_config.h: VIDEO_CODEC_PROFILE_MAX = VP9PROFILE_MAX, > > $ git grep "\bNUM_" -- *media/*h > content/browser/renderer_host/media/media_stream_manager.h: int > active_enumeration_ref_count_[NUM_MEDIA_TYPES]; > content/common/media/media_stream_messages.h: > content::NUM_MEDIA_TYPES - 1) > content/common/media/media_stream_messages.h: > content::NUM_MEDIA_VIDEO_FACING_MODE - 1) > media/audio/win/audio_unified_win.h: NUM_FRAMES_IN_FIFO, > media/base/demuxer_stream.h: NUM_TYPES, // Always keep this entry as the > last one! > > > If these enums were in the global namespace we'd also be forced to go with > FOO_MAX because enums in the global namespace share a common prefix e.g., > ChannelLayout > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/channel... It seemed (looking at kVideoCodecMax and VIDEO_CODEC_PROFILE_MAX) that FOO_MAX implied FOO_MAX = LAST_VALID_FOO, whereas NUM_FOO (as in the ffmpeg color range enum) implied NUM_FOO = LAST_VALID_FOO + 1, so since I was making it the max valid format + 1 I corrected to NUM_FORMATS, but it appears that in various places FOO_MAX can be either LAST_VALID_FOO, or LAST_VALID_FOO + 1, so I guess I wasn't incorrect initially.
On 2013/10/29 01:19:41, rileya wrote: > On 2013/10/29 01:00:23, scherkus wrote: > > On 2013/10/29 00:52:07, rileya wrote: > > > On 2013/10/29 00:36:30, scherkus wrote: > > > > > > > > > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > > > > File media/ffmpeg/ffmpeg_common.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > > > > media/ffmpeg/ffmpeg_common.cc:376: AVCOL_RANGE_NB + 1); > > > > should this be AVCOL_RANGE_NB as opposed to AVCOL_RANGE_NB + 1? > > > > > > > > IIUC, AVCOL_RANGE_NB is equivalent to your FORMAT_MAX and not actually a > > valid > > > > value > > > > > > Makes sense, I also renamed FORMAT_MAX to NUM_FORMATS because that describes > > it > > > better and better matches conventions elsewhere. > > > > Where did you see that? I believe media code has more FOO_MAX instead of > > NUM_FOOS. This isn't perfect grep but it's close: > > > > $ git grep "_MAX\b" -- *media/*h > > content/common/gpu/media/vaapi_h264_decoder.h: > > VAVDA_H264_DECODER_FAILURES_MAX, > > media/base/channel_layout.h: CHANNEL_LAYOUT_MAX // Must always be last! > > media/base/channel_layout.h: CHANNELS_MAX > > media/base/channel_layout.h:// from 0 to CHANNELS_MAX - 1. > > media/base/container_names.h:// done at the end of the list (before > > CONTAINER_MAX). This list must be kept in > > media/base/container_names.h: CONTAINER_MAX // Must be last > > media/base/pipeline_status.h: PIPELINE_STATUS_MAX, // Must be greater than > all > > other values logged. > > media/base/video_decoder_config.h: H264PROFILE_MAX = > H264PROFILE_MULTIVIEWHIGH, > > media/base/video_decoder_config.h: VP8PROFILE_MAX = VP8PROFILE_MAIN, > > media/base/video_decoder_config.h: VP9PROFILE_MAX = VP9PROFILE_MAIN, > > media/base/video_decoder_config.h: VIDEO_CODEC_PROFILE_MAX = VP9PROFILE_MAX, > > > > $ git grep "\bNUM_" -- *media/*h > > content/browser/renderer_host/media/media_stream_manager.h: int > > active_enumeration_ref_count_[NUM_MEDIA_TYPES]; > > content/common/media/media_stream_messages.h: > > content::NUM_MEDIA_TYPES - 1) > > content/common/media/media_stream_messages.h: > > content::NUM_MEDIA_VIDEO_FACING_MODE - 1) > > media/audio/win/audio_unified_win.h: NUM_FRAMES_IN_FIFO, > > media/base/demuxer_stream.h: NUM_TYPES, // Always keep this entry as the > > last one! > > > > > > If these enums were in the global namespace we'd also be forced to go with > > FOO_MAX because enums in the global namespace share a common prefix e.g., > > ChannelLayout > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/channel... > > It seemed (looking at kVideoCodecMax and VIDEO_CODEC_PROFILE_MAX) that FOO_MAX > implied FOO_MAX = LAST_VALID_FOO, whereas NUM_FOO (as in the ffmpeg color range > enum) implied NUM_FOO = LAST_VALID_FOO + 1, so since I was making it the max > valid format + 1 I corrected to NUM_FORMATS, but it appears that in various > places FOO_MAX can be either LAST_VALID_FOO, or LAST_VALID_FOO + 1, so I guess I > wasn't incorrect initially. Perhaps we'll have to clean up some of the other enums. The issue with FOO_MAX = LAST_VALID_FOO is that it requires UMA histogramming code to remember to +1 the value. For example kVideoCodecMax has one use and +1's it: https://code.google.com/p/chromium/codesearch#search/&q=kVideoCodecMax&sq=pac... VIDEO_CODEC_PROFILE_MAX is used in two locations, one of which forgets to +1 it: https://code.google.com/p/chromium/codesearch#search/&q=VIDEO_CODEC_PROFILE_M... I guess I always read _MAX as being the +1 value, but I can see how it can be misleading. Want to email videostack-eng@ soliciting some more feedback between the two?
On 2013/10/29 02:29:14, scherkus wrote: > On 2013/10/29 01:19:41, rileya wrote: > > On 2013/10/29 01:00:23, scherkus wrote: > > > On 2013/10/29 00:52:07, rileya wrote: > > > > On 2013/10/29 00:36:30, scherkus wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > > > > > File media/ffmpeg/ffmpeg_common.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/45053003/diff/90001/media/ffmpeg/ffmpeg_commo... > > > > > media/ffmpeg/ffmpeg_common.cc:376: AVCOL_RANGE_NB + 1); > > > > > should this be AVCOL_RANGE_NB as opposed to AVCOL_RANGE_NB + 1? > > > > > > > > > > IIUC, AVCOL_RANGE_NB is equivalent to your FORMAT_MAX and not actually a > > > valid > > > > > value > > > > > > > > Makes sense, I also renamed FORMAT_MAX to NUM_FORMATS because that > describes > > > it > > > > better and better matches conventions elsewhere. > > > > > > Where did you see that? I believe media code has more FOO_MAX instead of > > > NUM_FOOS. This isn't perfect grep but it's close: > > > > > > $ git grep "_MAX\b" -- *media/*h > > > content/common/gpu/media/vaapi_h264_decoder.h: > > > VAVDA_H264_DECODER_FAILURES_MAX, > > > media/base/channel_layout.h: CHANNEL_LAYOUT_MAX // Must always be last! > > > media/base/channel_layout.h: CHANNELS_MAX > > > media/base/channel_layout.h:// from 0 to CHANNELS_MAX - 1. > > > media/base/container_names.h:// done at the end of the list (before > > > CONTAINER_MAX). This list must be kept in > > > media/base/container_names.h: CONTAINER_MAX // Must be last > > > media/base/pipeline_status.h: PIPELINE_STATUS_MAX, // Must be greater than > > all > > > other values logged. > > > media/base/video_decoder_config.h: H264PROFILE_MAX = > > H264PROFILE_MULTIVIEWHIGH, > > > media/base/video_decoder_config.h: VP8PROFILE_MAX = VP8PROFILE_MAIN, > > > media/base/video_decoder_config.h: VP9PROFILE_MAX = VP9PROFILE_MAIN, > > > media/base/video_decoder_config.h: VIDEO_CODEC_PROFILE_MAX = > VP9PROFILE_MAX, > > > > > > $ git grep "\bNUM_" -- *media/*h > > > content/browser/renderer_host/media/media_stream_manager.h: int > > > active_enumeration_ref_count_[NUM_MEDIA_TYPES]; > > > content/common/media/media_stream_messages.h: > > > content::NUM_MEDIA_TYPES - 1) > > > content/common/media/media_stream_messages.h: > > > content::NUM_MEDIA_VIDEO_FACING_MODE - 1) > > > media/audio/win/audio_unified_win.h: NUM_FRAMES_IN_FIFO, > > > media/base/demuxer_stream.h: NUM_TYPES, // Always keep this entry as the > > > last one! > > > > > > > > > If these enums were in the global namespace we'd also be forced to go with > > > FOO_MAX because enums in the global namespace share a common prefix e.g., > > > ChannelLayout > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/channel... > > > > It seemed (looking at kVideoCodecMax and VIDEO_CODEC_PROFILE_MAX) that FOO_MAX > > implied FOO_MAX = LAST_VALID_FOO, whereas NUM_FOO (as in the ffmpeg color > range > > enum) implied NUM_FOO = LAST_VALID_FOO + 1, so since I was making it the max > > valid format + 1 I corrected to NUM_FORMATS, but it appears that in various > > places FOO_MAX can be either LAST_VALID_FOO, or LAST_VALID_FOO + 1, so I guess > I > > wasn't incorrect initially. > > Perhaps we'll have to clean up some of the other enums. The issue with FOO_MAX = > LAST_VALID_FOO is that it requires UMA histogramming code to remember to +1 the > value. > > For example kVideoCodecMax has one use and +1's it: > https://code.google.com/p/chromium/codesearch#search/&q=kVideoCodecMax&sq=pac... > > VIDEO_CODEC_PROFILE_MAX is used in two locations, one of which forgets to +1 it: > https://code.google.com/p/chromium/codesearch#search/&q=VIDEO_CODEC_PROFILE_M... > > I guess I always read _MAX as being the +1 value, but I can see how it can be > misleading. Want to email videostack-eng@ soliciting some more feedback between > the two? Alright, I sent an email to the mailing list.
https://codereview.chromium.org/45053003/diff/170001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/45053003/diff/170001/media/base/video_frame.h... media/base/video_frame.h:55: NUM_FORMATS, // Must be greater than all other formats based on discussion ... want to use some sort of histogrammy name that's equal to YV12A + 1?
On 2013/10/29 23:39:10, scherkus wrote: > https://codereview.chromium.org/45053003/diff/170001/media/base/video_frame.h > File media/base/video_frame.h (right): > > https://codereview.chromium.org/45053003/diff/170001/media/base/video_frame.h... > media/base/video_frame.h:55: NUM_FORMATS, // Must be greater than all other > formats > based on discussion ... want to use some sort of histogrammy name that's equal > to YV12A + 1? Alright, I used FORMAT_HISTOGRAM_MAX.
one nit on naming https://codereview.chromium.org/45053003/diff/290001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/45053003/diff/290001/media/base/video_frame.h... media/base/video_frame.h:55: FORMAT_HISTOGRAM_MAX, // Must be greater than all other formats. since we don't prefix the other enums here with FORMAT_ (because they're already prefixed via VideoFrame::) we can use "HISTOGRAM_MAX"
On 2013/10/30 19:37:05, scherkus wrote: > one nit on naming > > https://codereview.chromium.org/45053003/diff/290001/media/base/video_frame.h > File media/base/video_frame.h (right): > > https://codereview.chromium.org/45053003/diff/290001/media/base/video_frame.h... > media/base/video_frame.h:55: FORMAT_HISTOGRAM_MAX, // Must be greater than all > other formats. > since we don't prefix the other enums here with FORMAT_ (because they're already > prefixed via VideoFrame::) we can use "HISTOGRAM_MAX" What about the case where we have several enums in one class which want to have a HISTOGRAM_MAX value? The other enums in VideoFrame happen to be in k* format, so this isn't an issue here, but I could see collisions possibly being a problem elsewhere...
On 2013/10/30 19:45:18, rileya wrote: > On 2013/10/30 19:37:05, scherkus wrote: > > one nit on naming > > > > https://codereview.chromium.org/45053003/diff/290001/media/base/video_frame.h > > File media/base/video_frame.h (right): > > > > > https://codereview.chromium.org/45053003/diff/290001/media/base/video_frame.h... > > media/base/video_frame.h:55: FORMAT_HISTOGRAM_MAX, // Must be greater than > all > > other formats. > > since we don't prefix the other enums here with FORMAT_ (because they're > already > > prefixed via VideoFrame::) we can use "HISTOGRAM_MAX" > > What about the case where we have several enums in one class which want to have > a HISTOGRAM_MAX value? The other enums in VideoFrame happen to be in k* format, > so this isn't an issue here, but I could see collisions possibly being a problem > elsewhere... If that happens it'd be a strong signal that we should either a) change the naming and/or b) move enums into their own scope (see other HISTOGRAM_MAX enums in chrome [1]), but if it's not a problem today for VideoFrame then we shouldn't stick FORMAT_ in front just-in-case. [1] https://code.google.com/p/chromium/codesearch#search/&q=HISTOGRAM_MAX&sq=pack...
On 2013/10/30 19:55:04, scherkus wrote: > On 2013/10/30 19:45:18, rileya wrote: > > On 2013/10/30 19:37:05, scherkus wrote: > > > one nit on naming > > > > > > > https://codereview.chromium.org/45053003/diff/290001/media/base/video_frame.h > > > File media/base/video_frame.h (right): > > > > > > > > > https://codereview.chromium.org/45053003/diff/290001/media/base/video_frame.h... > > > media/base/video_frame.h:55: FORMAT_HISTOGRAM_MAX, // Must be greater than > > all > > > other formats. > > > since we don't prefix the other enums here with FORMAT_ (because they're > > already > > > prefixed via VideoFrame::) we can use "HISTOGRAM_MAX" > > > > What about the case where we have several enums in one class which want to > have > > a HISTOGRAM_MAX value? The other enums in VideoFrame happen to be in k* > format, > > so this isn't an issue here, but I could see collisions possibly being a > problem > > elsewhere... > > If that happens it'd be a strong signal that we should either a) change the > naming and/or b) move enums into their own scope (see other HISTOGRAM_MAX enums > in chrome [1]), but if it's not a problem today for VideoFrame then we shouldn't > stick FORMAT_ in front just-in-case. > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=HISTOGRAM_MAX&sq=pack... Sounds good, changed to HISTOGRAM_MAX.
lgtm you'll need to add someone from cc/OWNERS and tools/metrics/OWNERS for the respective changes
Added OWNERS reviewers for histogram additions and minor cc/ changes.
On 2013/10/31 00:17:03, rileya wrote: > Added OWNERS reviewers for histogram additions and minor cc/ changes. mpearson@, please take a look at: tools/metrics/histograms/histograms.xml danakj@, please take a look at: cc/resources/video_resource_updater.cc Thanks!
https://codereview.chromium.org/45053003/diff/410001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/45053003/diff/410001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6245: + <summary>Pixel format color range of HTML5 video.</summary> Here and below: Histogram descriptions should clearly state when the histogram is emitted (video load? video playback? etc.) https://codereview.chromium.org/45053003/diff/410001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:28139: + <int value="14" label="YV12A"/> I see the underlying enum has gaps. Is there a good reason for this? Why not change it to be gapless, otherwise we're allocating histogram buckets that clearly never get used.
https://codereview.chromium.org/45053003/diff/410001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/45053003/diff/410001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6245: + <summary>Pixel format color range of HTML5 video.</summary> On 2013/10/31 10:33:10, Mark P wrote: > Here and below: Histogram descriptions should clearly state when the histogram > is emitted (video load? video playback? etc.) Alright, added a note about this. https://codereview.chromium.org/45053003/diff/410001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:28139: + <int value="14" label="YV12A"/> On 2013/10/31 10:33:10, Mark P wrote: > I see the underlying enum has gaps. Is there a good reason for this? Why not > change it to be gapless, otherwise we're allocating histogram buckets that > clearly never get used. Looking at past revisions, it looks like the gaps came to be due to needing to mirror an enum in WebKit (WebVideoFrame::Format), from the looks of it this isn't necessary anymore, since that enum has since been removed in Blink... I'll ask around and see if there's any reason not to remove the gaps.
https://codereview.chromium.org/45053003/diff/410001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/45053003/diff/410001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:28139: + <int value="14" label="YV12A"/> On 2013/10/31 17:54:00, rileya wrote: > On 2013/10/31 10:33:10, Mark P wrote: > > I see the underlying enum has gaps. Is there a good reason for this? Why not > > change it to be gapless, otherwise we're allocating histogram buckets that > > clearly never get used. > > Looking at past revisions, it looks like the gaps came to be due to needing to > mirror an enum in WebKit (WebVideoFrame::Format), from the looks of it this > isn't necessary anymore, since that enum has since been removed in Blink... I'll > ask around and see if there's any reason not to remove the gaps. Looks like it's safe to remove the gaps, the newest patch set removes them.
hmm... we should do some extra VideoFrame::Format cleanup prior to UMA'ing them,.. but that shouldn't block the colour format UMA. Let's do the following: 1) Have this CL land the FFmpeg colour ranges histogram since that's what we're really interested in for YUVJ420P support 2) Tackle removing unnecessary VideoFrame::Format enums https://code.google.com/p/chromium/issues/detail?id=313827 3) Add in a VideoFrame::Format UMA
Alright, removed VideoFrame::Format changes and UMA-ing, I'll put out some CLs to clean that up before we start logging it. Also, -danakj@, since this no longer needs a cc/ OWNERS review.
lgtm sorry for all the thrashing on this CL!
in this shortened change, histograms.xml lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/45053003/640001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/45053003/640001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/45053003/640001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/45053003/640001
Failed to apply patch for media/ffmpeg/ffmpeg_common.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file media/ffmpeg/ffmpeg_common.cc
Hunk #1 FAILED at 6.
Hunk #2 succeeded at 382 (offset 13 lines).
1 out of 2 hunks FAILED -- saving rejects to file
media/ffmpeg/ffmpeg_common.cc.rej
Patch: media/ffmpeg/ffmpeg_common.cc
Index: media/ffmpeg/ffmpeg_common.cc
diff --git a/media/ffmpeg/ffmpeg_common.cc b/media/ffmpeg/ffmpeg_common.cc
index
72b31252f8508df48cc10c7d2d832264525233cd..7f3411789916cb4b58d3ce491ce2d97c9314d27a
100644
--- a/media/ffmpeg/ffmpeg_common.cc
+++ b/media/ffmpeg/ffmpeg_common.cc
@@ -6,6 +6,7 @@
#include "base/basictypes.h"
#include "base/logging.h"
+#include "base/metrics/histogram.h"
#include "media/base/decoder_buffer.h"
#include "media/base/video_frame.h"
#include "media/base/video_util.h"
@@ -369,6 +370,12 @@ void AVStreamToVideoDecoderConfig(
gfx::Size natural_size = GetNaturalSize(
visible_rect.size(), aspect_ratio.num, aspect_ratio.den);
+ if (record_stats) {
+ UMA_HISTOGRAM_ENUMERATION("Media.VideoColorRange",
+ stream->codec->color_range,
+ AVCOL_RANGE_NB);
+ }
+
VideoFrame::Format format = PixelFormatToVideoFormat(stream->codec->pix_fmt);
if (codec == kCodecVP9) {
// TODO(tomfinegan): libavcodec doesn't know about VP9.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/45053003/970001
Failed to apply patch for media/ffmpeg/ffmpeg_common.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file media/ffmpeg/ffmpeg_common.cc
Hunk #1 FAILED at 6.
Hunk #2 succeeded at 297 with fuzz 1 (offset 13 lines).
Hunk #3 FAILED at 304.
Hunk #4 succeeded at 394 (offset 13 lines).
Hunk #5 FAILED at 518.
3 out of 5 hunks FAILED -- saving rejects to file
media/ffmpeg/ffmpeg_common.cc.rej
Patch: media/ffmpeg/ffmpeg_common.cc
Index: media/ffmpeg/ffmpeg_common.cc
diff --git a/media/ffmpeg/ffmpeg_common.cc b/media/ffmpeg/ffmpeg_common.cc
index
72b31252f8508df48cc10c7d2d832264525233cd..1a45134c8376c575dc0cf18101bd88b60494cdae
100644
--- a/media/ffmpeg/ffmpeg_common.cc
+++ b/media/ffmpeg/ffmpeg_common.cc
@@ -6,6 +6,8 @@
#include "base/basictypes.h"
#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
#include "media/base/decoder_buffer.h"
#include "media/base/video_frame.h"
#include "media/base/video_util.h"
@@ -284,6 +286,18 @@ static void AVCodecContextToAudioDecoderConfig(
sample_format = kSampleFormatS16;
}
+ base::TimeDelta seek_preroll;
+ if (codec_context->seek_preroll > 0) {
+ seek_preroll = base::TimeDelta::FromMicroseconds(
+ codec_context->seek_preroll * 1000000.0 / codec_context->sample_rate);
+ }
+
+ base::TimeDelta codec_delay;
+ if (codec_context->delay > 0) {
+ codec_delay = base::TimeDelta::FromMicroseconds(
+ codec_context->delay * 1000000.0 / codec_context->sample_rate);
+ }
+
config->Initialize(codec,
sample_format,
channel_layout,
@@ -292,8 +306,8 @@ static void AVCodecContextToAudioDecoderConfig(
codec_context->extradata_size,
is_encrypted,
record_stats,
- base::TimeDelta(),
- base::TimeDelta());
+ seek_preroll,
+ codec_delay);
if (codec != kCodecOpus) {
DCHECK_EQ(av_get_bytes_per_sample(codec_context->sample_fmt) * 8,
config->bits_per_channel());
@@ -369,6 +383,12 @@ void AVStreamToVideoDecoderConfig(
gfx::Size natural_size = GetNaturalSize(
visible_rect.size(), aspect_ratio.num, aspect_ratio.den);
+ if (record_stats) {
+ UMA_HISTOGRAM_ENUMERATION("Media.VideoColorRange",
+ stream->codec->color_range,
+ AVCOL_RANGE_NB);
+ }
+
VideoFrame::Format format = PixelFormatToVideoFormat(stream->codec->pix_fmt);
if (codec == kCodecVP9) {
// TODO(tomfinegan): libavcodec doesn't know about VP9.
@@ -500,7 +520,7 @@ VideoFrame::Format PixelFormatToVideoFormat(PixelFormat
pixel_format) {
default:
DVLOG(1) << "Unsupported PixelFormat: " << pixel_format;
}
- return VideoFrame::INVALID;
+ return VideoFrame::UNKNOWN;
}
PixelFormat VideoFormatToPixelFormat(VideoFrame::Format video_format) {
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/45053003/1020001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/45053003/1020001
Message was sent while issue was closed.
Change committed as 232875 |
