|
|
DescriptionUpdating OpenH264 to v1.7.0
There is bug in AQ in v1.6.0 which causes raising QP to maximum value
and results in very poor quality of video no matter of allocated bitrate.
To trigger this someone needs to set packetization_mode=0 (or just do
not transmit this flag at all) in SDP. In this mode the encoder
enables multi-slice and disables AQ partially such that some part of
AQ algo still works and leads QP to maximum. The issue is fixed in v1.7.0.
BUG=webrtc:8070
Review-Url: https://codereview.webrtc.org/3011373002
Cr-Commit-Position: refs/heads/master@{#20015}
Committed: https://webrtc.googlesource.com/src/+/1440c9f70cdd701210652c7758b664ae82a4fef3
Patch Set 1 #Patch Set 2 : added test for SingleNalUnit mode #Patch Set 3 : increasing frame size mismatch threshold #
Total comments: 2
Patch Set 4 : moved unit test to separate cl #Messages
Total messages: 44 (31 generated)
Description was changed from ========== updated OpenH264 to v1.7.0 to fix issue (https://bugs.chromium.org/p/webrtc/issues/detail?id=8070) in adaptive quantization BUG= ========== to ========== updated OpenH264 to v1.7.0 to fix issue in adaptive quantization BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=8070 ==========
ssilkin@webrtc.org changed reviewers: + brandtr@webrtc.org, hbos@webrtc.org
Description was changed from ========== updated OpenH264 to v1.7.0 to fix issue in adaptive quantization BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=8070 ========== to ========== updated OpenH264 to v1.7.0 (https://github.com/cisco/openh264/releases/tag/v1.7.0) to fix issue in adaptive quantization BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=8070 ==========
The CQ bit was checked by ssilkin@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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21850)
Description was changed from ========== updated OpenH264 to v1.7.0 (https://github.com/cisco/openh264/releases/tag/v1.7.0) to fix issue in adaptive quantization BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=8070 ========== to ========== updated OpenH264 to v1.7.0 (https://github.com/cisco/openh264/releases/tag/v1.7.0) to fix issue in adaptive quantization BUG=webrtc:8070 ==========
This lgtm, but try to update the commit message to be on the form: "Short description in imperative form. Details go in a second paragraph here. BUG=webrtc:8070". Also, add kjellander@webrtc.org for the approval. Do we need to update DEPS in Chromium too?
On 2017/09/21 08:17:57, brandtr wrote: > This lgtm, but try to update the commit message to be on the form: > > "Short description in imperative form. > > Details go in a second paragraph here. > > BUG=webrtc:8070". > > Also, add mailto:kjellander@webrtc.org for the approval. Do we need to update DEPS in > Chromium too? Also, can we add a test that exercises the single NAL unit mode of the encoder here? https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
Description was changed from ========== updated OpenH264 to v1.7.0 (https://github.com/cisco/openh264/releases/tag/v1.7.0) to fix issue in adaptive quantization BUG=webrtc:8070 ========== to ========== Updating OpenH264 to v1.7.0 (https://github.com/cisco/openh264/releases/tag/v1.7.0) to fix issue in adaptive quantization There is bug in AQ in v1.6.0 which causes raising QP to maximum value and results in very poor quality of video no matter of allocated bitrate. To trigger this someone needs to set packetization_mode=0 (or just do not transmit this flag at all) in SDP. In this mode the encoder enables multi-slice and disables AQ partially such that some part of AQ algo still works and leads QP to maximum. BUG=webrtc:8070 ==========
ssilkin@webrtc.org changed reviewers: + kjellander@webrtc.org
On 2017/09/21 08:19:15, brandtr wrote: > On 2017/09/21 08:17:57, brandtr wrote: > > This lgtm, but try to update the commit message to be on the form: > > > > "Short description in imperative form. > > > > Details go in a second paragraph here. > > > > BUG=webrtc:8070". > > > > Also, add mailto:kjellander@webrtc.org for the approval. Do we need to update > DEPS in > > Chromium too? > > Also, can we add a test that exercises the single NAL unit mode of the encoder > here? > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... I'm updating chromium DEPS as well - https://chromium-review.googlesource.com/c/chromium/src/+/674868. But one test fails with v1.7. I'm debugging this. I will add test for single NALU and include it into the CL.
The CQ bit was checked by ssilkin@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: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_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/24192)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ssilkin@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_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by ssilkin@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.
Sorry for the delayed review. Yes we should update chromium to, and we should try to land both changes at the same time, we don't want two different versions. The chromium CL needs to update https://cs.chromium.org/chromium/src/third_party/openh264/README.chromium to reflect the new version number. Might I suggest splitting up the CL for adding the NALU test and updating the version into two CLs? It changes more than just unittest files. If the test fails before the version is updated you can expect it to fail and have a TODO saying when we use v.1.7 the test will pass. https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor.cc:115: return 0; Does this mean that if HW is used we don't know the max NALU length? Prefer rtc::Optional<size_t> for N/A rather than 0. https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... File modules/video_coding/codecs/test/videoprocessor_integrationtest.cc (right): https://codereview.webrtc.org/3011373002/diff/60001/modules/video_coding/code... modules/video_coding/codecs/test/videoprocessor_integrationtest.cc:345: codec.SetParam(cricket::kH264FmtpPacketizationMode, "0"); nit: RTC_DCHECK_EQ(config_packetization_mode, H264PacketizationMode::SingleNalUnit);
I moved unit test to separate CL - https://codereview.webrtc.org/3014623002/ - and addressed the notes there.
LGTM, but don't land this without having the chromium bits sorted out
Awesome btw, just one more thing. With the CL description try to: - Keep a 70 character limit per line, this is good for git log in terminal windows. - First line should be what the CL does (Updating OpenH264 to v.1.7.0), this is like a title. Subsequent lines can add details and why we want to do this (including link and that it fixes the issue) that don't fit in the title. - The codereview CL title and the first line should match. It's not visible in git log that's why the first line acts as a title.
On 2017/09/25 15:57:30, hbos wrote: > Awesome btw, just one more thing. With the CL description try to: > - Keep a 70 character limit per line, this is good for git log in terminal > windows. > - First line should be what the CL does (Updating OpenH264 to v.1.7.0), this is > like a title. Subsequent lines can add details and why we want to do this > (including link and that it fixes the issue) that don't fit in the title. > - The codereview CL title and the first line should match. It's not visible in > git log that's why the first line acts as a title. lgtm and +1 to the above suggestions.
Description was changed from ========== Updating OpenH264 to v1.7.0 (https://github.com/cisco/openh264/releases/tag/v1.7.0) to fix issue in adaptive quantization There is bug in AQ in v1.6.0 which causes raising QP to maximum value and results in very poor quality of video no matter of allocated bitrate. To trigger this someone needs to set packetization_mode=0 (or just do not transmit this flag at all) in SDP. In this mode the encoder enables multi-slice and disables AQ partially such that some part of AQ algo still works and leads QP to maximum. BUG=webrtc:8070 ========== to ========== Updating OpenH264 to v1.7.0 There is bug in AQ in v1.6.0 which causes raising QP to maximum value and results in very poor quality of video no matter of allocated bitrate. To trigger this someone needs to set packetization_mode=0 (or just do not transmit this flag at all) in SDP. In this mode the encoder enables multi-slice and disables AQ partially such that some part of AQ algo still works and leads QP to maximum. The issue is fixed in v1.7.0. BUG=webrtc:8070 ==========
Description was changed from ========== Updating OpenH264 to v1.7.0 There is bug in AQ in v1.6.0 which causes raising QP to maximum value and results in very poor quality of video no matter of allocated bitrate. To trigger this someone needs to set packetization_mode=0 (or just do not transmit this flag at all) in SDP. In this mode the encoder enables multi-slice and disables AQ partially such that some part of AQ algo still works and leads QP to maximum. The issue is fixed in v1.7.0. BUG=webrtc:8070 ========== to ========== Updating OpenH264 to v1.7.0 There is bug in AQ in v1.6.0 which causes raising QP to maximum value and results in very poor quality of video no matter of allocated bitrate. To trigger this someone needs to set packetization_mode=0 (or just do not transmit this flag at all) in SDP. In this mode the encoder enables multi-slice and disables AQ partially such that some part of AQ algo still works and leads QP to maximum. The issue is fixed in v1.7.0. BUG=webrtc:8070 ==========
On 2017/09/25 15:57:30, hbos wrote: > Awesome btw, just one more thing. With the CL description try to: > - Keep a 70 character limit per line, this is good for git log in terminal > windows. Done. > - First line should be what the CL does (Updating OpenH264 to v.1.7.0), this is > like a title. Subsequent lines can add details and why we want to do this > (including link and that it fixes the issue) that don't fit in the title. Done. > - The codereview CL title and the first line should match. It's not visible in > git log that's why the first line acts as a title. Done.
The CQ bit was checked by ssilkin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/3011373002/#ps80001 (title: "moved unit test to separate cl")
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 ssilkin@webrtc.org
The CQ bit was checked by ssilkin@webrtc.org
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": 80001, "attempt_start_ts": 1506588936902530, "parent_rev": "760c4b4da98b4bee78f4509f8496f6e170cdb88b", "commit_rev": "1440c9f70cdd701210652c7758b664ae82a4fef3"}
Message was sent while issue was closed.
Description was changed from ========== Updating OpenH264 to v1.7.0 There is bug in AQ in v1.6.0 which causes raising QP to maximum value and results in very poor quality of video no matter of allocated bitrate. To trigger this someone needs to set packetization_mode=0 (or just do not transmit this flag at all) in SDP. In this mode the encoder enables multi-slice and disables AQ partially such that some part of AQ algo still works and leads QP to maximum. The issue is fixed in v1.7.0. BUG=webrtc:8070 ========== to ========== Updating OpenH264 to v1.7.0 There is bug in AQ in v1.6.0 which causes raising QP to maximum value and results in very poor quality of video no matter of allocated bitrate. To trigger this someone needs to set packetization_mode=0 (or just do not transmit this flag at all) in SDP. In this mode the encoder enables multi-slice and disables AQ partially such that some part of AQ algo still works and leads QP to maximum. The issue is fixed in v1.7.0. BUG=webrtc:8070 Review-Url: https://codereview.webrtc.org/3011373002 Cr-Commit-Position: refs/heads/master@{#20015} Committed: https://webrtc.googlesource.com/src/+/1440c9f70cdd701210652c7758b664ae82a4fef3 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://webrtc.googlesource.com/src/+/1440c9f70cdd701210652c7758b664ae82a4fef3 |