Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
57663002:
Add VDA decode time test. (Closed)
|
Created:
7 years, 1 month ago by wuchengli Modified:
7 years, 1 month ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, apatrick_chromium, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd VDA decode time test and refactor.
This measures the median of the decode time of all the frames.
VDA::Decode is called 30 times per second to simulate WebRTC.
VideoDecodeAcceleratorTest class is renamed to
VideoDecodeAcceleratorParamTest, which shows it is a
value-parameterized test. The common code is moved to a new
test fixture VideoDecodeAcceleratorTest, which is inherited
by VideoDecodeAcceleratorParamTest. The future new tests should
be based on VideoDecodeAcceleratorTest if they do not need to
test different combinations of the parameters.
BUG=292703
TEST=Run VDA tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233503
Patch Set 1 : #
Total comments: 8
Patch Set 2 : change average to median #Patch Set 3 : sort the decode time vector #Patch Set 4 : address owenlin's comments #
Total comments: 10
Patch Set 5 : address Ami's comments #
Total comments: 1
Patch Set 6 : fix code style by owenlin's comment #
Total comments: 2
Patch Set 7 : address review comments #Messages
Total messages: 17 (0 generated)
PTAL.
I'm changing the average to median.
https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:612: it = decode_start_time_.find(picture.bitstream_buffer_id()); merge these two lines ? https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:871: base::TimeDelta::FromMilliseconds(1000 / decode_fps_)); base::TimeDelta::FromSeconds(1) / decode_fps_ https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:890: return total_decode_time_ / num_decoded_frames() / 1000; Why 1000 ? May be you should use TimeTicks for "total_decode_time_". So it is more clear what's unit you are using. https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:1425: 30); // Use 30 fps to simulate webrtc. It sounds like a parameter.
All done. PTAL. https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:612: it = decode_start_time_.find(picture.bitstream_buffer_id()); On 2013/11/04 10:20:04, Owen Lin wrote: > merge these two lines ? Done. https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:871: base::TimeDelta::FromMilliseconds(1000 / decode_fps_)); On 2013/11/04 10:20:04, Owen Lin wrote: > base::TimeDelta::FromSeconds(1) / decode_fps_ Done. https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:890: return total_decode_time_ / num_decoded_frames() / 1000; On 2013/11/04 10:20:04, Owen Lin wrote: > Why 1000 ? > > May be you should use TimeTicks for "total_decode_time_". > So it is more clear what's unit you are using. Done. Changed to |decode_start_time_| vector. https://codereview.chromium.org/57663002/diff/40001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:1425: 30); // Use 30 fps to simulate webrtc. On 2013/11/04 10:20:04, Owen Lin wrote: > It sounds like a parameter. There is no use case yet. But I changed it to a constant.
Please describe as part of your CL description the refactoring changes done in this CL as well. https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:85: : num_windows(1), render_as_thumbnails(false) {} Why not instead make these ctor params and turn the members into const? https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:94: // the filename by the "--frame_delivery_log" or "--output_log" switch. If the log is for more general purposes than just frame delivery info then the first sentence of this comment needs a rewrite, too. I think you want to rewrite this paragraph from scratch. https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:363: // |decode_fps| is the number of VDA::Decode calls per second. does this imply num_in_flight_decodes is always 1 in this case? https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:363: // |decode_fps| is the number of VDA::Decode calls per second. decode_calls_per_second would be a clearer name, as FPS implies things about the decoded stream (presentation frequency), not the decode rate. https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:911: void UpdateVideoFileParam(const std::vector<TestVideoFile*>& test_video_files, doco unclear methods
All done. PTAL. https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/rendering_helper.cc:85: : num_windows(1), render_as_thumbnails(false) {} On 2013/11/04 18:26:51, Ami Fischman wrote: > Why not instead make these ctor params and turn the members into const? Making all the members to ctor introduces more code eventually. I reverted this change to be consistent. https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:94: // the filename by the "--frame_delivery_log" or "--output_log" switch. On 2013/11/04 18:26:51, Ami Fischman wrote: > If the log is for more general purposes than just frame delivery info then the > first sentence of this comment needs a rewrite, too. > I think you want to rewrite this paragraph from scratch. Done. https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:363: // |decode_fps| is the number of VDA::Decode calls per second. On 2013/11/04 18:26:51, Ami Fischman wrote: > decode_calls_per_second would be a clearer name, as FPS implies things about the > decoded stream (presentation frequency), not the decode rate. Done. https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:363: // |decode_fps| is the number of VDA::Decode calls per second. On 2013/11/04 18:26:51, Ami Fischman wrote: > does this imply num_in_flight_decodes is always 1 in this case? Yes. num_in_flight_decodes is always 1 in this case. There is a CHECK in the constructor to ensure this. I've also added the comments here. https://codereview.chromium.org/57663002/diff/130001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:911: void UpdateVideoFileParam(const std::vector<TestVideoFile*>& test_video_files, On 2013/11/04 18:26:51, Ami Fischman wrote: > doco unclear methods Done.
lgtm https://codereview.chromium.org/57663002/diff/240001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/57663002/diff/240001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:616: = decode_start_time_.find(picture.bitstream_buffer_id()); put = in previous line
LGTM % nits. https://codereview.chromium.org/57663002/diff/290001/content/common/gpu/media... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/57663002/diff/290001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:918: const std::vector<TestVideoFile*>& test_video_files, |test_video_files| is an output parameter, so it should be last in the param list, and should be passed via non-const-pointer instead of const-ref (even though the vagaries of C++ constness makes the const-ref approach also work b/c the constness doesn't bleed through the container to the contained pointers). https://codereview.chromium.org/57663002/diff/290001/content/common/gpu/media... content/common/gpu/media/video_decode_accelerator_unittest.cc:1493: // TODO(wuchengli): remove frame_deliver_log after CrOS test get updated. Could point to the pending CL in this TODO.
All done. Submitting.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/57663002/360001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/57663002/360001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/57663002/360001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/57663002/360001
Message was sent while issue was closed.
Change committed as 233503 |
