Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903163002:
Add unit tests for qp parser. (Closed)
DescriptionAdd unit tests for qp parser.
Add test for vp8/vp9 qp parser in both videoprocessor_integrationtest.
Check the qp from parser equal to that from the encoder
on every frame in every test.
Add test for vp8/vp9 qp parser in vp8/vp9_impl_test.
Check the qp parser on a single key frame.
BUG=None
Review-Url: https://codereview.webrtc.org/2903163002
Cr-Commit-Position: refs/heads/master@{#18334}
Committed: https://chromium.googlesource.com/external/webrtc/+/b5f5bdba77a8f10cd46c229166a2bcce96be55f3
Patch Set 1 #Patch Set 2 : Fix header. #Patch Set 3 : Add qp parser test to videoprocessor_integrationtest. #Patch Set 4 : Add vp8 qp parser test. Refactor. #Patch Set 5 : Fix test failure. #
Total comments: 20
Patch Set 6 : Refactor. #
Messages
Total messages: 37 (31 generated)
Description was changed from ========== Add unit tests for vp9 qp parser. BUG=None ========== to ========== Add unit tests for vp9 qp parser. BUG=None ==========
The CQ bit was checked by jianj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/20571) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25887) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24816) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/24905) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by jianj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jianj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jianj@google.com changed reviewers: + brandtr@webrtc.org, marpan@webrtc.org, mflodman@webrtc.org
Description was changed from ========== Add unit tests for vp9 qp parser. BUG=None ========== to ========== Add unit tests for qp parser. Add test for vp8/vp9 qp parser in both videoprocessor_integrationtest. Check the qp from parser equal to that from the encoder on every frame in every test. Add test for vp8/vp9 qp parser in vp8/vp9_impl_test. Check the qp parser on a single key frame. BUG=None ==========
Description was changed from ========== Add unit tests for qp parser. Add test for vp8/vp9 qp parser in both videoprocessor_integrationtest. Check the qp from parser equal to that from the encoder on every frame in every test. Add test for vp8/vp9 qp parser in vp8/vp9_impl_test. Check the qp parser on a single key frame. BUG=None ========== to ========== Add unit tests for qp parser. Add test for vp8/vp9 qp parser in both videoprocessor_integrationtest. Check the qp from parser equal to that from the encoder on every frame in every test. Add test for vp8/vp9 qp parser in vp8/vp9_impl_test. Check the qp parser on a single key frame. BUG=None ==========
The CQ bit was checked by jianj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/26329) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/12520)
The CQ bit was checked by jianj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for adding this! It looks good, but I have added some comments. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:366: qp_encoder_ = encoded_image.qp_; Move to after line 351, and add to |frame_info| instead. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:368: vp8::GetQp(encoded_image._buffer, encoded_image._length, &qp_bitstream_); These too :) https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:436: int VideoProcessorImpl::GetQpFromEncoder() { After moving the declarations in the .h file, please move these definitions to have the same relative order to the other member functions in the .cc file. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor.h (right): https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:149: virtual int GetQpFromEncoder() = 0; See comment below first. Add parameter "int frame_number", to be used to pick out the right object from |frame_infos_|. Move to between "EncodedFrameType" and "NumberDroppedFrames" below. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:152: virtual int GetQpFromBitstream() = 0; Same here. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:187: int GetQpFromEncoder() override; Place these two lines in the private section (*), between "EncodedFrameType" and "NumberDroppedFrames". (*) This is OK since they are overriding public functions in VideoProcessor. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:270: Remove. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:337: int qp_encoder_; We are trying to get away from using direct member variables to carry state about the encoded frames in this class. Could you move these two variables to be part of the FrameInfo struct instead? https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:609: #if !defined(WEBRTC_VIDEOPROCESSOR_H264_TESTS) This will disable the EXPECT whenever H264 is compiled. Better to check |codec_type| instead. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:610: if (!process.hw_codec) {}
The CQ bit was checked by jianj@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:366: qp_encoder_ = encoded_image.qp_; On 2017/05/26 07:18:45, brandtr wrote: > Move to after line 351, and add to |frame_info| instead. Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:368: vp8::GetQp(encoded_image._buffer, encoded_image._length, &qp_bitstream_); On 2017/05/26 07:18:45, brandtr wrote: > These too :) Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:436: int VideoProcessorImpl::GetQpFromEncoder() { On 2017/05/26 07:18:45, brandtr wrote: > After moving the declarations in the .h file, please move these definitions to > have the same relative order to the other member functions in the .cc file. Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor.h (right): https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:149: virtual int GetQpFromEncoder() = 0; On 2017/05/26 07:18:45, brandtr wrote: > See comment below first. > > Add parameter "int frame_number", to be used to pick out the right object from > |frame_infos_|. > > Move to between "EncodedFrameType" and "NumberDroppedFrames" below. Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:152: virtual int GetQpFromBitstream() = 0; On 2017/05/26 07:18:45, brandtr wrote: > Same here. Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:187: int GetQpFromEncoder() override; On 2017/05/26 07:18:45, brandtr wrote: > Place these two lines in the private section (*), between "EncodedFrameType" and > "NumberDroppedFrames". > > (*) This is OK since they are overriding public functions in VideoProcessor. Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:270: On 2017/05/26 07:18:45, brandtr wrote: > Remove. Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.h:337: int qp_encoder_; On 2017/05/26 07:18:45, brandtr wrote: > We are trying to get away from using direct member variables to carry state > about the encoded frames in this class. > > Could you move these two variables to be part of the FrameInfo struct instead? Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h (right): https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:609: #if !defined(WEBRTC_VIDEOPROCESSOR_H264_TESTS) On 2017/05/26 07:18:46, brandtr wrote: > This will disable the EXPECT whenever H264 is compiled. Better to check > |codec_type| instead. Done. https://codereview.webrtc.org/2903163002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:610: if (!process.hw_codec) On 2017/05/26 07:18:46, brandtr wrote: > {} Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jianj@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496165521285960,
"parent_rev": "6c4bbfa06f0cdc9f1ba51757dc78ffc4547c5672", "commit_rev":
"b5f5bdba77a8f10cd46c229166a2bcce96be55f3"}
Message was sent while issue was closed.
Description was changed from ========== Add unit tests for qp parser. Add test for vp8/vp9 qp parser in both videoprocessor_integrationtest. Check the qp from parser equal to that from the encoder on every frame in every test. Add test for vp8/vp9 qp parser in vp8/vp9_impl_test. Check the qp parser on a single key frame. BUG=None ========== to ========== Add unit tests for qp parser. Add test for vp8/vp9 qp parser in both videoprocessor_integrationtest. Check the qp from parser equal to that from the encoder on every frame in every test. Add test for vp8/vp9 qp parser in vp8/vp9_impl_test. Check the qp parser on a single key frame. BUG=None Review-Url: https://codereview.webrtc.org/2903163002 Cr-Commit-Position: refs/heads/master@{#18334} Committed: https://chromium.googlesource.com/external/webrtc/+/b5f5bdba77a8f10cd46c22916... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/b5f5bdba77a8f10cd46c22916... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
