Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1117853002:
vea_unittest: Calculate per-frame encode latency (Closed)
|
Created:
5 years, 7 months ago by Justin Chuang Modified:
5 years, 7 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionvea_unittest: Calculate per-frame encode latency
Calculates per-frame encode latency, which is basically the time delay from
input of each VideoFrame (VEA::Encode()) to output of the corresponding
BitstreamBuffer (VEA::Client::BitstreamBufferReady()).
It calculates encode latency values at the 50th(median), 75th, and 95th percentiles.
Also fix a wrong comment.
BUG=345181
TEST=Check "Encode latency for the 50%/75%/95%" in output logs.
Committed: https://crrev.com/b21ffeee2aff4da8edb9808b2227165c37e5ce86
Cr-Commit-Position: refs/heads/master@{#329819}
Committed: https://crrev.com/738487a69f150975376c36e214c64a77b1ff0cc6
Cr-Commit-Position: refs/heads/master@{#329838}
Patch Set 1 #Patch Set 2 : cl format #
Total comments: 1
Patch Set 3 : Calculate 50%/75%/95% values #
Total comments: 25
Patch Set 4 : Address Owen's comments #
Total comments: 17
Patch Set 5 : Address WuCheng and Owen's comments #
Total comments: 41
Patch Set 6 : Address Wucheng's comments #Patch Set 7 : Address Pawel's comments #
Total comments: 21
Patch Set 8 : Rebase only #Patch Set 9 : Address Pawel's comments #
Total comments: 2
Patch Set 10 : Change to option 2 #
Total comments: 12
Patch Set 11 : Address Pawel's comments and fix const type #Patch Set 12 : Refactor VEAClient::run_at_fps_ #Patch Set 13 : Added a CHECK() #
Total comments: 2
Patch Set 14 : Rename utility methods #Patch Set 15 : Rebase #Patch Set 16 : Fix compile error on x86/daisy #
Total comments: 1
Messages
Total messages: 47 (7 generated)
jchuang@chromium.org changed reviewers: + owenlin@chromium.org, posciak@chromium.org, wuchengli@chromium.org
PTAL. Thanks
https://codereview.chromium.org/1117853002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (left): https://codereview.chromium.org/1117853002/diff/20001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:646: // num_encoded_frames_). The original comments is wrong. So remove it. (It actually starts at 0)
https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:573: // Returns the number of encoded frames per second. why Return"s"? Most comments in this file use "Return". I think we should just follow it. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:576: // Returns a list of encode latency of the frames at the percentages in Why make it public? it is only used in VEAClient. In fact, I would suggest just have a helper function to calculate the percentile. static float percentile( const vector<float>& sorted_values, float percentage); https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:634: std::pair<scoped_refptr<media::VideoFrame>, int32> PrepareInputFrame( We already has the next_input_id_ using around the code. How about just using it instead of changing the interface? https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:942: size_t index = base::checked_cast<size_t>(ratio * (size - 1)); Can we either implement the Nearest Rank method or the Linear Interpolation Between Closest Ranks method ? http://en.wikipedia.org/wiki/Percentile https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1139: auto ret = PrepareInputFrame(pos_in_input_stream_); we can get the input id by: int32 input_id = next_input_id_; https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1232: std::vector<float> ratios; Define them as constant, e.g. static float kLoggedLatencyPercentages[] = {0.50, 0.75, 0.95}; https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1240: LOG(INFO) << "Encode latency at " << ratio_str << ": " Why using StringPrintf? The following code won't work? LOG(INFO) << "Encode latency at " << static_cast<int>(ratios[i] * 100) << "%: " .....
Owen, thanks for the comments. Please check the replies, then I'll amend accordingly. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:573: // Returns the number of encoded frames per second. On 2015/05/04 09:37:12, Owen Lin wrote: > why Return"s"? Most comments in this file use "Return". I think we should just > follow it. OK. The correct way should be "returns" for first person comments in docstring, but "return" in other comment for code block. (it's defined somewhere in coding convention, and I was corrected by others before) But consistency is more important than rules. Will follow it. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:576: // Returns a list of encode latency of the frames at the percentages in On 2015/05/04 09:37:12, Owen Lin wrote: > Why make it public? it is only used in VEAClient. > I put this close to frames_per_second(). Or maybe I should move both methods to private? > In fact, I would suggest just have a helper function to calculate the > percentile. > > static float percentile( > const vector<float>& sorted_values, float percentage); Sounds good. Will do. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:634: std::pair<scoped_refptr<media::VideoFrame>, int32> PrepareInputFrame( On 2015/05/04 09:37:12, Owen Lin wrote: > We already has the next_input_id_ using around the code. How about just using it > instead of changing the interface? But the value is incremented by 1 than the value I need after the function call. I can pick the next_input_id_ value *before* calling PrepareInputFrame, or decrement the value by one *after* calling it, etc. IMO either way lacks encapsulation (why should the caller know this function controls next_input_id_?). We can argue variable is in the context of this class, but this class is big. Returning the input id of the frame to be encoded doesn't seem too terrible compared to alternative methods. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:942: size_t index = base::checked_cast<size_t>(ratio * (size - 1)); On 2015/05/04 09:37:12, Owen Lin wrote: > Can we either implement the Nearest Rank method or the Linear Interpolation > Between Closest Ranks method ? > > http://en.wikipedia.org/wiki/Percentile Yeah... I thought about this. But I think the real value of outlier is more useful than the interpolated value on the dashboard. How do you think? I'll change to interpolation if you feel it's a better measurement. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1139: auto ret = PrepareInputFrame(pos_in_input_stream_); On 2015/05/04 09:37:12, Owen Lin wrote: > we can get the input id by: > > int32 input_id = next_input_id_; I don't like this. See above. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1232: std::vector<float> ratios; On 2015/05/04 09:37:12, Owen Lin wrote: > Define them as constant, e.g. > > static float kLoggedLatencyPercentages[] = {0.50, 0.75, 0.95}; Will do. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1240: LOG(INFO) << "Encode latency at " << ratio_str << ": " On 2015/05/04 09:37:12, Owen Lin wrote: > Why using StringPrintf? The following code won't work? > > LOG(INFO) << "Encode latency at " > << static_cast<int>(ratios[i] * 100) << "%: " > ..... > This string used in two places.
https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:576: // Returns a list of encode latency of the frames at the percentages in On 2015/05/04 10:25:09, Justin Chuang wrote: > On 2015/05/04 09:37:12, Owen Lin wrote: > > Why make it public? it is only used in VEAClient. > > > I put this close to frames_per_second(). Or maybe I should move both methods to > private? > > > In fact, I would suggest just have a helper function to calculate the > > percentile. > > > > static float percentile( > > const vector<float>& sorted_values, float percentage); > > Sounds good. Will do. I see. It sounds we should move frames_per_second() to private as well. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:634: std::pair<scoped_refptr<media::VideoFrame>, int32> PrepareInputFrame( On 2015/05/04 10:25:09, Justin Chuang wrote: > On 2015/05/04 09:37:12, Owen Lin wrote: > > We already has the next_input_id_ using around the code. How about just using > it > > instead of changing the interface? > > But the value is incremented by 1 than the value I need after the function call. > I can pick the next_input_id_ value *before* calling PrepareInputFrame, or > decrement the value by one *after* calling it, etc. > IMO either way lacks encapsulation (why should the caller know this function > controls next_input_id_?). We can argue variable is in the context of this > class, but this class is big. > Returning the input id of the frame to be encoded doesn't seem too terrible > compared to alternative methods. I got your point. See my reply below. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:942: size_t index = base::checked_cast<size_t>(ratio * (size - 1)); On 2015/05/04 10:25:09, Justin Chuang wrote: > On 2015/05/04 09:37:12, Owen Lin wrote: > > Can we either implement the Nearest Rank method or the Linear Interpolation > > Between Closest Ranks method ? > > > > http://en.wikipedia.org/wiki/Percentile > > Yeah... I thought about this. But I think the real value of outlier is more > useful than the interpolated value on the dashboard. > > How do you think? I'll change to interpolation if you feel it's a better > measurement. First, it won't do extrapolation for outliers (for p near 0 or 1). The percentile value is always between two real values. But I am also fine with the nearest rank method. It seems the used formula doesn't match. I thought it should be index = std::max(base::checked_cast<size_t>(ratio * size), size - 1) https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1134: if (keyframe_period_ && next_input_id_ % keyframe_period_ == 0) { We already used "next_input_id_" in the same function. I think it would be good to make them consist. Besides, I would prefer got the returned value from the argument instead of returned a pair. At least we can have a name for the argument. So, what about int32 input_id; scoped_refptr<media::VideoFrame> video_frame = PrepareInputFrame(pos_in_input_stream_, &input_id); and set force_keyframe by using input_id and then call Encode()? https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1240: LOG(INFO) << "Encode latency at " << ratio_str << ": " On 2015/05/04 10:25:09, Justin Chuang wrote: > On 2015/05/04 09:37:12, Owen Lin wrote: > > Why using StringPrintf? The following code won't work? > > > > LOG(INFO) << "Encode latency at " > > << static_cast<int>(ratios[i] * 100) << "%: " > > ..... > > > > This string used in two places. OK, it's fine. But I think you just need the percentage. int percentage = static_cast<int>(ratios[i] * 100 + .5); Then you can use it at both places.
PTAL again. Thanks https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:573: // Returns the number of encoded frames per second. On 2015/05/04 10:25:09, Justin Chuang wrote: > On 2015/05/04 09:37:12, Owen Lin wrote: > > why Return"s"? Most comments in this file use "Return". I think we should just > > follow it. > > But consistency is more important than rules. Will follow it. Done. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:576: // Returns a list of encode latency of the frames at the percentages in On 2015/05/05 06:03:13, Owen Lin wrote: > On 2015/05/04 10:25:09, Justin Chuang wrote: > > > static float percentile( > > > const vector<float>& sorted_values, float percentage); > > > > Sounds good. Will do. > > I see. It sounds we should move frames_per_second() to private as well. Done. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:942: size_t index = base::checked_cast<size_t>(ratio * (size - 1)); On 2015/05/05 06:03:13, Owen Lin wrote: > On 2015/05/04 10:25:09, Justin Chuang wrote: > > On 2015/05/04 09:37:12, Owen Lin wrote: > > > Can we either implement the Nearest Rank method or the Linear Interpolation > > > Between Closest Ranks method ? > > > > > > http://en.wikipedia.org/wiki/Percentile > > > > Yeah... I thought about this. But I think the real value of outlier is more > > useful than the interpolated value on the dashboard. > > > > How do you think? I'll change to interpolation if you feel it's a better > > measurement. > > First, it won't do extrapolation for outliers (for p near 0 or 1). The > percentile value is always between two real values. > > But I am also fine with the nearest rank method. It seems the used formula > doesn't match. I thought it should be > > index = std::max(base::checked_cast<size_t>(ratio * size), size - 1) Changed to your formula (but s/max/min/). Thanks! https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1139: auto ret = PrepareInputFrame(pos_in_input_stream_); On 2015/05/04 10:25:09, Justin Chuang wrote: > On 2015/05/04 09:37:12, Owen Lin wrote: > > we can get the input id by: > > > > int32 input_id = next_input_id_; > > I don't like this. See above. Changed the prototype and remove reference to next_input_id_ in caller. Thx https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1232: std::vector<float> ratios; On 2015/05/04 10:25:09, Justin Chuang wrote: > On 2015/05/04 09:37:12, Owen Lin wrote: > > Define them as constant, e.g. > > > > static float kLoggedLatencyPercentages[] = {0.50, 0.75, 0.95}; > > Will do. Done. https://codereview.chromium.org/1117853002/diff/40001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1240: LOG(INFO) << "Encode latency at " << ratio_str << ": " On 2015/05/05 06:03:13, Owen Lin wrote: > On 2015/05/04 10:25:09, Justin Chuang wrote: > > This string used in two places. > OK, it's fine. But I think you just need the percentage. > > int percentage = static_cast<int>(ratios[i] * 100 + .5); > Then you can use it at both places. Done. Thanks. Nice tip. +0.5 is good, I added it, although normally the FP library won't have such issue.
https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:662: IdToTicks encode_start_time_per_frame_; s/encode_start_time_per_frame_/frame_encode_start_time_/. I think "per frame" is used when a value can be divided by different number of frames. Here the value start time is not divided. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1146: CHECK(encode_start_time_per_frame_.insert( Why do we need to CHECK the return value of insert? Doesn't it always return something? https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1228: void VEAClient::VerifyPerf() { s/VerifyPerf/LogPerf/ and move line 1234-1235 out of this function. Most of this function logs the performance result. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1239: for (size_t i = 0; i < sizeof(kLoggedLatencyPercentages) / sizeof(float); can use arraysize of base/macros.h https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1244: LOG(INFO) << "Encode latency at " << percentage Store the string in std::string and use it for both LOG and LogToFile. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1246: g_env->LogToFile( Add a comment saying the string have to match CrOS autotest.
https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:938: if (size == 0) { I think we should not return 0 when size == 0. How about just let the test crash: CHECK_NE(0, size). https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1146: CHECK(encode_start_time_per_frame_.insert( On 2015/05/07 05:39:09, wuchengli wrote: > Why do we need to CHECK the return value of insert? Doesn't it always return > something? second is true iff the key wasn't in the map. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1243: int percentage = static_cast<int>(percentage_f * 100 + .5); After a second thought, I think we should use round instead of + .5. int percentage = std::round(percentage_f * 100); In addition, static_cast is not needed to case from double to int. (It's fine to me if you would like to keep it.) http://www.cplusplus.com/doc/tutorial/typecasting/
https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1146: CHECK(encode_start_time_per_frame_.insert( On 2015/05/07 06:29:53, Owen Lin wrote: > On 2015/05/07 05:39:09, wuchengli wrote: > > Why do we need to CHECK the return value of insert? Doesn't it always return > > something? > > second is true iff the key wasn't in the map. I see. I didn't see .second. :)
https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:662: IdToTicks encode_start_time_per_frame_; On 2015/05/07 05:39:09, wuchengli wrote: > s/encode_start_time_per_frame_/frame_encode_start_time_/. I think "per frame" is > used when a value can be divided by different number of frames. Here the value > start time is not divided. Done. Use a slightly different name. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:938: if (size == 0) { On 2015/05/07 06:29:53, Owen Lin wrote: > I think we should not return 0 when size == 0. How about just let the test > crash: CHECK_NE(0, size). Done. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1228: void VEAClient::VerifyPerf() { On 2015/05/07 05:39:09, wuchengli wrote: > s/VerifyPerf/LogPerf/ and move line 1234-1235 out of this function. Most of this > function logs the performance result. Done. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1239: for (size_t i = 0; i < sizeof(kLoggedLatencyPercentages) / sizeof(float); On 2015/05/07 05:39:09, wuchengli wrote: > can use arraysize of base/macros.h Done. Thanks https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1243: int percentage = static_cast<int>(percentage_f * 100 + .5); On 2015/05/07 06:29:53, Owen Lin wrote: > After a second thought, I think we should use round instead of + .5. > > int percentage = std::round(percentage_f * 100); > > In addition, static_cast is not needed to case from double to int. > (It's fine to me if you would like to keep it.) > http://www.cplusplus.com/doc/tutorial/typecasting/ > I removed the typecast completely. No need to waste time on a risky casting. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1244: LOG(INFO) << "Encode latency at " << percentage On 2015/05/07 05:39:09, wuchengli wrote: > Store the string in std::string and use it for both LOG and LogToFile. I moved the LOG(INFO) to LogToFile(), saving a few dup lines. https://codereview.chromium.org/1117853002/diff/60001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1246: g_env->LogToFile( On 2015/05/07 05:39:09, wuchengli wrote: > Add a comment saying the string have to match CrOS autotest. Added the comment on the LogToFile() method. Thx
https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:394: // Note that Chrome OS Autotest is parsing the output key and value pairs. Be s/is parsing/parses/ https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:663: IdToTicks frame_encode_start_ticks_; Hmm. The name is not consistent with |encode_start_time_|, whose type is also TimeTicks. I still prefer |frame_encode_start_times_|. As we discussed, remove |frame_encode_start_ticks_| and make |encode_start_time_| a vector. There's no need to erase the element of |frame_encode_start_ticks_| in HandleEncodedFrame(). I'm not a fan of ticks in the name. "start time" is common and easier to understand for a name. There's no need to put the type into the name.
lgtm % one nit (and wucheng's) https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:79: static int32 kLoggedLatencyPercentages[] = {50, 75, 95}; Please change it back to using float. We have the issue because of data presentation. But 1. We should not change the code just because of the data presentation. 2. We don't need those "round" and "cast" things anymore. See below. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1238: base::StringPrintf("Encode latency at %d%%", percentage), We can just use base::StringPrintf("Encode latency at %.0f%%", percentage * 100),
https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:394: // Note that Chrome OS Autotest is parsing the output key and value pairs. Be On 2015/05/07 09:22:17, wuchengli wrote: > s/is parsing/parses/ Done. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:663: IdToTicks frame_encode_start_ticks_; On 2015/05/07 09:22:17, wuchengli wrote: > Hmm. The name is not consistent with |encode_start_time_|, whose type is also > TimeTicks. I still prefer |frame_encode_start_times_|. > > As we discussed, remove |frame_encode_start_ticks_| and make > |encode_start_time_| a vector. There's no need to erase the element of > |frame_encode_start_ticks_| in HandleEncodedFrame(). > > I'm not a fan of ticks in the name. "start time" is common and easier to > understand for a name. There's no need to put the type into the name. Done. Thanks. The code looks simpler.
https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:79: static int32 kLoggedLatencyPercentages[] = {50, 75, 95}; On 2015/05/07 09:53:45, Owen Lin wrote: > Please change it back to using float. We have the issue because of data > presentation. > > But > 1. We should not change the code just because of the data presentation. > 2. We don't need those "round" and "cast" things anymore. See below. Interesting. I'd prefer to keep the current code because 1. The array is added in this CL. So no harm to change its data format before commit. 2. Float => int conversion is not very comfortable although we have good helpers and modern compiler/lib support is good. int to float is more straightforward. Thoughts?
https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:79: static int32 kLoggedLatencyPercentages[] = {50, 75, 95}; s/Percentages/Percentiles/ Do we need to use int32 in particular? Just unsigned int should work? Also, as a side note, intXX are deprecated in favor of intXX_t. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:398: LOG(INFO) << s; Is this to make it convenient for humans running the test manually or to make this visible in logs on bots as well? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:661: typedef std::map<int32, base::TimeTicks> IdToTicks; s/Id/InputId/ https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:663: IdToTicks frame_encode_start_ticks_; Since we use input ids starting from 0 and incrementing by 1, a vector would be enough? You could access input_id frame via vector[input_id]? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:664: // The encode latencies of all encoded frames. We should describe how we define latency. Maybe ", i.e. time delta between calling VEA::Encode() on a frame and receiving it encoded via VEA::BitstreamBufferReady()" ? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:718: // If true, verify performance at the end of the test. We should probably call this something like ensure_min_fps_, since this is the only thing that it governs now. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:787: run_at_fps_(run_at_fps) { We should have a CHECK here that verifies that if run_at_fps_ is true, test_perf_ (or actually ensure_min_fps_) is not true. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:935: base::TimeDelta VEAClient::percentile( I think this doesn't have to be a member method? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:939: CHECK_NE(0, size); CHECK_GT(size, 0) ? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:944: std::min(base::checked_cast<size_t>(percentage * size), size - 1); Is this to guard ourselves from getting a percentage > 1 as an argument to this function? Would it be more explicit to say if (percentage >= 1) return sorted_values.back() ? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1173: // Record encode latency of this frame. We should CHECK that run_at_fps is true if we are going to measure encode latency. Otherwise we get skewed results, since we may queue many frames at once with the same encode start time. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1235: int32 percentage = kLoggedLatencyPercentages[i]; percentiles, also, no need for int32 specifically? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1238: base::StringPrintf("Encode latency at %d%%", percentage), Encode latency for %dth percentile:
https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:79: static int32 kLoggedLatencyPercentages[] = {50, 75, 95}; On 2015/05/07 11:31:50, Pawel Osciak wrote: > s/Percentages/Percentiles/ > > Do we need to use int32 in particular? Just unsigned int should work? Also, as a > side note, intXX are deprecated in favor of intXX_t. Done. also changed to uint32_t. Thx There are other occurrences of int32 in this file. Leave it for another CL. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:398: LOG(INFO) << s; On 2015/05/07 11:31:51, Pawel Osciak wrote: > Is this to make it convenient for humans running the test manually or to make > this visible in logs on bots as well? for human running the test manually. (even when --output_log is not used) Prior to this CL, another metric using LogToFile() is also output to INFO(LOG). https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:661: typedef std::map<int32, base::TimeTicks> IdToTicks; On 2015/05/07 11:31:50, Pawel Osciak wrote: > s/Id/InputId/ Removed this typedef after changing data structure. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:663: IdToTicks frame_encode_start_ticks_; On 2015/05/07 11:31:50, Pawel Osciak wrote: > Since we use input ids starting from 0 and incrementing by 1, a vector would be > enough? You could access input_id frame via vector[input_id]? Yes, already changed in patchset 6. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:664: // The encode latencies of all encoded frames. On 2015/05/07 11:31:51, Pawel Osciak wrote: > We should describe how we define latency. Maybe ", i.e. time delta between > calling VEA::Encode() on a frame and receiving it encoded via > VEA::BitstreamBufferReady()" ? Yes, it's in commit message. Copied over here. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:935: base::TimeDelta VEAClient::percentile( On 2015/05/07 11:31:51, Pawel Osciak wrote: > I think this doesn't have to be a member method? It's class static private method. Shall I move to global anonymous namespace? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:939: CHECK_NE(0, size); On 2015/05/07 11:31:51, Pawel Osciak wrote: > CHECK_GT(size, 0) ? Done. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:944: std::min(base::checked_cast<size_t>(percentage * size), size - 1); On 2015/05/07 11:31:51, Pawel Osciak wrote: > Is this to guard ourselves from getting a percentage > 1 as an argument to this > function? Would it be more explicit to say if (percentage >= 1) return > sorted_values.back() ? Can't change to that. It overflows even when percentage is slightly smaller than 1.0f. My first version is index = (size - 1) * ratio, which doesn't have overflow issue, but is different than the official formula. The official formula is index = max(ceil(size * ratio) - 1, 0) The current code uses index = min(floor(size * ratio), size - 1), which is the same as the official formula except for marginal cases. It's still ok, I think. Discussed with Owen, I'll change to the same as official formula to reduce confusions for future readers. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1173: // Record encode latency of this frame. On 2015/05/07 11:31:51, Pawel Osciak wrote: > We should CHECK that run_at_fps is true if we are going to measure encode > latency. Otherwise we get skewed results, since we may queue many frames at once > with the same encode start time. You're right! This value is used in autotest only when run_at_fps is enabled. But I cannot CHECK() it. I can only skip the related code when run_at_fps is false. Please check if the next PatchSet meets your expectation (I added a helper method to decide that). https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1218: if (test_perf_) Added VerifyMinFPS() method https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1235: int32 percentage = kLoggedLatencyPercentages[i]; On 2015/05/07 11:31:51, Pawel Osciak wrote: > percentiles, also, no need for int32 specifically? Changed to p. percentile is method name. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1238: base::StringPrintf("Encode latency at %d%%", percentage), On 2015/05/07 11:31:50, Pawel Osciak wrote: > Encode latency for %dth percentile: Done. Added 'the' since it's full sentence.
On 2015/05/07 11:31:51, Pawel Osciak wrote: > > https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... > content/common/gpu/media/video_encode_accelerator_unittest.cc:787: > run_at_fps_(run_at_fps) { > We should have a CHECK here that verifies that if run_at_fps_ is true, > test_perf_ (or actually ensure_min_fps_) is not true. Right, leave to another CL because it's unrelated to this one.
lgtm https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:968: // Speedup vector insertion. s/Speedup/Speed up/
https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:79: static int32 kLoggedLatencyPercentages[] = {50, 75, 95}; On 2015/05/07 10:13:18, Justin Chuang wrote: > On 2015/05/07 09:53:45, Owen Lin wrote: > > Please change it back to using float. We have the issue because of data > > presentation. > > > > But > > 1. We should not change the code just because of the data presentation. > > 2. We don't need those "round" and "cast" things anymore. See below. > > Interesting. I'd prefer to keep the current code because > 1. The array is added in this CL. So no harm to change its data format before > commit. > 2. Float => int conversion is not very comfortable although we have good helpers > and modern compiler/lib support is good. int to float is more straightforward. > Thoughts? I am OK with the code it is now. But my point is the number should be float in its nature. It makes sense for asking for a 0.1% percentile. We don't need float => int conversion at all. What we need is to show a float number and that is what printf does.
https://chromiumcodereview.appspot.com/1117853002/diff/80001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1117853002/diff/80001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:79: static int32 kLoggedLatencyPercentages[] = {50, 75, 95}; On 2015/05/08 03:57:52, Justin Chuang wrote: > On 2015/05/07 11:31:50, Pawel Osciak wrote: > > s/Percentages/Percentiles/ > > > > Do we need to use int32 in particular? Just unsigned int should work? Also, as > a > > side note, intXX are deprecated in favor of intXX_t. > > Done. also changed to uint32_t. Thx > There are other occurrences of int32 in this file. Leave it for another CL. Thanks, but my point was we didn't explicitly need a 32-bit value. Just unsigned int should be fine. Where we use int32 here is for the input ids, because the VDA interface still uses int32 there. https://chromiumcodereview.appspot.com/1117853002/diff/80001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:664: // The encode latencies of all encoded frames. On 2015/05/08 03:57:52, Justin Chuang wrote: > On 2015/05/07 11:31:51, Pawel Osciak wrote: > > We should describe how we define latency. Maybe ", i.e. time delta between > > calling VEA::Encode() on a frame and receiving it encoded via > > VEA::BitstreamBufferReady()" ? > > Yes, it's in commit message. Copied over here. Thanks. Better to have it here for people reading the code in the future. https://chromiumcodereview.appspot.com/1117853002/diff/80001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:718: // If true, verify performance at the end of the test. On 2015/05/07 11:31:51, Pawel Osciak wrote: > We should probably call this something like ensure_min_fps_, since this is the > only thing that it governs now. Could we do this please? https://chromiumcodereview.appspot.com/1117853002/diff/80001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:935: base::TimeDelta VEAClient::percentile( On 2015/05/08 03:57:52, Justin Chuang wrote: > On 2015/05/07 11:31:51, Pawel Osciak wrote: > > I think this doesn't have to be a member method? > > It's class static private method. Shall I move to global anonymous namespace? Yes please. https://chromiumcodereview.appspot.com/1117853002/diff/80001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:1173: // Record encode latency of this frame. On 2015/05/08 03:57:52, Justin Chuang wrote: > On 2015/05/07 11:31:51, Pawel Osciak wrote: > > We should CHECK that run_at_fps is true if we are going to measure encode > > latency. Otherwise we get skewed results, since we may queue many frames at > once > > with the same encode start time. > > You're right! This value is used in autotest only when run_at_fps is enabled. > But I cannot CHECK() it. I can only skip the related code when run_at_fps is > false. Why can't we CHECK it? > Please check if the next PatchSet meets your expectation (I added a helper > method to decide that). https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:395: // to keep the Autotest in sync. Could we have the test name for which this needs to be updated here please? https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:592: bool needs_encode_latency() { return run_at_fps_; } We may not always want encode latency when we run_at_fps. We should instead have a separate test or separate flag for enabling encode latency measurement and forcefully enable run_at_fps for it with a warning, or refuse to run it without it. https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:597: // Return the percentile from a sorted array. s/percentile/|percentile|/ s/array/vector/ https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:600: float percentage); s/percentage/percentile/ https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:669: // Encode start time of all encoded frames. The array index is the frame input s/array index/position in the vector/ https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:672: // The encode latencies of all encoded frames. We define encode latency as the s/frames/frames in encode order/ https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1187: encode_latencies_.push_back(last_frame_ready_time_ - start_time); encode_latencies_ is pre-reserved at l.971, but this creates a new element at the end enlarging it further? https://chromiumcodereview.appspot.com/1117853002/diff/120001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1245: for (size_t i = 0; i < arraysize(kLoggedLatencyPercentiles); i++) { for (const auto& p : kLoggedLatencyPercentiles) { }
https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:79: static int32 kLoggedLatencyPercentages[] = {50, 75, 95}; On 2015/05/11 03:43:56, Pawel Osciak wrote: > On 2015/05/08 03:57:52, Justin Chuang wrote: > > On 2015/05/07 11:31:50, Pawel Osciak wrote: > > > s/Percentages/Percentiles/ > > > > > > Do we need to use int32 in particular? Just unsigned int should work? Also, > as > > a > > > side note, intXX are deprecated in favor of intXX_t. > > > > Done. also changed to uint32_t. Thx > > There are other occurrences of int32 in this file. Leave it for another CL. > > Thanks, but my point was we didn't explicitly need a 32-bit value. Just unsigned > int should be fine. > > Where we use int32 here is for the input ids, because the VDA interface still > uses int32 there Got it. Changed. I thought we're using LP64 in arm64 and x86_64, and there is no difference between int32_t and int. But I'm fine w/ both if Chromium coding style doesn't prefer explicit sized types. https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:935: base::TimeDelta VEAClient::percentile( On 2015/05/11 03:43:56, Pawel Osciak wrote: > On 2015/05/08 03:57:52, Justin Chuang wrote: > > On 2015/05/07 11:31:51, Pawel Osciak wrote: > > > I think this doesn't have to be a member method? > > > > It's class static private method. Shall I move to global anonymous namespace? > > Yes please. Done. BTW, I see many static functions in this file. We don't need those 'static' keyword if they are already in anonymous namespace, right? https://codereview.chromium.org/1117853002/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1173: // Record encode latency of this frame. On 2015/05/11 03:43:56, Pawel Osciak wrote: > On 2015/05/08 03:57:52, Justin Chuang wrote: > > On 2015/05/07 11:31:51, Pawel Osciak wrote: > > > We should CHECK that run_at_fps is true if we are going to measure encode > > > latency. Otherwise we get skewed results, since we may queue many frames at > > once > > > with the same encode start time. > > > > You're right! This value is used in autotest only when run_at_fps is enabled. > > But I cannot CHECK() it. I can only skip the related code when run_at_fps is > > false. > > Why can't we CHECK it? > Please see below. https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:395: // to keep the Autotest in sync. On 2015/05/11 03:43:56, Pawel Osciak wrote: > Could we have the test name for which this needs to be updated here please? Add reference to video_VEAPerf here. https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:592: bool needs_encode_latency() { return run_at_fps_; } On 2015/05/11 03:43:57, Pawel Osciak wrote: > We may not always want encode latency when we run_at_fps. We should instead have > a separate test or separate flag for enabling encode latency measurement and > forcefully enable run_at_fps for it with a warning, or refuse to run it without > it. The problem is that run_at_fps_ is controlled by command line arg --run_at_fps, we do not override it per test. If we only add encode latency as per-test flag in VideoEncodeAcceleratorTest params and enable it in some tests, we need to add --run_at_fps in video_VideoEncodeAccelerator because it runs all tests. And --run_at_fps probably should be enabled by default for users running the unittest manually. This only complicates things. Suggested methods are 1. Keep the current method, but add comments that run_at_fps will also measure encode latency. 2. Add a separate command-line option to measure encode latency, and check the dependency with --run_at_fps at main(). I'm okay for both method, but I'm not a fan of adding more command-line args. Thoughts? https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:597: // Return the percentile from a sorted array. On 2015/05/11 03:43:56, Pawel Osciak wrote: > s/percentile/|percentile|/ > s/array/vector/ Done https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:600: float percentage); On 2015/05/11 03:43:57, Pawel Osciak wrote: > s/percentage/percentile/ Done. https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:669: // Encode start time of all encoded frames. The array index is the frame input On 2015/05/11 03:43:57, Pawel Osciak wrote: > s/array index/position in the vector/ Done. https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:672: // The encode latencies of all encoded frames. We define encode latency as the On 2015/05/11 03:43:57, Pawel Osciak wrote: > s/frames/frames in encode order/ At first, the order is irrelevant. Then the vector will be in-place sorted by value. So I prefer not to change the comment. https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:968: // Speedup vector insertion. On 2015/05/08 05:11:59, wuchengli wrote: > s/Speedup/Speed up/ Done. https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1187: encode_latencies_.push_back(last_frame_ready_time_ - start_time); On 2015/05/11 03:43:57, Pawel Osciak wrote: > encode_latencies_ is pre-reserved at l.971, but this creates a new element at > the end enlarging it further? No, vector::reserve() only increase the capacity, not the vector size. The vector size is still 0 after reserve(). push_back() increments the size by 1. capacity is internal data representation usually hidden from caller. The reserve() call only speedup the insertion by avoiding repeated reallocation of the internal array. (the behavior is the same with or without reserve(), only the performance differs.) https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1245: for (size_t i = 0; i < arraysize(kLoggedLatencyPercentiles); i++) { On 2015/05/11 03:43:57, Pawel Osciak wrote: > for (const auto& p : kLoggedLatencyPercentiles) { > } Done. Thanks. I didn't know this new style also works on C array.
> On 2015/05/07 11:31:51, Pawel Osciak wrote: > > We should probably call this something like ensure_min_fps_, since this is the > > only thing that it governs now. > > Could we do this please? > I already added in later patchsets. Thanks
https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:592: bool needs_encode_latency() { return run_at_fps_; } On 2015/05/11 11:24:54, Justin Chuang wrote: > On 2015/05/11 03:43:57, Pawel Osciak wrote: > > We may not always want encode latency when we run_at_fps. We should instead > have > > a separate test or separate flag for enabling encode latency measurement and > > forcefully enable run_at_fps for it with a warning, or refuse to run it > without > > it. > > The problem is that run_at_fps_ is controlled by command line arg --run_at_fps, > we do not override it per test. We could if needed though, just for that test (and display a warning about this). But just having a different "mode" activated via a flag (option 2) is probably better. > If we only add encode latency as per-test flag in VideoEncodeAcceleratorTest > params and enable it in some tests, we need to add --run_at_fps in > video_VideoEncodeAccelerator because it runs all tests. And --run_at_fps > probably should be enabled by default for users running the unittest manually. > This only complicates things. If we had --run_at_fps by default for users running the test manually, we would slow down the test significantly for those who didn't want to measure perf. We regularly run the test at maximum speed without run_at_fps to run all the other tests (basic test, keyframes, bitrates, etc.). > > Suggested methods are > 1. Keep the current method, but add comments that run_at_fps will also measure > encode latency. > 2. Add a separate command-line option to measure encode latency, and check the > dependency with --run_at_fps at main(). > I'm okay for both method, but I'm not a fan of adding more command-line args. > > Thoughts? Option 2 sounds better to me. https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1187: encode_latencies_.push_back(last_frame_ready_time_ - start_time); On 2015/05/11 11:24:54, Justin Chuang wrote: > On 2015/05/11 03:43:57, Pawel Osciak wrote: > > encode_latencies_ is pre-reserved at l.971, but this creates a new element at > > the end enlarging it further? > > No, vector::reserve() only increase the capacity, not the vector size. The > vector size is still 0 after reserve(). > push_back() increments the size by 1. capacity is internal data representation > usually hidden from caller. > > The reserve() call only speedup the insertion by avoiding repeated reallocation > of the internal array. (the behavior is the same with or without reserve(), only > the performance differs.) Acknowledged. https://codereview.chromium.org/1117853002/diff/160001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/160001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1236: for (unsigned int percentile : kLoggedLatencyPercentiles) { s/unsigned int/const auto&/ ?
Pawel, PTAL again. Thanks! https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/120001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:592: bool needs_encode_latency() { return run_at_fps_; } On 2015/05/13 03:21:18, Pawel Osciak wrote: > > Suggested methods are > > 1. Keep the current method, but add comments that run_at_fps will also measure > > encode latency. > > 2. Add a separate command-line option to measure encode latency, and check the > > dependency with --run_at_fps at main(). > > I'm okay for both method, but I'm not a fan of adding more command-line args. > > > > Thoughts? > > Option 2 sounds better to me. Done. https://codereview.chromium.org/1117853002/diff/160001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/160001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1236: for (unsigned int percentile : kLoggedLatencyPercentiles) { On 2015/05/13 03:21:18, Pawel Osciak wrote: > s/unsigned int/const auto&/ ? Done. I changed to unsigned int because Percentile() uses this type explicitly, but using auto is also okay.
lgtm % nits https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:424: bool needs_encode_latency_; Perhaps this should be in VEAClient, since it's the client collecting this and for consistency with run_at_fps. https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:941: double VEAClient::frames_per_second() { CHECK(!encode_start_time_.empty()); https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1176: base::TimeTicks start_time = encode_start_time_[num_encoded_frames_]; CHECK_LT(num_encoded_frames_, encode_start_time_.size()); https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1229: // Log measured FPS. Comment unnecessary. https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1481: if (it->first == "encode_latency") { s/encode_latency/measure_latency/
Pawel. Thanks. PTAL again. https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:424: bool needs_encode_latency_; On 2015/05/14 01:46:29, Pawel Osciak wrote: > Perhaps this should be in VEAClient, since it's the client collecting this and > for consistency with run_at_fps. Thanks. I'm aware how run_at_fps was placed. For now, run_at_fps is not a per-test flag yet. Although it looks "more correct" to define the flag in VEAClient, I prefer to remove VEAClient.run_at_fps_, add VEATestEnvironment.RunAtFPS(), and refactor other global vars (g_fake_encoder and g_num_frames_to_encode) into methods in VEATestEnvironment, too. Does it sound okay (in separate CL)? It's probably about different perspectives. When I was new to this test, I felt this file is too big to understand quickly, and I prefer to keep it as simple as possible, trading off other criteria. But for the initiated, complexity may not be a good reason. https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:941: double VEAClient::frames_per_second() { On 2015/05/14 01:46:29, Pawel Osciak wrote: > CHECK(!encode_start_time_.empty()); Done. But please see below. https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1176: base::TimeTicks start_time = encode_start_time_[num_encoded_frames_]; On 2015/05/14 01:46:29, Pawel Osciak wrote: > CHECK_LT(num_encoded_frames_, encode_start_time_.size()); Vector will throw out_of_range. Shall I CHECK it again? For Python, we usually don't do it at all and relies on runtime exception in builtin types, or there will be many to check, and no much differences between manual checking and runtime exceptions in types. https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1229: // Log measured FPS. On 2015/05/14 01:46:29, Pawel Osciak wrote: > Comment unnecessary. Done. https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1481: if (it->first == "encode_latency") { On 2015/05/14 01:46:29, Pawel Osciak wrote: > s/encode_latency/measure_latency/ Done.
https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1117853002/diff/180001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1176: base::TimeTicks start_time = encode_start_time_[num_encoded_frames_]; On 2015/05/14 05:16:13, Justin Chuang wrote: > On 2015/05/14 01:46:29, Pawel Osciak wrote: > > CHECK_LT(num_encoded_frames_, encode_start_time_.size()); > > Vector will throw out_of_range. Shall I CHECK it again? For Python, we usually > don't do it at all and relies on runtime exception in builtin types, or there > will be many to check, and no much differences between manual checking and > runtime exceptions in types. Please CHECK. The terminate message for out_of_range exception won't show us where this happened. Much easier to CHECK here.
Refactor out VEAClient::run_at_fps_ after discuss with Pawel. https://codereview.chromium.org/1117853002/diff/180001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/180001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1176: base::TimeTicks start_time = encode_start_time_[num_encoded_frames_]; On 2015/05/14 06:02:52, Pawel Osciak wrote: > On 2015/05/14 05:16:13, Justin Chuang wrote: > > On 2015/05/14 01:46:29, Pawel Osciak wrote: > > > CHECK_LT(num_encoded_frames_, encode_start_time_.size()); > > > > Vector will throw out_of_range. Shall I CHECK it again? For Python, we usually > > don't do it at all and relies on runtime exception in builtin types, or there > > will be many to check, and no much differences between manual checking and > > runtime exceptions in types. > > Please CHECK. The terminate message for out_of_range exception won't show us > where this happened. Much easier to CHECK here. Done.
lgtm % nits https://codereview.chromium.org/1117853002/diff/240001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/240001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:418: bool RunAtFps() const { return run_at_fps_; } Simple accessors like this should be named like this: run_at_fps() (http://www.chromium.org/developers/coding-style#TOC-Inline-functions). https://codereview.chromium.org/1117853002/diff/240001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:422: bool NeedsEncodeLatency() const { return needs_encode_latency_; } Here as well.
On 2015/05/14 07:24:06, Pawel Osciak wrote: > lgtm % nits > > https://codereview.chromium.org/1117853002/diff/240001/content/common/gpu/med... > File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): > > https://codereview.chromium.org/1117853002/diff/240001/content/common/gpu/med... > content/common/gpu/media/video_encode_accelerator_unittest.cc:418: bool > RunAtFps() const { return run_at_fps_; } > Simple accessors like this should be named like this: run_at_fps() > (http://www.chromium.org/developers/coding-style#TOC-Inline-functions). > > https://codereview.chromium.org/1117853002/diff/240001/content/common/gpu/med... > content/common/gpu/media/video_encode_accelerator_unittest.cc:422: bool > NeedsEncodeLatency() const { return needs_encode_latency_; } > Here as well. Done thanks. Will CQ after rebasing.
The CQ bit was checked by jchuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org, wuchengli@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1117853002/#ps280001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1117853002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/b21ffeee2aff4da8edb9808b2227165c37e5ce86 Cr-Commit-Position: refs/heads/master@{#329819}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/1137643003/ by jchuang@chromium.org. The reason for reverting is: Fail to compile on x86 Chrome OS..
The CQ bit was checked by jchuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org, wuchengli@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1117853002/#ps300001 (title: "Fix compile error on x86/daisy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1117853002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/738487a69f150975376c36e214c64a77b1ff0cc6 Cr-Commit-Position: refs/heads/master@{#329838}
Message was sent while issue was closed.
achuith@chromium.org changed reviewers: + achuith@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1117853002/diff/300001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1117853002/diff/300001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:205: CHECK_LE(percentile, 100); It's pretty sad, but the compiler seems to think 100 is a signed int here.
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/1145543002/ by achuith@chromium.org. The reason for reverting is: https://uberchromegw.corp.google.com/i/chromiumos.chromium/builders/X86%20%28... chromium-os builders went red. Looks like there's a signed/unsigned comparison..
Message was sent while issue was closed.
On 2015/05/15 00:24:10, achuithb wrote: > https://codereview.chromium.org/1117853002/diff/300001/content/common/gpu/med... > File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): > > https://codereview.chromium.org/1117853002/diff/300001/content/common/gpu/med... > content/common/gpu/media/video_encode_accelerator_unittest.cc:205: > CHECK_LE(percentile, 100); > It's pretty sad, but the compiler seems to think 100 is a signed int here. Yes, that's really unfortunate. :P Thanks for reverting.
Message was sent while issue was closed.
Patchset #17 (id:320001) has been deleted |
