H.264 packetization mode 0 (try 3)

This approach passes packetization mode to the encoder as part of
a cricket::VideoCodec structure, rather than as part of struct VideoCodecH264 inside webrtc::VideoCodec.

BUG=600254
Committed: https://crrev.com/e59647b991f61cf1cf61b020356705e6c0f81257
Cr-Commit-Position: refs/heads/master@{#15437}

[hta-webrtc, 2016-11-28 09:00:05]

Created Reland of H.264 packetization mode 0 (try 2)

[hta-webrtc, 2016-11-28 15:11:21]

Description was changed from

==========
Reland of H.264 packetization mode 0 (try 2) (patchset #1 id:1 of
https://codereview.webrtc.org/2500743002/ )

Reason for revert:
Going to rework this approach a bit based on other changes.


Original issue's description:
> Revert of H.264 packetization mode 0 (try 2) (patchset #27 id:520001 of
https://codereview.webrtc.org/2337453002/ )
> 
> Reason for revert:
> 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)
> #
> 
> Original issue's description:
> > 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}
> 
> TBR=hbos@webrtc.org,sprang@webrtc.org,mflodman@webrtc.org,hta@webrtc.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=600254
> NOPRESUBMIT=true
> 
> Committed: https://crrev.com/13ceeeadfc81a7ab73f212ebf960c3cc1bfca034
> Cr-Commit-Position: refs/heads/master@{#15050}

TBR=hbos@webrtc.org,sprang@webrtc.org,mflodman@webrtc.org,magjed@webrtc.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=600254
==========

to

==========
This approach passes packetization mode to the encoder as part of
a cricket::VideoCodec structure, rather than as part of struct VideoCodecH264
inside webrtc::VideoCodec.

> > 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}
> 
> TBR=hbos@webrtc.org,sprang@webrtc.org,mflodman@webrtc.org,hta@webrtc.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=600254
> NOPRESUBMIT=true
> 
> Committed: https://crrev.com/13ceeeadfc81a7ab73f212ebf960c3cc1bfca034
> Cr-Commit-Position: refs/heads/master@{#15050}

TBR=hbos@webrtc.org,sprang@webrtc.org,mflodman@webrtc.org,magjed@webrtc.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=600254
==========

[hta-webrtc, 2016-11-28 15:11:45]

Description was changed from

==========
This approach passes packetization mode to the encoder as part of
a cricket::VideoCodec structure, rather than as part of struct VideoCodecH264
inside webrtc::VideoCodec.

> > 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}
> 
> TBR=hbos@webrtc.org,sprang@webrtc.org,mflodman@webrtc.org,hta@webrtc.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=600254
> NOPRESUBMIT=true
> 
> Committed: https://crrev.com/13ceeeadfc81a7ab73f212ebf960c3cc1bfca034
> Cr-Commit-Position: refs/heads/master@{#15050}

TBR=hbos@webrtc.org,sprang@webrtc.org,mflodman@webrtc.org,magjed@webrtc.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=600254
==========

to

==========
This approach passes packetization mode to the encoder as part of
a cricket::VideoCodec structure, rather than as part of struct VideoCodecH264
inside webrtc::VideoCodec.

BUG=600254
==========

[hta-webrtc, 2016-11-28 18:21:09]

The CQ bit was checked by hta@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 18:21:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-28 18:22:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 18:22:28]

Dry run: 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/12852)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12873)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15225)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15084)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20441)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19155)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10718)

[hta-webrtc, 2016-11-28 18:42:35]

The CQ bit was checked by hta@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 18:42:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-28 18:53:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 18:53:37]

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/19855)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/20045)

[hta-webrtc, 2016-11-29 08:26:17]

The CQ bit was checked by hta@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 08:26:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-29 08:43:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 08:43:25]

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/18813)

[hta-webrtc, 2016-12-01 09:20:42]

The CQ bit was checked by hta@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 09:20:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hta-webrtc, 2016-12-01 09:21:07]

This should be ready for final review now.

