|
|
Created:
3 years, 11 months ago by sakal Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, mflodman, hbos Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd QP for libvpx VP8 decoder.
BUG=webrtc:6541, webrtc:7065
TBR=hta@webrtc.org
Review-Url: https://codereview.webrtc.org/2656603002
Cr-Commit-Position: refs/heads/master@{#16722}
Committed: https://chromium.googlesource.com/external/webrtc/+/5fec128de94c0b2d0090de8eac692dce9aa23a80
Patch Set 1 : Fix more tests. #
Total comments: 1
Patch Set 2 : DCHECK return value, format. #Patch Set 3 : Add an unittest. #Patch Set 4 : Remove unused timestamp from the test. #
Total comments: 4
Patch Set 5 : Changes according to språng's comments. #Patch Set 6 : Rebase. #Patch Set 7 : Fix tests after rebase. #Patch Set 8 : Fix new test. #Patch Set 9 : Rebase. #
Messages
Total messages: 65 (54 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sakal@webrtc.org 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_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/21879)
The CQ bit was checked by sakal@webrtc.org 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: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17342) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22599)
The CQ bit was checked by sakal@webrtc.org 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: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21365) linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/5049) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/18603)
The CQ bit was checked by sakal@webrtc.org 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/22934)
The CQ bit was checked by sakal@webrtc.org 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.
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Add QP for libvpx VP8 decoder. BUG=webrtc:6541 ========== to ========== Add QP for libvpx VP8 decoder. BUG=webrtc:6541 ==========
sakal@webrtc.org changed reviewers: + sprang@webrtc.org
PTAL https://codereview.webrtc.org/2656603002/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right): https://codereview.webrtc.org/2656603002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:1149: vpx_codec_control(decoder_, VPXD_GET_LAST_QUANTIZER, &qp); This method can return failure in theory. However, in practice it only happens if it is not supplied a valid int pointer. I would prefer not adding dead code for handling this. WDYT?
looks good, but I'd appreciate a unit test as well
On 2017/02/17 11:41:31, språng wrote: > looks good, but I'd appreciate a unit test as well Done. PTAL
lgtm with nits https://codereview.webrtc.org/2656603002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right): https://codereview.webrtc.org/2656603002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:257: encoder_->Encode(*input_frame_, NULL, NULL); nullptr for all new code https://codereview.webrtc.org/2656603002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:266: EXPECT_EQ(encoded_frame_.qp_, *decoded_qp_); I'd assert decoded_qp_ has a value first, or maybe decoded_qp_.value_or(-1)
https://codereview.webrtc.org/2656603002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right): https://codereview.webrtc.org/2656603002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:257: encoder_->Encode(*input_frame_, NULL, NULL); On 2017/02/17 12:31:59, språng wrote: > nullptr for all new code Done. https://codereview.webrtc.org/2656603002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:266: EXPECT_EQ(encoded_frame_.qp_, *decoded_qp_); On 2017/02/17 12:31:59, språng wrote: > I'd assert decoded_qp_ has a value first, or maybe decoded_qp_.value_or(-1) Done.
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2656603002/#ps180001 (title: "Changes according to språng's comments.")
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
Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/18868) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13618)
The CQ bit was checked by sakal@webrtc.org 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: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22888)
The CQ bit was checked by sakal@webrtc.org 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/23219)
The CQ bit was checked by sakal@webrtc.org 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_arm on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/5931)
The CQ bit was checked by sakal@webrtc.org 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/...
Description was changed from ========== Add QP for libvpx VP8 decoder. BUG=webrtc:6541 ========== to ========== Add QP for libvpx VP8 decoder. BUG=webrtc:6541, webrtc:7065 ==========
sakal@webrtc.org changed reviewers: + hta@webrtc.org
hta, PTAL at webrtc/pc/rtcstats_integrationtest.cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hbos@webrtc.org changed reviewers: + hbos@webrtc.org
hta is OOO, lgtm webrtc/pc/rtcstats_integrationtest.cc
Description was changed from ========== Add QP for libvpx VP8 decoder. BUG=webrtc:6541, webrtc:7065 ========== to ========== Add QP for libvpx VP8 decoder. BUG=webrtc:6541, webrtc:7065 TBR=hta@webrtc.org ==========
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2656603002/#ps260001 (title: "Rebase.")
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": 260001, "attempt_start_ts": 1487601698582850, "parent_rev": "4228784609afcea28174a673e4a2024b9a75c628", "commit_rev": "5fec128de94c0b2d0090de8eac692dce9aa23a80"}
Message was sent while issue was closed.
Description was changed from ========== Add QP for libvpx VP8 decoder. BUG=webrtc:6541, webrtc:7065 TBR=hta@webrtc.org ========== to ========== Add QP for libvpx VP8 decoder. BUG=webrtc:6541, webrtc:7065 TBR=hta@webrtc.org Review-Url: https://codereview.webrtc.org/2656603002 Cr-Commit-Position: refs/heads/master@{#16722} Committed: https://chromium.googlesource.com/external/webrtc/+/5fec128de94c0b2d0090de8ea... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://chromium.googlesource.com/external/webrtc/+/5fec128de94c0b2d0090de8ea... |