Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2337453002:
H.264 packetization mode 0 (try 2) (Closed)
|
Created:
4 years, 3 months ago by hta-webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, danilchap, yujie_mao (webrtc), zhuangzesen_agora.io, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, perkj_webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplement H.264 packetization mode 0.
This approach extends the H.264 specific information with
a packetization mode enum.
Status: Parameter is in code. No way to set it yet.
Rebase of CL 2009213002
BUG=600254
Committed: https://crrev.com/3bba101f36483b8030a693dfbc93af736d1dba68
Cr-Commit-Position: refs/heads/master@{#15032}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Remove some more version markers #Patch Set 4 : Another rebase #Patch Set 5 : Fall back from error to warning when mode is not set #Patch Set 6 : Added tests for H264 packetizer in mode 0 #Patch Set 7 : Remove death tests on Android and where they don't exist #Patch Set 8 : Upload for GN debugging #Patch Set 9 : Working H.264 test where packetization mode 0 is set #
Total comments: 30
Patch Set 10 : Addressed hbos comments #
Total comments: 3
Patch Set 11 : Revised RTP packetization code #Patch Set 12 : Added some small unit tests #Patch Set 13 : Fix death tests #Patch Set 14 : JNI bug modification #Patch Set 15 : Rebase #Patch Set 16 : Bot-detected compiler issues #
Total comments: 5
Patch Set 17 : Parameterized packet mode tests #Patch Set 18 : Rebase #
Total comments: 2
Patch Set 19 : Fixes #Patch Set 20 : OpenH264 fix #Patch Set 21 : Moved tests to minimize diff #
Total comments: 10
Patch Set 22 : Addressed mflodman review comments #Patch Set 23 : Use accessors for codecSpecific #Patch Set 24 : One more accessor #Patch Set 25 : Rebase #Patch Set 26 : Fix OpenH264 1.6 compilation error #Patch Set 27 : Upload try 2 (with rebase) #Messages
Total messages: 139 (91 generated)
Description was changed from ========== Merge branch 'master' of https://chromium.googlesource.com/external/webrtc into packetization-mode-0 Patching in CL 2009213002 BUG= ========== to ========== Implement H.264 packetization mode 0. This approach extends the H.264 specific information with a packetization mode enum. Status: Parameter is in code. No way to set it yet. Rebase of CL 2009213002 BUG=600254 ==========
The CQ bit was checked by hta@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/14171) 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 hta@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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11839) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19428) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9539)
The CQ bit was checked by hta@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: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by hta@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.
The CQ bit was checked by hta@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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11930) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/11952)
The CQ bit was checked by hta@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.
The CQ bit was checked by hta@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.
hta@webrtc.org changed reviewers: + hbos@webrtc.org, magjed@webrtc.org
This might be a good time to review this effort. Once the mechanism is working, we can connect it to SDP.
hbos@webrtc.org changed reviewers: + sprang@webrtc.org
+sprang: See comment where your name is mentioned. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:546: kH264PacketizationModeNotSet, Is this ever set? This enum is used in constructorless structs, if not set properly it will have a garbage value, not this one. The GetDefaultH264Settings uses kH264PacketizationMode1. Oh but perhaps we memset the entire struct to 0? If that is how it ends up being used I prefer to explicitly say "kH264PacketizationModeNotSet = 0," here to imply that its value being 0 does have significance. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:88: packetization_mode_ = kH264PacketizationMode1; From https://tools.ietf.org/html/rfc6184#section-6.2: "This mode is in use when the value of the OPTIONAL packetization-mode media type parameter is equal to 0 or the packetization-mode is not present." If not present in the SDP, mode 0 is used. Does it make more sense for the default to be Mode0 here as well or do we make sure to always set it if missing in the SDP? https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:174: RTC_CHECK(packetization_mode_ == kH264PacketizationMode1); nit: Here and other places in this file: RTC_CHECK_EQ https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:97: H264PacketizationMode packetization_mode_; Would it make more sense to store all parameters as a RTPVideoHeaderH264? If more parameters are added, wouldn't we just end up copying the members of RTPVideoHeaderH264 to here? https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codec_database.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codec_database.cc:68: h264_settings.packetization_mode = kH264PacketizationMode1; Similar question as earlier: Should the default be 0 or 1? If missing in the SDP, it should be 0? https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:206: kH264PacketizationModeNotSet); nit: RTC_DCHECK_NE https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:461: encoder_params.sSpatialLayers[0].sSliceArgument.uiSliceNum = 0; +sprang tried this and uiSliceNum != 1 resulted in borked rate control resulting in major quality regressions and having to revert I believe. This makes me afraid that we might run into the same problem if slicing happens due to uiSlizeSizeConstraint, but not sure if that is the same problem. Do you have anything to add, sprang? See https://codereview.webrtc.org/2458673002/diff2/1:20001/webrtc/modules/video_c... https://codereview.webrtc.org/2337453002/diff/160001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:393: TEST_F(EndToEndTest, SendsAndReceivesH264PacketizationMode0) { How big is a NAL and are we sure that this will trigger the packetization being different than for Mode1? https://codereview.webrtc.org/2337453002/diff/160001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:403: } How about a test that explicitly runs with Mode1 without assuming that that is the default?
https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:555: H264PacketizationMode packetization_mode; Since the packetization mode information is included in cricket::VideoCodec, I don't think we need to add it to the VideoCodecH264 struct as well. External encoders can now include packetization mode information in the cricket::VideoCodecs they return from 'std::vector<cricket::VideoCodec>& WebRtcVideoEncoderFactory::supported_codecs()', and that information will be passed back into 'webrtc::VideoEncoder* WebRtcVideoEncoderFactory::CreateVideoEncoder(const cricket::VideoCodec& codec)'. So the factory should be able to pass packetization mode information (as well as profile information) to the constructor of the webrtc::VideoEncoder, so it shouldn't be necessary to also add it to webrtc::VideoCodec which is passed to 'int32_t VideoEncoder::InitEncode(const webrtc::VideoCodec* codec_settings, int32_t number_of_cores, size_t max_payload_size)'. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:261: RTC_CHECK(packetization_mode_ == kH264PacketizationMode1); nit: use RTC_CHECK_EQ https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:46: static const H264PacketizationMode packetization_modes[] = { nit: This should be named kPacketizationModes. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:173: for (auto packetization_mode : packetization_modes) { Maybe it would be better to parameterize this test using TEST_P instead of manually looping. You can see https://codereview.webrtc.org/2406463002/diff2/20001:40001/webrtc/common_vide... for an example how it's done.
https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:461: encoder_params.sSpatialLayers[0].sSliceArgument.uiSliceNum = 0; On 2016/10/31 10:32:26, hbos wrote: > +sprang tried this and uiSliceNum != 1 resulted in borked rate control resulting > in major quality regressions and having to revert I believe. > This makes me afraid that we might run into the same problem if slicing happens > due to uiSlizeSizeConstraint, but not sure if that is the same problem. > > Do you have anything to add, sprang? > > See > https://codereview.webrtc.org/2458673002/diff2/1:20001/webrtc/modules/video_c... Not sure what will even happen when uiSliceMode = SM_SIZELIMITED_SLICE rather than SM_FIXEDSLCNUM_SLICE. It's simple enough to test though. Just run video_loopback with the desired parameters and see if it looks like crap.
PTAL hbos https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:546: kH264PacketizationModeNotSet, On 2016/10/31 10:32:26, hbos wrote: > Is this ever set? This enum is used in constructorless structs, if not set > properly it will have a garbage value, not this one. The GetDefaultH264Settings > uses kH264PacketizationMode1. > > Oh but perhaps we memset the entire struct to 0? If that is how it ends up being > used I prefer to explicitly say "kH264PacketizationModeNotSet = 0," here to > imply that its value being 0 does have significance. Good point about being explicit that it is 0. The struct VideoCodecH264 is initialized using memset() all over the place - the last change I did (going from DCHECK to LOG(WARNING) when it wasn't set) was because it's initialized from Java, where I'm unable to figure out exactly where it is initialized. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:88: packetization_mode_ = kH264PacketizationMode1; On 2016/10/31 10:32:26, hbos wrote: > From https://tools.ietf.org/html/rfc6184#section-6.2: > "This mode is in use when the value of the OPTIONAL packetization-mode media > type parameter is equal to 0 or the packetization-mode is not present." > > If not present in the SDP, mode 0 is used. Does it make more sense for the > default to be Mode0 here as well or do we make sure to always set it if missing > in the SDP? I started out with a RTC_CHECK here instead of the LOG(LS_WARNING). So I know all callers set it - EXCEPT for a few android java tests that have been added in the last few months - and I couldn't figure out which code path they were using to get the uninitialized value. So in the interest of not changing existing behavior, I made it LOG(WARNING) + a default of mode 1. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:174: RTC_CHECK(packetization_mode_ == kH264PacketizationMode1); On 2016/10/31 10:32:26, hbos wrote: > nit: Here and other places in this file: RTC_CHECK_EQ Acknowledged. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:97: H264PacketizationMode packetization_mode_; On 2016/10/31 10:32:26, hbos wrote: > Would it make more sense to store all parameters as a RTPVideoHeaderH264? If > more parameters are added, wouldn't we just end up copying the members of > RTPVideoHeaderH264 to here? Absolutely not. webrtc::RTPVideoHeaderH264 from modules/include/modules_common_types.h is information about one specific RTP packet and what NALUs it contains. It doesn't contain a pacetization mode. Perhaps you were thinking of some other struct? https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codec_database.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codec_database.cc:68: h264_settings.packetization_mode = kH264PacketizationMode1; On 2016/10/31 10:32:26, hbos wrote: > Similar question as earlier: Should the default be 0 or 1? If missing in the > SDP, it should be 0? The whole stack expects 1 at this time, and info from others (Cisco) indicates that the only people using mode 0 are some "special" (=old) devices, mostly telephone gateways. So keeping it at 1 is a lower risk. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:206: kH264PacketizationModeNotSet); On 2016/10/31 10:32:26, hbos wrote: > nit: RTC_DCHECK_NE Acknowledged. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:461: encoder_params.sSpatialLayers[0].sSliceArgument.uiSliceNum = 0; On 2016/10/31 10:32:26, hbos wrote: > +sprang tried this and uiSliceNum != 1 resulted in borked rate control resulting > in major quality regressions and having to revert I believe. > This makes me afraid that we might run into the same problem if slicing happens > due to uiSlizeSizeConstraint, but not sure if that is the same problem. > > Do you have anything to add, sprang? > > See > https://codereview.webrtc.org/2458673002/diff2/1:20001/webrtc/modules/video_c... In mode 0, the packet size is the important thing to control. In mode 1, I defer to sprang. My compare-codecs tests indicate that yes, there is borkedness in the rate control of openh264. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:393: TEST_F(EndToEndTest, SendsAndReceivesH264PacketizationMode0) { On 2016/10/31 10:32:26, hbos wrote: > How big is a NAL and are we sure that this will trigger the packetization being > different than for Mode1? The NALU limit is what's being set over in the h264_encoder_impl test. I'm not sure what the NALU sizes are; they range from small control NALUs to frame-containing NALUs. No, we're not sure this will trigger. It was the only test I could find that did actual data passing. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:403: } On 2016/10/31 10:32:26, hbos wrote: > How about a test that explicitly runs with Mode1 without assuming that that is > the default? Added.
Looks good. I'm curious about magjed's comment about packatization mode being included in cricket::VideoCodec, awaiting what happens with that. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:88: packetization_mode_ = kH264PacketizationMode1; On 2016/10/31 15:03:04, hta-webrtc wrote: > On 2016/10/31 10:32:26, hbos wrote: > > From https://tools.ietf.org/html/rfc6184#section-6.2: > > "This mode is in use when the value of the OPTIONAL packetization-mode media > > type parameter is equal to 0 or the packetization-mode is not present." > > > > If not present in the SDP, mode 0 is used. Does it make more sense for the > > default to be Mode0 here as well or do we make sure to always set it if > missing > > in the SDP? > > I started out with a RTC_CHECK here instead of the LOG(LS_WARNING). So I know > all callers set it - EXCEPT for a few android java tests that have been added in > the last few months - and I couldn't figure out which code path they were using > to get the uninitialized value. > > So in the interest of not changing existing behavior, I made it LOG(WARNING) + a > default of mode 1. Acknowledged. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:97: H264PacketizationMode packetization_mode_; On 2016/10/31 15:03:04, hta-webrtc wrote: > On 2016/10/31 10:32:26, hbos wrote: > > Would it make more sense to store all parameters as a RTPVideoHeaderH264? If > > more parameters are added, wouldn't we just end up copying the members of > > RTPVideoHeaderH264 to here? > > Absolutely not. webrtc::RTPVideoHeaderH264 from > modules/include/modules_common_types.h is information about one specific RTP > packet and what NALUs it contains. It doesn't contain a pacetization mode. > Perhaps you were thinking of some other struct? Uhm, confused. I'm thinking about this line: https://codereview.webrtc.org/2337453002/diff/180001/webrtc/modules/rtp_rtcp/... packetization_mode_ is set at construction from an RTPVideoHeaderH264. But yeah, I was thinking of a different struct not packet-specific. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codec_database.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codec_database.cc:68: h264_settings.packetization_mode = kH264PacketizationMode1; On 2016/10/31 15:03:04, hta-webrtc wrote: > On 2016/10/31 10:32:26, hbos wrote: > > Similar question as earlier: Should the default be 0 or 1? If missing in the > > SDP, it should be 0? > > The whole stack expects 1 at this time, and info from others (Cisco) indicates > that the only people using mode 0 are some "special" (=old) devices, mostly > telephone gateways. So keeping it at 1 is a lower risk. Acknowledged. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:461: encoder_params.sSpatialLayers[0].sSliceArgument.uiSliceNum = 0; On 2016/10/31 15:03:04, hta-webrtc wrote: > On 2016/10/31 10:32:26, hbos wrote: > > +sprang tried this and uiSliceNum != 1 resulted in borked rate control > resulting > > in major quality regressions and having to revert I believe. > > This makes me afraid that we might run into the same problem if slicing > happens > > due to uiSlizeSizeConstraint, but not sure if that is the same problem. > > > > Do you have anything to add, sprang? > > > > See > > > https://codereview.webrtc.org/2458673002/diff2/1:20001/webrtc/modules/video_c... > > In mode 0, the packet size is the important thing to control. In mode 1, I defer > to sprang. My compare-codecs tests indicate that yes, there is borkedness in the > rate control of openh264. > When merging this with master later there should be a conflict with sprang's CL and you can keep the values he sets. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/video/end_to_end_... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/video/end_to_end_... webrtc/video/end_to_end_tests.cc:393: TEST_F(EndToEndTest, SendsAndReceivesH264PacketizationMode0) { On 2016/10/31 15:03:04, hta-webrtc wrote: > On 2016/10/31 10:32:26, hbos wrote: > > How big is a NAL and are we sure that this will trigger the packetization > being > > different than for Mode1? > > The NALU limit is what's being set over in the h264_encoder_impl test. I'm not > sure what the NALU sizes are; they range from small control NALUs to > frame-containing NALUs. > > No, we're not sure this will trigger. It was the only test I could find that did > actual data passing. Acknowledged. https://codereview.webrtc.org/2337453002/diff/180001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/180001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:206: kH264PacketizationModeNotSet); nit: RTC_DCHECK_NE https://codereview.webrtc.org/2337453002/diff/180001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2337453002/diff/180001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:200: specifics.codecType = kVideoCodecH264; nit: Same line twice here.
Asking for more guidance to understand magjed's suggestion. https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:555: H264PacketizationMode packetization_mode; On 2016/10/31 13:00:50, magjed_webrtc wrote: > Since the packetization mode information is included in cricket::VideoCodec, I > don't think we need to add it to the VideoCodecH264 struct as well. External > encoders can now include packetization mode information in the > cricket::VideoCodecs they return from 'std::vector<cricket::VideoCodec>& > WebRtcVideoEncoderFactory::supported_codecs()', and that information will be > passed back into > 'webrtc::VideoEncoder* WebRtcVideoEncoderFactory::CreateVideoEncoder(const > cricket::VideoCodec& codec)'. So the factory should be able to pass > packetization mode information (as well as profile information) to the > constructor of the webrtc::VideoEncoder, so it shouldn't be necessary to also > add it to webrtc::VideoCodec which is passed to 'int32_t > VideoEncoder::InitEncode(const webrtc::VideoCodec* codec_settings, int32_t > number_of_cores, size_t max_payload_size)'. Now I'm somewhat confused..... what code path can we use to get the information contained in a cricket::VideoCodec to the places where we need to have access to that information? The points we need the info are: - video_coding/codecs/h264/h264_encoder_impl.cc (to set the encoder parameters) - rtp_format_h264.cc (to check that we always take the single NALU path when mode 0 is in effect) Which structure that is visible from these 2 places (or the functions that construct them) contains a cricket::VideoCodec? https://codereview.webrtc.org/2337453002/diff/180001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet.cc (right): https://codereview.webrtc.org/2337453002/diff/180001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet.cc:134: } Note to self: This looks like it should be reverted.
On 2016/10/31 20:54:52, hta-webrtc wrote: > Asking for more guidance to understand magjed's suggestion. > > https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h > File webrtc/common_types.h (right): > > https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h#ne... > webrtc/common_types.h:555: H264PacketizationMode packetization_mode; > On 2016/10/31 13:00:50, magjed_webrtc wrote: > > Since the packetization mode information is included in cricket::VideoCodec, I > > don't think we need to add it to the VideoCodecH264 struct as well. External > > encoders can now include packetization mode information in the > > cricket::VideoCodecs they return from 'std::vector<cricket::VideoCodec>& > > WebRtcVideoEncoderFactory::supported_codecs()', and that information will be > > passed back into > > 'webrtc::VideoEncoder* WebRtcVideoEncoderFactory::CreateVideoEncoder(const > > cricket::VideoCodec& codec)'. So the factory should be able to pass > > packetization mode information (as well as profile information) to the > > constructor of the webrtc::VideoEncoder, so it shouldn't be necessary to also > > add it to webrtc::VideoCodec which is passed to 'int32_t > > VideoEncoder::InitEncode(const webrtc::VideoCodec* codec_settings, int32_t > > number_of_cores, size_t max_payload_size)'. > > Now I'm somewhat confused..... what code path can we use to get the information > contained in a cricket::VideoCodec to the places where we need to have access to > that information? > > The points we need the info are: > - video_coding/codecs/h264/h264_encoder_impl.cc (to set the encoder parameters) > - rtp_format_h264.cc (to check that we always take the single NALU path when > mode 0 is in effect) > > Which structure that is visible from these 2 places (or the functions that > construct them) contains a cricket::VideoCodec? Trying to parse this again..... you say "add it to webrtc::VideoCodec". VideoCodecH264 is a member of an union that is part of webrtc::VideoCodec. So to me (second reading), it seems that you're saying that this CL should do exactly what it does, but that a second CL should make sure a cricket::VideoCodec is used to pass the information into the constructor instead of a webrtc::VideoCodec being passed to InitCodec. Is that what you are saying?
https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2337453002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:555: H264PacketizationMode packetization_mode; On 2016/10/31 20:54:52, hta-webrtc wrote: > On 2016/10/31 13:00:50, magjed_webrtc wrote: > > Since the packetization mode information is included in cricket::VideoCodec, I > > don't think we need to add it to the VideoCodecH264 struct as well. External > > encoders can now include packetization mode information in the > > cricket::VideoCodecs they return from 'std::vector<cricket::VideoCodec>& > > WebRtcVideoEncoderFactory::supported_codecs()', and that information will be > > passed back into > > 'webrtc::VideoEncoder* WebRtcVideoEncoderFactory::CreateVideoEncoder(const > > cricket::VideoCodec& codec)'. So the factory should be able to pass > > packetization mode information (as well as profile information) to the > > constructor of the webrtc::VideoEncoder, so it shouldn't be necessary to also > > add it to webrtc::VideoCodec which is passed to 'int32_t > > VideoEncoder::InitEncode(const webrtc::VideoCodec* codec_settings, int32_t > > number_of_cores, size_t max_payload_size)'. > > Now I'm somewhat confused..... what code path can we use to get the information > contained in a cricket::VideoCodec to the places where we need to have access to > that information? > > The points we need the info are: > - video_coding/codecs/h264/h264_encoder_impl.cc (to set the encoder parameters) > - rtp_format_h264.cc (to check that we always take the single NALU path when > mode 0 is in effect) > > Which structure that is visible from these 2 places (or the functions that > construct them) contains a cricket::VideoCodec? > To clarify, I don't think we need to add packetization mode to webrtc::VideoCodec (via VideoCodecUnion/VideoCodecH264), because it should be redundant if we pass the cricket::VideoCodec to all encoder constructors. To get cricket::VideoCodec to video_coding/codecs/h264/h264_encoder_impl.cc, we need to pass the cricket::VideoCodec we have selected in WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder all the way down to H264EncoderImpl::H264EncoderImpl. I have been planning to do this change anyway, so that we can send the profile information to H264EncoderImpl. More concretely, the essential change is this: diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder( const VideoCodec& codec) { - if (type == webrtc::kVideoCodecVP8) { - return AllocatedEncoder( - webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kVp8), type, false); - } else if (type == webrtc::kVideoCodecVP9) { - return AllocatedEncoder( - webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kVp9), type, false); - } else if (type == webrtc::kVideoCodecH264) { - return AllocatedEncoder( - webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kH264), type, false); - } + // Try creating internal encoder. + if (webrtc::VideoEncoder::IsSupportedSoftware(type)) + return AllocatedEncoder(webrtc::VideoEncoder::Create(codec), codec, true); diff --git a/webrtc/video/video_encoder.cc b/webrtc/video/video_encoder.cc -VideoEncoder* VideoEncoder::Create(VideoEncoder::EncoderType codec_type) { +VideoEncoder* VideoEncoder::Create(const cricket::VideoCodec& codec) { + const VideoCodecType codec_type = CodecTypeFromName(codec.name); RTC_DCHECK(IsSupportedSoftware(codec_type)); switch (codec_type) { - case kH264: - return H264Encoder::Create(); - case kVp8: - return VP8Encoder::Create(); - case kVp9: - return VP9Encoder::Create(); - case kUnsupportedCodec: + case kVideoCodecH264: + return H264Encoder::Create(codec); + case kVideoCodecVP8: + return VP8Encoder::Create(codec); + case kVideoCodecVP9: + return VP9Encoder::Create(codec); + default: RTC_NOTREACHED(); return nullptr; } - RTC_NOTREACHED(); - return nullptr; } - static VideoEncoder* Create(EncoderType codec_type); + static VideoEncoder* Create(const cricket::VideoCodec& codec); diff --git a/webrtc/modules/video_coding/codecs/h264/h264.cc b/webrtc/modules/video_coding/codecs/h264/h264.cc -H264Encoder* H264Encoder::Create() { +H264Encoder* H264Encoder::Create(const cricket::VideoCodec& codec) { RTC_DCHECK(H264Encoder::IsSupported()); #if defined(WEBRTC_IOS) && defined(WEBRTC_VIDEO_TOOLBOX_SUPPORTED) if (IsH264CodecSupportedObjC()) { @@ -71,7 +71,7 @@ H264Encoder* H264Encoder::Create() { #if defined(WEBRTC_USE_H264) RTC_CHECK(g_rtc_use_h264); LOG(LS_INFO) << "Creating H264EncoderImpl."; - return new H264EncoderImpl(); + return new H264EncoderImpl(codec); #else RTC_NOTREACHED(); return nullptr; diff --git a/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc b/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc -H264EncoderImpl::H264EncoderImpl() +H264EncoderImpl::H264EncoderImpl(const cricket::VideoCodec& codec) : openh264_encoder_(nullptr), number_of_cores_(0), encoded_image_callback_(nullptr),
On 2016/11/01 19:23:38, magjed_webrtc wrote: .... > diff --git a/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc > b/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc > -H264EncoderImpl::H264EncoderImpl() > +H264EncoderImpl::H264EncoderImpl(const cricket::VideoCodec& codec) > : openh264_encoder_(nullptr), > number_of_cores_(0), > encoded_image_callback_(nullptr), This is where one more piece is missing before we can avoid adding packetization_mode to VideoCodec: Should we then change the type of H264EncoderImpl::codec_settings_ from webrtc::VideoCodec to something else - either cricket::VideoCodec or an even more specific parameter container? Changing this type gets rid of one usage of webrtc::VideoCodec that seems fairly isolated; this may be a Good Thing. Webrtc::VideoCodec is also used in InitEncode, which is called in a number of places, including VideoEncoderSoftwareFallbackWrapper and simulcast_encoder_adapter. Either this can modify the packetization mode or it can't; if it can't, that simplifies things. It would be cleanest if we could move all 3 parameters of InitEncoder onto the constructor and remove InitEncoder altogether; I'm not sure that's simple, and it's certainly a larger refactoring. Might be worth doing.
On 2016/11/02 02:07:35, hta-webrtc wrote: > On 2016/11/01 19:23:38, magjed_webrtc wrote: > > .... > > > diff --git a/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc > > b/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc > > -H264EncoderImpl::H264EncoderImpl() > > +H264EncoderImpl::H264EncoderImpl(const cricket::VideoCodec& codec) > > : openh264_encoder_(nullptr), > > number_of_cores_(0), > > encoded_image_callback_(nullptr), > > This is where one more piece is missing before we can avoid adding > packetization_mode to VideoCodec: Should we then change the type of > H264EncoderImpl::codec_settings_ from webrtc::VideoCodec to something else - > either cricket::VideoCodec or an even more specific parameter container? > > Changing this type gets rid of one usage of webrtc::VideoCodec that seems fairly > isolated; this may be a Good Thing. > > Webrtc::VideoCodec is also used in InitEncode, which is called in a number of > places, including VideoEncoderSoftwareFallbackWrapper and > simulcast_encoder_adapter. Either this can modify the packetization mode or it > can't; if it can't, that simplifies things. > > It would be cleanest if we could move all 3 parameters of InitEncoder onto the > constructor and remove InitEncoder altogether; I'm not sure that's simple, and > it's certainly a larger refactoring. Might be worth doing. I was more thinking that the encoders could either: - Store the whole cricket::VideoCodec they receive in a member variable. or - Extract the information they want from cricket::VideoCodec in the ctor and store in separate 'const H264PacketizationMode packetization_mode_', 'const H264::Profile profile_', etc. I don't feel strongly about this btw, so I'm fine with storing packetization mode in VideoCodecH264 if that's the easiest way forward. I don't think we should spend time refactoring InitEncoder at this point.
The CQ bit was checked by hta@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_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/19087) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/15808) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/19785)
The CQ bit was checked by hta@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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/19245)
The CQ bit was checked by hta@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: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/18068)
The CQ bit was checked by hta@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_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/2071) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12198) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14534) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14416) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19767) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18451)
The CQ bit was checked by hta@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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/18284)
The CQ bit was checked by hta@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.
PTAL Changes after offline discussion. I've tested with video_loopback + a non-submitted patch to the default that the change is effective, and the picture survives. If magjed's refactoring of encoder creation lands first, this one will adapt.
lgtm https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:14: nit: revert this empty line https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:173: for (auto packetization_mode : packetization_modes) { I'm reposting one of my previous comments that I think was missed: Maybe it would be better to parameterize this test using TEST_P instead of manually looping. You can see https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... for an example how it's done. The benefit of such a change is that if/when the test fails, you can immediately see from the test failure what packetization_mode was used.
The CQ bit was checked by hta@webrtc.org
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9829)
hta@webrtc.org changed reviewers: + mflodman@webrtc.org
Adding mflodman@ for OWNERS review.
lgtm
The CQ bit was checked by hta@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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
The CQ bit was checked by hta@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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/15928)
Harald, What parts do you want me to look at? It's a big CL and it would be good to know what to limit my review to.
Harald, What parts do you want me to look at? It's a big CL and it would be good to know what to limit my review to.
On 2016/11/08 11:54:36, mflodman wrote:
> Harald,
> What parts do you want me to look at? It's a big CL and it would be good to
know
> what to limit my review to.
I believe it has been adequately reviewed, but magjed is not an OWNER for
webrtc/ - you are.
In particular, presubmit says:
Missing LGTM from an OWNER for these files:
webrtc/common_types.h
webrtc/modules/include/module_common_types.h
webrtc/modules/rtp_rtcp/source/rtp_format.cc
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc
webrtc/modules/video_coding/codec_database.cc
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl_unittest.cc
webrtc/modules/video_coding/include/video_codec_interface.h
webrtc/test/fake_encoder.cc
webrtc/video/end_to_end_tests.cc
webrtc/video/payload_router.cc
webrtc/video/video_send_stream_tests.cc
https://codereview.webrtc.org/2337453002/diff/340001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/340001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:474: encoder_params.sSpatialLayers[0].sSliceArgument.uiSliceNum = 0; Should probably set this to 1, see below.
I'm still waiting for replies to some of my comments. BTW, it looks like you have committed this file: webrtc/modules/rtp_rtcp/source/#rtp_format_h264.cc# to the CL.
On 2016/11/09 08:48:32, magjed_webrtc wrote: > I'm still waiting for replies to some of my comments. The TEST_P is added. I'll get the blank line while removing the # file. > BTW, it looks like you have committed this file: > webrtc/modules/rtp_rtcp/source/#rtp_format_h264.cc# > to the CL. Oops.
Addressed comments. https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:14: On 2016/11/04 13:57:05, magjed_webrtc wrote: > nit: revert this empty line Done. https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:173: for (auto packetization_mode : packetization_modes) { On 2016/11/04 13:57:05, magjed_webrtc wrote: > I'm reposting one of my previous comments that I think was missed: > Maybe it would be better to parameterize this test using TEST_P instead of > manually looping. You can see > https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... > for an example how it's done. > > The benefit of such a change is that if/when the test fails, you can immediately > see from the test failure what packetization_mode was used. Done. Not enjoying the printout format (it prints the index not the value), but it makes the tests easier to read.
The CQ bit was checked by hta@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_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/2196)
https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:173: for (auto packetization_mode : packetization_modes) { On 2016/11/09 09:27:10, hta-webrtc wrote: > On 2016/11/04 13:57:05, magjed_webrtc wrote: > > I'm reposting one of my previous comments that I think was missed: > > Maybe it would be better to parameterize this test using TEST_P instead of > > manually looping. You can see > > > https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... > > for an example how it's done. > > > > The benefit of such a change is that if/when the test fails, you can > immediately > > see from the test failure what packetization_mode was used. > > Done. > Not enjoying the printout format (it prints the index not the value), but it > makes the tests easier to read. > When the tests run, it will just print the index, but if something fails, it will print out the actual value of the parameters. So it will look something like this: [ RUN ] PacketMode/RtpPacketizerH264ModeTest.TestSingleNalu/1 ... Failure ... [ FAILED ] PacketMode/RtpPacketizerH264ModeTest.TestSingleNalu/1, where GetParam() = 2 Since your parameter is an enum, it will just print out the integer value, which is not super helpful, but still helpful. Please move the tests back to this position to keep the diff small.
On 2016/11/09 15:18:26, magjed_webrtc wrote: > https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): > > https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:173: for (auto > packetization_mode : packetization_modes) { > On 2016/11/09 09:27:10, hta-webrtc wrote: > > On 2016/11/04 13:57:05, magjed_webrtc wrote: > > > I'm reposting one of my previous comments that I think was missed: > > > Maybe it would be better to parameterize this test using TEST_P instead of > > > manually looping. You can see > > > > > > https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... > > > for an example how it's done. > > > > > > The benefit of such a change is that if/when the test fails, you can > > immediately > > > see from the test failure what packetization_mode was used. > > > > Done. > > Not enjoying the printout format (it prints the index not the value), but it > > makes the tests easier to read. > > > > When the tests run, it will just print the index, but if something fails, it > will print out the actual value of the parameters. So it will look something > like this: > [ RUN ] PacketMode/RtpPacketizerH264ModeTest.TestSingleNalu/1 > ... Failure ... > [ FAILED ] PacketMode/RtpPacketizerH264ModeTest.TestSingleNalu/1, where > GetParam() = 2 > Since your parameter is an enum, it will just print out the integer value, which > is not super helpful, but still helpful. > > Please move the tests back to this position to keep the diff small. The reason for the move is that the TEST_P tests have to inherit from a common base (fixture), which the TEST tests don't have to do that. In other tests I've written, I've grouped tests with a common base together, and grouped tests with no base together separately from the tests with a base fixture. Do you think it's worth breaking this principle to keep the diff small?
https://codereview.webrtc.org/2337453002/diff/340001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/340001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:474: encoder_params.sSpatialLayers[0].sSliceArgument.uiSliceNum = 0; On 2016/11/08 13:55:28, språng wrote: > Should probably set this to 1, see below. Done.
On 2016/11/09 15:41:23, hta-webrtc wrote: > On 2016/11/09 15:18:26, magjed_webrtc wrote: > > > https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... > > File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): > > > > > https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... > > webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:173: for (auto > > packetization_mode : packetization_modes) { > > On 2016/11/09 09:27:10, hta-webrtc wrote: > > > On 2016/11/04 13:57:05, magjed_webrtc wrote: > > > > I'm reposting one of my previous comments that I think was missed: > > > > Maybe it would be better to parameterize this test using TEST_P instead of > > > > manually looping. You can see > > > > > > > > > > https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... > > > > for an example how it's done. > > > > > > > > The benefit of such a change is that if/when the test fails, you can > > > immediately > > > > see from the test failure what packetization_mode was used. > > > > > > Done. > > > Not enjoying the printout format (it prints the index not the value), but it > > > makes the tests easier to read. > > > > > > > When the tests run, it will just print the index, but if something fails, it > > will print out the actual value of the parameters. So it will look something > > like this: > > [ RUN ] PacketMode/RtpPacketizerH264ModeTest.TestSingleNalu/1 > > ... Failure ... > > [ FAILED ] PacketMode/RtpPacketizerH264ModeTest.TestSingleNalu/1, where > > GetParam() = 2 > > Since your parameter is an enum, it will just print out the integer value, > which > > is not super helpful, but still helpful. > > > > Please move the tests back to this position to keep the diff small. > > The reason for the move is that the TEST_P tests have to inherit from a common > base (fixture), which the TEST tests don't have to do that. In other tests I've > written, I've grouped tests with a common base together, and grouped tests with > no base together separately from the tests with a base fixture. > > Do you think it's worth breaking this principle to keep the diff small? But even if you don't move the tests, the TEST_P/TEST/TEST_F would be grouped together since the new TEST_P are the first two tests in the file. Why does the TEST group have to precede the TEST_P group? I don't like bundling moving code with code changes if it's not necessary since it's more or less impossible to review the code changes.
On 2016/11/09 21:13:42, magjed_webrtc wrote: > On 2016/11/09 15:41:23, hta-webrtc wrote: > > On 2016/11/09 15:18:26, magjed_webrtc wrote: > > > > > > https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... > > > File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): > > > > > > > > > https://codereview.webrtc.org/2337453002/diff/300001/webrtc/modules/rtp_rtcp/... > > > webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:173: for (auto > > > packetization_mode : packetization_modes) { > > > On 2016/11/09 09:27:10, hta-webrtc wrote: > > > > On 2016/11/04 13:57:05, magjed_webrtc wrote: > > > > > I'm reposting one of my previous comments that I think was missed: > > > > > Maybe it would be better to parameterize this test using TEST_P instead > of > > > > > manually looping. You can see > > > > > > > > > > > > > > > https://codereview.webrtc.org/2337453002/diff/160001/webrtc/modules/rtp_rtcp/... > > > > > for an example how it's done. > > > > > > > > > > The benefit of such a change is that if/when the test fails, you can > > > > immediately > > > > > see from the test failure what packetization_mode was used. > > > > > > > > Done. > > > > Not enjoying the printout format (it prints the index not the value), but > it > > > > makes the tests easier to read. > > > > > > > > > > When the tests run, it will just print the index, but if something fails, it > > > will print out the actual value of the parameters. So it will look something > > > like this: > > > [ RUN ] PacketMode/RtpPacketizerH264ModeTest.TestSingleNalu/1 > > > ... Failure ... > > > [ FAILED ] PacketMode/RtpPacketizerH264ModeTest.TestSingleNalu/1, where > > > GetParam() = 2 > > > Since your parameter is an enum, it will just print out the integer value, > > which > > > is not super helpful, but still helpful. > > > > > > Please move the tests back to this position to keep the diff small. > > > > The reason for the move is that the TEST_P tests have to inherit from a common > > base (fixture), which the TEST tests don't have to do that. In other tests > I've > > written, I've grouped tests with a common base together, and grouped tests > with > > no base together separately from the tests with a base fixture. > > > > Do you think it's worth breaking this principle to keep the diff small? > > But even if you don't move the tests, the TEST_P/TEST/TEST_F would be grouped > together since the new TEST_P are the first two tests in the file. Why does the > TEST group have to precede the TEST_P group? > > I don't like bundling moving code with code changes if it's not necessary since > it's more or less impossible to review the code changes. OK, that's compelling enough. Moved to top. That's where all tests that should succeed no matter what the packetization mode is should go, anyway. Diff | wc -l reduced from 276 to 209 lines.
lgtm
A few comments, LGTM with these addressed. https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/include/m... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/include/m... webrtc/modules/include/module_common_types.h:284: H264PacketizationMode packetization_mode; This is similar to H264PacketizationTypes above, but only used when packetizing. It would be good with a comment describing what this is. It would also be good with a comment about H264PacketizationTypes then, since that is only used for depacketizing. https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format.cc (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format.cc:11: #include <memory> Do we really need to include memory here? https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:15: #include <memory> Do we need the memory include here? https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:467: static_cast<unsigned int>(max_payload_size_ - 50); Why the -50? Can you add comment or const. https://codereview.webrtc.org/2337453002/diff/400001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:203: specifics.codecType = kVideoCodecH264; Repeated line, remove.
Last chance to take another look.... https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/include/m... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/include/m... webrtc/modules/include/module_common_types.h:284: H264PacketizationMode packetization_mode; On 2016/11/10 12:52:39, mflodman wrote: > This is similar to H264PacketizationTypes above, but only used when packetizing. > It would be good with a comment describing what this is. > > It would also be good with a comment about H264PacketizationTypes then, since > that is only used for depacketizing. The enum definitions contain a somewhat longer description of what the fields are used for. I tried to add words that explain what they are used for in this particular context, without repeating too much from the enum definitions. See if it looks good. https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format.cc (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format.cc:11: #include <memory> On 2016/11/10 12:52:39, mflodman wrote: > Do we really need to include memory here? The presubmit gods did not complain here. https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:15: #include <memory> On 2016/11/10 12:52:39, mflodman wrote: > Do we need the memory include here? The presubmit script says: rtp_format_h264.h:123: Add #include <memory> for unique_ptr<> [build/include_what_you_use] [4] So in order to keep the presubmit gods happy.... yes. https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:467: static_cast<unsigned int>(max_payload_size_ - 50); On 2016/11/10 12:52:39, mflodman wrote: > Why the -50? Can you add comment or const. It is explained in the comment 3 lines above - and it's a 3-line statement. I hope we can delete it when we're sure that the 1.6 version of Openh264 sticks. https://codereview.webrtc.org/2337453002/diff/400001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2337453002/diff/400001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:203: specifics.codecType = kVideoCodecH264; On 2016/11/10 12:52:39, mflodman wrote: > Repeated line, remove. Done.
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org, magjed@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2337453002/#ps420001 (title: "Addressed mflodman review 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: ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19971) linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, hbos@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2337453002/#ps440001 (title: "Use accessors for codecSpecific")
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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12403) ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/2282) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14753) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14631) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19986) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18680)
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, hbos@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2337453002/#ps460001 (title: "One more accessor")
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: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...) ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12404) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12418) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14754) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14632) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19987) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, hbos@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2337453002/#ps480001 (title: "Rebase")
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: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, hbos@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2337453002/#ps500001 (title: "Fix OpenH264 1.6 compilation error")
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: android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)
The CQ bit was checked by hta@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, hbos@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2337453002/#ps520001 (title: "Upload try 2 (with rebase)")
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_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/3053)
The CQ bit was checked by hta@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.
Description was changed from ========== Implement H.264 packetization mode 0. This approach extends the H.264 specific information with a packetization mode enum. Status: Parameter is in code. No way to set it yet. Rebase of CL 2009213002 BUG=600254 ========== to ========== Implement H.264 packetization mode 0. This approach extends the H.264 specific information with a packetization mode enum. Status: Parameter is in code. No way to set it yet. Rebase of CL 2009213002 BUG=600254 ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== Implement H.264 packetization mode 0. This approach extends the H.264 specific information with a packetization mode enum. Status: Parameter is in code. No way to set it yet. Rebase of CL 2009213002 BUG=600254 ========== to ========== Implement H.264 packetization mode 0. This approach extends the H.264 specific information with a packetization mode enum. Status: Parameter is in code. No way to set it yet. Rebase of CL 2009213002 BUG=600254 Committed: https://crrev.com/3bba101f36483b8030a693dfbc93af736d1dba68 Cr-Commit-Position: refs/heads/master@{#15032} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/3bba101f36483b8030a693dfbc93af736d1dba68 Cr-Commit-Position: refs/heads/master@{#15032}
Message was sent while issue was closed.
A revert of this CL (patchset #27 id:520001) has been created in https://codereview.webrtc.org/2500743002/ by magjed@webrtc.org. The reason for reverting is: Broke a lot of tests in chromium.webrtc browser_tests. See e.g. https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/62228 and https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/30102. [ RUN ] WebRtcVideoQualityBrowserTests/WebRtcVideoQualityBrowserTest.MANUAL_TestVideoQualityH264/1 ... # # Fatal error in e:\b\c\b\win_builder\src\third_party\webrtc\modules\rtp_rtcp\source\rtp_format_h264.cc, line 170 # last system error: 0 # Check failed: packetization_mode_ == kH264PacketizationMode1 (0 vs. 2) # . |