[commit-bot: I haz the power, 2016-12-01 09:42:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 09:42:32]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2016-12-01 16:58:57]

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/media/base/codec....
webrtc/media/base/codec.cc:119: }
Why was this moved here?

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:268: kH264PacketizationMode0      
// Only single NALU allowed
nit: Drop H264Packetization from the names of the actual values and define the
type of the enum. Personally I'd even prefer an enum class, but up to you.
Is there a more descriptive name than "mode 1" btw? If not, maybe name it
kModeOne, just so it doesn't look like kModel.

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format.cc:27: assert(rtp_type_header !=
NULL);
nit: RTC_DCHECK(rtp_type_header != nullptr);
here and below

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format.cc:39: assert(false);
RTC_NOTREACHED();

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:170:
RTC_CHECK_EQ(packetization_mode_, kH264PacketizationMode1);
If you use enum class you can add switch statement here and get a compilation
error if the model is not covered by a case.

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:264: // This is the
same setup as for the TestStapA test.
Then maybe move the setup to a helper in RtpPacketizerH264ModeTest?

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:66: }
Can we get around this somehow?

[hta-webrtc, 2016-12-01 18:26:28]

See if this looks better. Enum class looks OK.
(just wish I could add a toString method on it...)

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/media/base/codec....
webrtc/media/base/codec.cc:119: }
On 2016/12/01 16:58:56, språng wrote:
> Why was this moved here?

Because it was the simplest way to set a default value for packetization mode
that would apply everywhere the cricket::Codec object was created by its
constructor (which is most places).

It did not cover all cases, unfortunately (see the comment next to the value
assignment for the packetization mode enum).

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:268: kH264PacketizationMode0      
// Only single NALU allowed
On 2016/12/01 16:58:57, språng wrote:
> nit: Drop H264Packetization from the names of the actual values and define the
> type of the enum. Personally I'd even prefer an enum class, but up to you.
> Is there a more descriptive name than "mode 1" btw? If not, maybe name it
> kModeOne, just so it doesn't look like kModel.

Heh. They're called "Single NAL Unit Mode" (0) and "Non-Interleaved Mode" (1).
I'll use those names.

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format.cc:27: assert(rtp_type_header !=
NULL);
On 2016/12/01 16:58:57, språng wrote:
> nit: RTC_DCHECK(rtp_type_header != nullptr);
> here and below

RTC_DCHECK(rtp_type_header)?
As you can tell, this is old code :-)

done.

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format.cc:39: assert(false);
On 2016/12/01 16:58:57, språng wrote:
> RTC_NOTREACHED();

Done.

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:170:
RTC_CHECK_EQ(packetization_mode_, kH264PacketizationMode1);
On 2016/12/01 16:58:57, språng wrote:
> If you use enum class you can add switch statement here and get a compilation
> error if the model is not covered by a case.

Done.

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:264: // This is the
same setup as for the TestStapA test.
On 2016/12/01 16:58:57, språng wrote:
> Then maybe move the setup to a helper in RtpPacketizerH264ModeTest?

It's a bit ugly. Started moving stuff.

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:66: }
On 2016/12/01 16:58:57, språng wrote:
> Can we get around this somehow?

We could expose PacketizationModeEqualsForTesting(mode) (returning
packetization_mode_ == mode), but I dont' see how it makes either the test or
the code more readable.

I don't like friend classes.

[hta-webrtc, 2016-12-01 18:28:04]

The CQ bit was checked by hta@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 18:28:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-01 18:51:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 18:51:27]

Dry run: This issue passed the CQ dry run.

