|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by sprang_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman, pbos-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake WebRTC compatible with OpenH264 v1.6.
The API has changed for the slice config of SSpatialLayerConfig as of
OpenH264 v1.6. Update H264EncoderImpl with an ifdef that uses the
correct API depending on what version of OpenH264 is being used.
BUG=webrtc:6583
Committed: https://crrev.com/611f2673703e745d9e060ccbaed44b136b4d1cfc
Cr-Commit-Position: refs/heads/master@{#14762}
Patch Set 1 #Patch Set 2 : Make WebRTC compatible with OpenH264 v1.6 #Patch Set 3 : Changed from SM_SIZELIMITED_SLICE to SM_FIXEDSLCNUM_SLICE #
Total comments: 3
Patch Set 4 : Added comment and explicit assignment of uiSliceNum #Messages
Total messages: 27 (12 generated)
sprang@webrtc.org changed reviewers: + pbos@webrtc.org
I know you don't want to, but please help! :) I have no idea what slice type is appropriate here, or if it even really matters. Since you added this code, could you have a quick look or loop in someone else if not? This CL.. "It's only wafer thin."
sprang@webrtc.org changed reviewers: + mcasas@webrtc.org - pbos@webrtc.org
-pbos +mcasas since he seems to have written the code in chrome that pbos based this part of the webrtc impl on.
sprang@webrtc.org changed reviewers: + hbos@webrtc.org - mcasas@webrtc.org
Changed to SM_FIXEDSLCNUM_SLICE as per comment in https://bugs.chromium.org/p/webrtc/issues/detail?id=6583#c2 No reply from mcasas.. hbos, can you take a look so we can land this?
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
lgtm https://codereview.chromium.org/2440113002/diff/40001/webrtc/modules/video_co... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.chromium.org/2440113002/diff/40001/webrtc/modules/video_co... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:448: SM_FIXEDSLCNUM_SLICE; Maybe comment on what this means? Reading the header file it seems to mean that it will use the appropriate number of slices to achieve good multi-threading. The slices are not optimized for bitstream robustness. https://github.com/cisco/openh264/blob/997f0ed69b811697611fceb2d12ca8cfe42352...
https://codereview.chromium.org/2440113002/diff/40001/webrtc/modules/video_co... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.chromium.org/2440113002/diff/40001/webrtc/modules/video_co... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:448: SM_FIXEDSLCNUM_SLICE; On 2016/10/24 10:42:02, stefan-webrtc (holmer) wrote: > Maybe comment on what this means? Reading the header file it seems to mean that > it will use the appropriate number of slices to achieve good multi-threading. > The slices are not optimized for bitstream robustness. > > https://github.com/cisco/openh264/blob/997f0ed69b811697611fceb2d12ca8cfe42352... Comment above still says "// Slice num according to number of threads."? :)
lgtm
The CQ bit was checked by sprang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
hbos@chromium.org changed reviewers: + hbos@chromium.org
lgtm https://codereview.chromium.org/2440113002/diff/40001/webrtc/modules/video_co... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.chromium.org/2440113002/diff/40001/webrtc/modules/video_co... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:448: SM_FIXEDSLCNUM_SLICE; On 2016/10/24 11:28:19, språng wrote: > On 2016/10/24 10:42:02, stefan-webrtc (holmer) wrote: > > Maybe comment on what this means? Reading the header file it seems to mean > that > > it will use the appropriate number of slices to achieve good multi-threading. > > The slices are not optimized for bitstream robustness. > > > > > https://github.com/cisco/openh264/blob/997f0ed69b811697611fceb2d12ca8cfe42352... > > Comment above still says "// Slice num according to number of threads."? :) According to the link, SM_FIXEDSLCNUM_SLICE sets it in accordance with SliceNum (SSliceArgument::uiSliceNum). Since we don't set sSpatialLayers[0].sSliceArgument it would use its default value, which I believe is 0, https://github.com/cisco/openh264/blob/997f0ed69b811697611fceb2d12ca8cfe42352.... "uiSliceNum=0 means auto design it with cpu core number". So this should be auto. Up to you if you want to explicitly set it to 0 and add a comment saying it is based on the number of cores.
And lgtm from @webrtc account.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org, stefan@webrtc.org, hbos@chromium.org Link to the patchset: https://codereview.webrtc.org/2440113002/#ps60001 (title: "Added comment and explicit assignment of uiSliceNum")
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: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/19306)
The CQ bit was checked by sprang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make WebRTC compatible with OpenH264 v1.6. The API has changed for the slice config of SSpatialLayerConfig as of OpenH264 v1.6. Update H264EncoderImpl with an ifdef that uses the correct API depending on what version of OpenH264 is being used. BUG=webrtc:6583 ========== to ========== Make WebRTC compatible with OpenH264 v1.6. The API has changed for the slice config of SSpatialLayerConfig as of OpenH264 v1.6. Update H264EncoderImpl with an ifdef that uses the correct API depending on what version of OpenH264 is being used. BUG=webrtc:6583 Committed: https://crrev.com/611f2673703e745d9e060ccbaed44b136b4d1cfc Cr-Commit-Position: refs/heads/master@{#14762} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/611f2673703e745d9e060ccbaed44b136b4d1cfc Cr-Commit-Position: refs/heads/master@{#14762} |
