Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
769133003:
vea_unittest - Control the rate of input frames. (Closed)
|
Created:
6 years ago by Owen Lin Modified:
6 years 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 - Control the rate of input frames.
For some performance tests, we need to control the rate of the
input frames, instead of feeding those frames at the full speed.
For example, this can simulate the frames coming from a video
capture device. So that we can measure the CPU usage and the power
consumption in these scenarios.
BUG=chromium:350255
TEST=Run the test on peach-pit
Committed: https://crrev.com/a4b5815816e5f45423e8cf76fea84eec5f520634
Cr-Commit-Position: refs/heads/master@{#309397}
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 16
Patch Set 6 : Address Pawel's comments #
Total comments: 2
Patch Set 7 : #
Total comments: 1
Patch Set 8 : rebase and rename to run_at_fps #Messages
Total messages: 20 (2 generated)
owenlin@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
PTAL. Thanks.
https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:99: double g_input_fps = 0; Document this and move it to VideoEncodeAcceleratorTestEnvironment. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:506: void FeedEncoderWithOneInputIfPossible(); Document this. Since FeedEncoderWithInputs doesn't have IfPossible suffix, this doesn't need it, either. s/FeedEncoderWithOneInputIfPossible/FeedEncoderWithOneInput/ https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:622: base::TimeDelta input_duration_; Document these two variables. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:666: if (g_input_fps > 0) This should be passed from the constructor. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:916: void VEAClient::FeedEncoderWithInputs() { FeedEncoderWithInputs should be implemented by calling FeedEncoderWithOneInputIfPossible. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:953: // Destroctor will also stop the timer. Destructor
Thanks. Please take another look. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:99: double g_input_fps = 0; On 2014/12/09 05:26:32, wuchengli wrote: > Document this and move it to VideoEncodeAcceleratorTestEnvironment. Done. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:506: void FeedEncoderWithOneInputIfPossible(); On 2014/12/09 05:26:33, wuchengli wrote: > Document this. Since FeedEncoderWithInputs doesn't have IfPossible suffix, this > doesn't need it, either. > s/FeedEncoderWithOneInputIfPossible/FeedEncoderWithOneInput/ The postfix "IfPossible" is because we may not feed any input at all. So, I prefer to keep it. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:622: base::TimeDelta input_duration_; On 2014/12/09 05:26:32, wuchengli wrote: > Document these two variables. Done. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:666: if (g_input_fps > 0) On 2014/12/09 05:26:33, wuchengli wrote: > This should be passed from the constructor. Done. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:916: void VEAClient::FeedEncoderWithInputs() { On 2014/12/09 05:26:32, wuchengli wrote: > FeedEncoderWithInputs should be implemented by calling > FeedEncoderWithOneInputIfPossible. Done. https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:953: // Destroctor will also stop the timer. On 2014/12/09 05:26:33, wuchengli wrote: > Destructor Done.
https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:506: void FeedEncoderWithOneInputIfPossible(); On 2014/12/09 08:29:15, Owen Lin wrote: > On 2014/12/09 05:26:33, wuchengli wrote: > > Document this. Since FeedEncoderWithInputs doesn't have IfPossible suffix, > this > > doesn't need it, either. > > s/FeedEncoderWithOneInputIfPossible/FeedEncoderWithOneInput/ > > The postfix "IfPossible" is because we may not feed any input at all. > So, I prefer to keep it. Then FeedEncoderWithInputs should be changed to FeedEncoderWithInputsIfPossible because we may not feed any input at all? https://codereview.chromium.org/769133003/diff/20001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/20001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:681: if (input_fps_ > 0) |input_fps_| is only used in the constructor. Remove the class variable. https://codereview.chromium.org/769133003/diff/20001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:822: FeedEncoderWithInputs(); This can be while(FeedEncoderWithOneInput()) ; InputNoLongerNeededCallback() can use FeedEncoderWithOneInput. Then FeedEncoderWithInputs can be removed. https://codereview.chromium.org/769133003/diff/20001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:937: // We keep |FeedEncoderWIthOneInputInternal| and Use base::IgnoreResultHelper and remove FeedEncoderWIthOneInputInternal. https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....
https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/1/content/common/gpu/media/vid... content/common/gpu/media/video_encode_accelerator_unittest.cc:506: void FeedEncoderWithOneInputIfPossible(); On 2014/12/10 02:29:14, wuchengli wrote: > On 2014/12/09 08:29:15, Owen Lin wrote: > > On 2014/12/09 05:26:33, wuchengli wrote: > > > Document this. Since FeedEncoderWithInputs doesn't have IfPossible suffix, > > this > > > doesn't need it, either. > > > s/FeedEncoderWithOneInputIfPossible/FeedEncoderWithOneInput/ > > > > The postfix "IfPossible" is because we may not feed any input at all. > > So, I prefer to keep it. > Then FeedEncoderWithInputs should be changed to FeedEncoderWithInputsIfPossible > because we may not feed any input at all? > I was consider 0 as an example of multiple. https://codereview.chromium.org/769133003/diff/20001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/20001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:681: if (input_fps_ > 0) On 2014/12/10 02:29:14, wuchengli wrote: > |input_fps_| is only used in the constructor. Remove the class variable. Done. https://codereview.chromium.org/769133003/diff/20001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:822: FeedEncoderWithInputs(); On 2014/12/10 02:29:14, wuchengli wrote: > This can be while(FeedEncoderWithOneInput()) > ; > > InputNoLongerNeededCallback() can use FeedEncoderWithOneInput. Then > FeedEncoderWithInputs can be removed. Done. https://codereview.chromium.org/769133003/diff/20001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:937: // We keep |FeedEncoderWIthOneInputInternal| and On 2014/12/10 02:29:14, wuchengli wrote: > Use base::IgnoreResultHelper and remove FeedEncoderWIthOneInputInternal. > https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers.... Done.
https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:503: // then it asked for via RequireBitstreamBuffers(). Return false if no input s/then/than/ https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:884: while (FeedEncoderWithOneInput()) One one input buffer is released by line 882. So we don't need the while loop here. Right? https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:927: input_timer_.reset(); Do we really need this here? No other clean-up is done here. I think this should be removed. https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:944: DVLOG(1) << "Drop Input Frame: " + pos_in_input_stream_; s/Input Frame/input frame/
https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:503: // then it asked for via RequireBitstreamBuffers(). Return false if no input On 2014/12/10 07:06:56, wuchengli wrote: > s/then/than/ Done. https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:884: while (FeedEncoderWithOneInput()) On 2014/12/10 07:06:56, wuchengli wrote: > One one input buffer is released by line 882. So we don't need the while loop > here. Right? Done. https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:927: input_timer_.reset(); On 2014/12/10 07:06:56, wuchengli wrote: > Do we really need this here? No other clean-up is done here. I think this should > be removed. Yes, it can be removed. But I prefer not. This is not only the clean-up. The main function here is to stop the timer. Otherwise, we will call into this function later (although, it should be no harms.) It can be replaced by "if (input_timer_) input_timer_->Stop()". But, since the timer will never be used, why not just destroy it? https://codereview.chromium.org/769133003/diff/40001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:944: DVLOG(1) << "Drop Input Frame: " + pos_in_input_stream_; On 2014/12/10 07:06:56, wuchengli wrote: > s/Input Frame/input frame/ Done.
https://codereview.chromium.org/769133003/diff/60001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/60001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:883: DCHECK(FeedEncoderWithOneInput()); if (input_duration_ == base::TimeDelta()) is still required here. Right? Otherwise, the encoder will be fed an input immediately after encoding.
https://chromiumcodereview.appspot.com/769133003/diff/60001/content/common/gp... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/769133003/diff/60001/content/common/gp... content/common/gpu/media/video_encode_accelerator_unittest.cc:883: DCHECK(FeedEncoderWithOneInput()); On 2014/12/11 05:08:08, wuchengli wrote: > if (input_duration_ == base::TimeDelta()) is still required here. Right? > Otherwise, the encoder will be fed an input immediately after encoding. Thanks.
LGTM. Pawel. Need owner review.
https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:467: double input_fps, Would it make more sense to make it an argument per test stream? https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:503: // than it asked for via RequireBitstreamBuffers(). Return false if no input I think you meant unless it has as many buffers as it asked, not more. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:884: DCHECK(FeedEncoderWithOneInput()); Isn't this basically a DCHECK on whether the encoder is fast enough or not? This makes it pretty indeterministic. I would prefer not having this. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:925: // Destructor will also stop the timer. If so, do we need to do this here as well? https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:941: if (inputs_at_client_.size() >= I'd extract this out from here and have the while loop in the caller. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:943: DVLOG(1) << "Drop input frame: " + pos_in_input_stream_; s/Drop/Dropping/ https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:1118: g_env->test_streams_[test_stream_index], g_env->input_fps_, Uh... did git cl format do this? The previous version was much more readable...
https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:467: double input_fps, On 2014/12/16 10:56:40, Pawel Osciak wrote: > Would it make more sense to make it an argument per test stream? Sound reasonable. Done. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:503: // than it asked for via RequireBitstreamBuffers(). Return false if no input On 2014/12/16 10:56:40, Pawel Osciak wrote: > I think you meant unless it has as many buffers as it asked, not more. Done. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:884: DCHECK(FeedEncoderWithOneInput()); On 2014/12/16 10:56:40, Pawel Osciak wrote: > Isn't this basically a DCHECK on whether the encoder is fast enough or not? This > makes it pretty indeterministic. I would prefer not having this. No, this callback called when the encoder free an input buffer. So, it is expected we can feed one input buffer to the encoder. However, I have refactored the code and FeedEncoderWithOneInput no longer return bool, so I removed the DCHECK as well. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:925: // Destructor will also stop the timer. On 2014/12/16 10:56:40, Pawel Osciak wrote: > If so, do we need to do this here as well? I would prefer to cancel the timer as early as we don't need it. But it should make no harm of removing it excepts this function will continue being called until the encoder gets destroyed. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:941: if (inputs_at_client_.size() >= On 2014/12/16 10:56:41, Pawel Osciak wrote: > I'd extract this out from here and have the while loop in the caller. Done. I have moved this out to the while loop in the caller. Due to this change we need another callback function for the |input_timer_|. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:943: DVLOG(1) << "Drop input frame: " + pos_in_input_stream_; On 2014/12/16 10:56:40, Pawel Osciak wrote: > s/Drop/Dropping/ Done. https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:1118: g_env->test_streams_[test_stream_index], g_env->input_fps_, On 2014/12/16 10:56:40, Pawel Osciak wrote: > Uh... did git cl format do this? The previous version was much more readable... I feel the same way, but it's git cl format. Due to refactoring, this code is no longer changed.
https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:925: // Destructor will also stop the timer. On 2014/12/17 05:27:29, Owen Lin wrote: > On 2014/12/16 10:56:40, Pawel Osciak wrote: > > If so, do we need to do this here as well? > > I would prefer to cancel the timer as early as we don't need it. But it should > make no harm of removing it excepts this function will continue being called > until the encoder gets destroyed. Ok, then the comment is confusing. It suggests that we are doing something superficial here. It would have been better to say what you explained above, or not have a comment at all. https://codereview.chromium.org/769133003/diff/100001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:135: unsigned int input_framerate; Hm, now it becomes more visible that we have a duplication of requested_framerate and input_framerate. I think what we should do is have three kinds of tests: 1) simple correctness test (SimpleEncode) which uses requested_framerate to set the framerate/bitrate calculation in encoder, but feeds the encoder as fast as possible with inputs to finish asap (and maybe test performance/fps) 2) framerate test (ForceFramerate), which is basically the same as simple encode (feeds as fast as possible), but checks bitrate is ok after it's done (and also changes it mid-stream in one of the versions like it does right now) We already have 1) and 2) and don't have to change anything. 3) real time test (RealTime), where we take requested_framerate and feed the encoder at that speed (and also set it to that framerate); And we wouldn't need input_framerate at all then I think?
https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:925: // Destructor will also stop the timer. On 2014/12/18 04:54:10, Pawel Osciak wrote: > On 2014/12/17 05:27:29, Owen Lin wrote: > > On 2014/12/16 10:56:40, Pawel Osciak wrote: > > > If so, do we need to do this here as well? > > > > I would prefer to cancel the timer as early as we don't need it. But it should > > make no harm of removing it excepts this function will continue being called > > until the encoder gets destroyed. > > Ok, then the comment is confusing. It suggests that we are doing something > superficial here. It would have been better to say what you explained above, or > not have a comment at all. Done. https://codereview.chromium.org/769133003/diff/100001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:135: unsigned int input_framerate; On 2014/12/18 04:54:10, Pawel Osciak wrote: > Hm, now it becomes more visible that we have a duplication of > requested_framerate and input_framerate. > > I think what we should do is have three kinds of tests: > > 1) simple correctness test (SimpleEncode) which uses requested_framerate to set > the framerate/bitrate calculation in encoder, but feeds the encoder as fast as > possible with inputs to finish asap (and maybe test performance/fps) > > 2) framerate test (ForceFramerate), which is basically the same as simple encode > (feeds as fast as possible), but checks bitrate is ok after it's done (and also > changes it mid-stream in one of the versions like it does right now) > > > We already have 1) and 2) and don't have to change anything. > > > 3) real time test (RealTime), where we take requested_framerate and feed the > encoder at that speed (and also set it to that framerate); > > And we wouldn't need input_framerate at all then I think? Done.
lgtm % nit https://codereview.chromium.org/769133003/diff/120001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/769133003/diff/120001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1229: if (it->first == "real_time_clock") { Hm, I know I proposed RealTime, but this doesn't tell us what's going on to be honest. Maybe run_at_fps?
The CQ bit was checked by owenlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/769133003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a4b5815816e5f45423e8cf76fea84eec5f520634 Cr-Commit-Position: refs/heads/master@{#309397} |