[hbos, 2016-12-02 10:21:27]

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:297: H264PacketizationMode
packetization_mode;
(Thinking its strange that this information is in the header when it needs to be
what has been negotiated elsewhere, but guessing any other way would be much
more difficult/require refactoring.)

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:178: break;
case default:
  RTC_NOTREACHED();

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:276:
RTC_CHECK(packetization_mode_ == H264PacketizationMode::NonInterleaved);
nit: RTC_CHECK_EQ

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:270:
TEST(RtpPacketizerH264Test, TestMode0HasNoStapA) {
nit/optional: Naming difference "mode" vs enum name in unittest names

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:438: const size_t
kFrameSize = kMaxPayloadSize + 100;
nit: I prefer to test the edge case + 1

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:484: // but is not
exported.
Make this a constant of our own, e.g. kNalHeaderSize, with this comment above
it.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:493: // it with cpu
core number.
This comment does not match with the code. I think you meant to remove it? And
say "Limit the size of the packets produced." maybe.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:498:
static_cast<unsigned int>(max_payload_size_);
Should this not be max_payload_size_ - kNalHeaderSize like above?
Should we set uiMaxNalSize (or the like) like above?

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:512:
SM_FIXEDSLCNUM_SLICE;
Given this TODO, why is this code different from the above #endif?

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl_unittest.cc
(right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl_unittest.cc:39:
encoder.InitEncode(&codec_settings, 1, kMaxPayloadSize));
nit: kNumCodes = 1

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl_unittest.cc:44:
TEST(H264EncoderImplTest, CanInitializeWithPacketizationMode1Explicitly) {
nit/optional: Naming difference "mode" vs enum name in unittest names

[sprang_webrtc, 2016-12-02 10:52:29]

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:178: break;
On 2016/12/02 10:21:26, hbos wrote:
> case default:
>   RTC_NOTREACHED();

I disagree. If you have an enum class with all values covered, leave default out
so you get a compile time error instead of runtime one.

[hbos, 2016-12-02 11:04:45]

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:178: break;
On 2016/12/02 10:52:29, språng wrote:
> On 2016/12/02 10:21:26, hbos wrote:
> > case default:
> >   RTC_NOTREACHED();
> 
> I disagree. If you have an enum class with all values covered, leave default
out
> so you get a compile time error instead of runtime one.

Oh right, neat, no default!

[hta-webrtc, 2016-12-02 11:09:01]

Comments addressed.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:297: H264PacketizationMode
packetization_mode;
On 2016/12/02 10:21:26, hbos wrote:
> (Thinking its strange that this information is in the header when it needs to
be
> what has been negotiated elsewhere, but guessing any other way would be much
> more difficult/require refactoring.)

The code that has the header doesn't have a link to the transport, which would
be the logical place to look it up. Yes, it's a bit weird, and could probably be
refactored to some benefit. But not in this CL.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:178: break;
On 2016/12/02 10:21:26, hbos wrote:
> case default:
>   RTC_NOTREACHED();

With an enum, it's better to leave the default off, as the compiler will then
tell you that you've failed to handle something.

https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:276:
RTC_CHECK(packetization_mode_ == H264PacketizationMode::NonInterleaved);
On 2016/12/02 10:21:26, hbos wrote:
> nit: RTC_CHECK_EQ

I tried. The RTC_CHECK_EQ macro borks on "enum class" arguments.
(It was fine when it was just an enum.)

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:270:
TEST(RtpPacketizerH264Test, TestMode0HasNoStapA) {
On 2016/12/02 10:21:26, hbos wrote:
> nit/optional: Naming difference "mode" vs enum name in unittest names

Done.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:438: const size_t
kFrameSize = kMaxPayloadSize + 100;
On 2016/12/02 10:21:26, hbos wrote:
> nit: I prefer to test the edge case + 1

Done.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:484: // but is not
exported.
On 2016/12/02 10:21:26, hbos wrote:
> Make this a constant of our own, e.g. kNalHeaderSize, with this comment above
> it.

Done. This is only needed for OpenH264 version 1.4, which we don't include any
more, so I kept it within the #if block (TODO: delete that code).

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:493: // it with cpu
core number.
On 2016/12/02 10:21:26, hbos wrote:
> This comment does not match with the code. I think you meant to remove it? And
> say "Limit the size of the packets produced." maybe.

Deleted. Yes, it's a leftover.
Moved the "Limit the size..." comment out of the #if block.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:498:
static_cast<unsigned int>(max_payload_size_);
On 2016/12/02 10:21:26, hbos wrote:
> Should this not be max_payload_size_ - kNalHeaderSize like above?
> Should we set uiMaxNalSize (or the like) like above?

They changed the calculation between 1.4 and 1.6, so the difference is
intentional.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:512:
SM_FIXEDSLCNUM_SLICE;
On 2016/12/02 10:21:26, hbos wrote:
> Given this TODO, why is this code different from the above #endif?

This code is simply reindented, not changed.
In noninterleaved mode, the RTP packetizer can break up overlarge packets, so
there's no need to specify a size limit.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl_unittest.cc
(right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl_unittest.cc:44:
TEST(H264EncoderImplTest, CanInitializeWithPacketizationMode1Explicitly) {
On 2016/12/02 10:21:26, hbos wrote:
> nit/optional: Naming difference "mode" vs enum name in unittest names

Done.

[hbos, 2016-12-02 11:58:42]

lgtm

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:297: H264PacketizationMode
packetization_mode;
On 2016/12/02 11:09:01, hta-webrtc wrote:
> On 2016/12/02 10:21:26, hbos wrote:
> > (Thinking its strange that this information is in the header when it needs
to
> be
> > what has been negotiated elsewhere, but guessing any other way would be much
> > more difficult/require refactoring.)
> 
> The code that has the header doesn't have a link to the transport, which would
> be the logical place to look it up. Yes, it's a bit weird, and could probably
be
> refactored to some benefit. But not in this CL.

Acknowledged.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:276:
RTC_CHECK(packetization_mode_ == H264PacketizationMode::NonInterleaved);
On 2016/12/02 11:09:01, hta-webrtc wrote:
> On 2016/12/02 10:21:26, hbos wrote:
> > nit: RTC_CHECK_EQ
> 
> I tried. The RTC_CHECK_EQ macro borks on "enum class" arguments.
> (It was fine when it was just an enum.)

Acknowledged.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:484: // but is not
exported.
On 2016/12/02 11:09:01, hta-webrtc wrote:
> On 2016/12/02 10:21:26, hbos wrote:
> > Make this a constant of our own, e.g. kNalHeaderSize, with this comment
above
> > it.
> 
> Done. This is only needed for OpenH264 version 1.4, which we don't include any
> more, so I kept it within the #if block (TODO: delete that code).

Acknowledged.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:498:
static_cast<unsigned int>(max_payload_size_);
On 2016/12/02 11:09:01, hta-webrtc wrote:
> On 2016/12/02 10:21:26, hbos wrote:
> > Should this not be max_payload_size_ - kNalHeaderSize like above?
> > Should we set uiMaxNalSize (or the like) like above?
> 
> They changed the calculation between 1.4 and 1.6, so the difference is
> intentional.

Acknowledged.

https://codereview.webrtc.org/2528343002/diff/410001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:512:
SM_FIXEDSLCNUM_SLICE;
On 2016/12/02 11:09:01, hta-webrtc wrote:
> On 2016/12/02 10:21:26, hbos wrote:
> > Given this TODO, why is this code different from the above #endif?
> 
> This code is simply reindented, not changed.
> In noninterleaved mode, the RTP packetizer can break up overlarge packets, so
> there's no need to specify a size limit.

Acknowledged.

https://codereview.webrtc.org/2528343002/diff/430001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2528343002/diff/430001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:479: // Limit the
size of the packets produced.
nit: indent this:
    case H264PacketizationMode::SingleNalUnit:
      // Limit the size of the packets produced.

[sprang_webrtc, 2016-12-02 12:25:50]

lgtm

[magjed_webrtc, 2016-12-02 12:34:26]

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/media/base/codec....
webrtc/media/base/codec.cc:119: }
On 2016/12/01 18:26:27, hta-webrtc wrote:
> On 2016/12/01 16:58:56, språng wrote:
> > Why was this moved here?
> 
> Because it was the simplest way to set a default value for packetization mode
> that would apply everywhere the cricket::Codec object was created by its
> constructor (which is most places).
> 
> It did not cover all cases, unfortunately (see the comment next to the value
> assignment for the packetization mode enum).

This is a step in the wrong direction. Why can't you track down all places that
needs this and add it there? I couldn't find the comment you mention.

https://codereview.webrtc.org/2528343002/diff/430001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2528343002/diff/430001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:172: std::string
packetization_mode_string;
nit: It feels unnecessary to do std::string copies all the time. Can you write
it like this instead:
auto it = codec.params.find(cricket::kH264FmtpPacketizationMode);
if (it != codec.params.end() && strcmp(it->second, "1") == 0)
  packetization_mode_ = H264PacketizationMode::NonInterleaved;

[hta-webrtc, 2016-12-02 12:48:48]

Responses to magjed.

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/media/base/codec.cc
File webrtc/media/base/codec.cc (right):

https://codereview.webrtc.org/2528343002/diff/390001/webrtc/media/base/codec....
webrtc/media/base/codec.cc:119: }
On 2016/12/02 12:34:25, magjed_webrtc wrote:
> On 2016/12/01 18:26:27, hta-webrtc wrote:
> > On 2016/12/01 16:58:56, språng wrote:
> > > Why was this moved here?
> > 
> > Because it was the simplest way to set a default value for packetization
mode
> > that would apply everywhere the cricket::Codec object was created by its
> > constructor (which is most places).
> > 
> > It did not cover all cases, unfortunately (see the comment next to the value
> > assignment for the packetization mode enum).
> 
> This is a step in the wrong direction. Why can't you track down all places
that
> needs this and add it there? I couldn't find the comment you mention.

Why is it a step in the wrong direction?

Previous to this CL, all cases of creating an H.264 codec were implicitly
packetization mode 1.
Subsequent to this CL, we can have two states:

- All codecs with no params are implicitly packetization mode 1. This means that
we have a different meaning of "no parameter" than SDP assigns to it, and would
have to handle that in SDP generation (emitting a parameter when there's none in
the codec struct, setting an explicit parameter with mode 0 when there's none in
the SDP)

- Unless the user specifically removes it, all H.264 codecs have a
"packetization-mode=1" parameter. This is the same as SDP, and SHOULD produce
the same effect as alternative 1 (IF all instances that create a codec use the
constructor).
That's the approach implemented by this CL.

The comment refers to
https://codereview.webrtc.org/2528343002/patch/430001/440005 - it seems that
there's *something* that creates H.264 codecs without calling the constructor.
It's likely in Java, and it's likely only running on Android.

https://codereview.webrtc.org/2528343002/diff/430001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2528343002/diff/430001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:172: std::string
packetization_mode_string;
On 2016/12/02 12:34:26, magjed_webrtc wrote:
> nit: It feels unnecessary to do std::string copies all the time. Can you write
> it like this instead:
> auto it = codec.params.find(cricket::kH264FmtpPacketizationMode);
> if (it != codec.params.end() && strcmp(it->second, "1") == 0)
>   packetization_mode_ = H264PacketizationMode::NonInterleaved;

Only by bypassing codec.GetParam. I don't want this piece of code to depend on
the internal representation of parameters inside cricket::Codec.

https://codereview.webrtc.org/2528343002/diff/430001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:479: // Limit the
size of the packets produced.
On 2016/12/02 11:58:42, hbos wrote:
> nit: indent this:
>     case H264PacketizationMode::SingleNalUnit:
>       // Limit the size of the packets produced.

git cl format did this. I haven't figured out where to file the bug yet.

[hta-webrtc, 2016-12-02 14:40:33]

Updated after discussion with magjed. Note todo.
PTAL.

[hta-webrtc, 2016-12-02 14:40:46]

The CQ bit was checked by hta@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 14:40:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-02 15:03:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 15:04:01]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2016-12-02 15:42:36]

lgtm

https://codereview.webrtc.org/2528343002/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:276:
RTC_CHECK(packetization_mode_ == H264PacketizationMode::NonInterleaved);
nit: RTC_CHECK_EQ

[hta-webrtc, 2016-12-05 13:07:40]

https://codereview.webrtc.org/2528343002/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2528343002/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:276:
RTC_CHECK(packetization_mode_ == H264PacketizationMode::NonInterleaved);
On 2016/12/02 15:42:36, magjed_webrtc wrote:
> nit: RTC_CHECK_EQ

Suggest doing that later. It requires having an << operator on the
H264PacketizationMode enum class, which will require that module_common_types.h
have a .cc file corresponding to it, which requires some BUILD file surgery.

One of the prices we pay for using enum class rather than plain enum.

[mflodman, 2016-12-06 08:04:00]

LGTM with one minor comment.

https://codereview.webrtc.org/2528343002/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format.cc (right):

https://codereview.webrtc.org/2528343002/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format.cc:13: #include
"webrtc/modules/rtp_rtcp/source/rtp_format.h"
Normally on top of all includes.

[hta-webrtc, 2016-12-06 09:16:22]

The CQ bit was checked by hta@webrtc.org

[hta-webrtc, 2016-12-06 09:16:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
hbos@webrtc.org, magjed@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2528343002/#ps470001 (title:
"Fix header order")

[commit-bot: I haz the power, 2016-12-06 09:16:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hta-webrtc, 2016-12-06 09:16:40]

https://codereview.webrtc.org/2528343002/diff/450001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format.cc (right):

https://codereview.webrtc.org/2528343002/diff/450001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format.cc:13: #include
"webrtc/modules/rtp_rtcp/source/rtp_format.h"
On 2016/12/06 08:04:00, mflodman wrote:
> Normally on top of all includes.

Done.

[commit-bot: I haz the power, 2016-12-06 09:17:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 09:17:37]

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/13146)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/13157)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15503)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15361)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19444)

[hta-webrtc, 2016-12-06 10:05:42]

The CQ bit was checked by hta@webrtc.org

[hta-webrtc, 2016-12-06 10:05:43]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
sprang@webrtc.org, hbos@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2528343002/#ps490001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-12-06 10:05:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-06 10:22:45]

CQ is committing da patch.

Bot data: {"patchset_id": 490001, "attempt_start_ts": 1481018742804950,
"parent_rev": "69ea6015f66badd582cdbdc5cdda0f821209c610", "commit_rev":
"310692bfbac8aedea5347eee22fc795557c79984"}

[commit-bot: I haz the power, 2016-12-06 10:22:48]

Description was changed from

==========
This approach passes packetization mode to the encoder as part of
a cricket::VideoCodec structure, rather than as part of struct VideoCodecH264
inside webrtc::VideoCodec.

BUG=600254
==========

to

==========
This approach passes packetization mode to the encoder as part of
a cricket::VideoCodec structure, rather than as part of struct VideoCodecH264
inside webrtc::VideoCodec.

BUG=600254
==========

[commit-bot: I haz the power, 2016-12-06 10:22:50]

Committed patchset #13 (id:490001)

[commit-bot: I haz the power, 2016-12-06 10:22:58]

Description was changed from

==========
This approach passes packetization mode to the encoder as part of
a cricket::VideoCodec structure, rather than as part of struct VideoCodecH264
inside webrtc::VideoCodec.

BUG=600254
==========

to

==========
This approach passes packetization mode to the encoder as part of
a cricket::VideoCodec structure, rather than as part of struct VideoCodecH264
inside webrtc::VideoCodec.

BUG=600254

Committed: https://crrev.com/e59647b991f61cf1cf61b020356705e6c0f81257
Cr-Commit-Position: refs/heads/master@{#15437}
==========

[commit-bot: I haz the power, 2016-12-06 10:23:00]

Patchset 13 (id:??) landed as
https://crrev.com/e59647b991f61cf1cf61b020356705e6c0f81257
Cr-Commit-Position: refs/heads/master@{#15437}

[hta-webrtc, 2016-12-06 12:21:42]

A revert of this CL (patchset #13 id:490001) has been created in
https://codereview.webrtc.org/2558453002/ by hta@webrtc.org.

The reason for reverting is: Failures on the Linux Memcheck bot
.