Pass selected cricket::VideoCodec down to internal H264 encoder

Pass selected cricket::VideoCodec down to internal H264 encoder

Pass the selected cricket::VideoCodec to H264EncoderImpl::H264EncoderImpl. The cricket::VideoCodec contains relevant information for H264 about selected profile and packetization mode.

BUG=chromium:600254, webrtc:6402, webrtc:6337
Committed: https://crrev.com/ceecea4559894d680daa689eef3677a612c9e7ec
Cr-Commit-Position: refs/heads/master@{#15270}

[magjed_webrtc, 2016-11-03 16:53:29]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-11-03 17:00:02]

Description was changed from

==========
Pass cricket::VideoCodec to internal software encoders

BUG=
==========

to

==========
Pass cricket::VideoCodec to internal software encoders

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

[magjed_webrtc, 2016-11-03 17:01:03]

Description was changed from

==========
Pass cricket::VideoCodec to internal software encoders

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

to

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

[magjed_webrtc, 2016-11-03 17:06:11]

Description was changed from

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

to

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

Pass the selected cricket::VideoCodec from
WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder down to
H264EncoderImpl::H264EncoderImpl. The cricket::VideoCodec contains relevant
information for H264 about selected profile and packetization mode.

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

[magjed_webrtc, 2016-11-03 17:06:47]

magjed@webrtc.org changed reviewers:
+ hta@webrtc.org

[magjed_webrtc, 2016-11-03 17:07:05]

Harald - please take a look.

[hta-webrtc, 2016-11-04 10:04:05]

lgtm, but I think you should have more reviewers.

Very clean replacement!
If the usage in h264_encoder_impl.cc is typical, we might want to think about
replacing the arg to InitEncoder with something that has width/height/framerate
info only.

Turtle race to see which one lands first....

https://codereview.webrtc.org/2474993002/diff/20001/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2474993002/diff/20001/webrtc/DEPS#newcode17
webrtc/DEPS:17: "+webrtc/media/base/codec.h",
I'd think Tommi would want to check if we can avoid this - alternatively, file a
bug to move the type that we need to share out to somewhere else.

Which type is it that we have to share, and with what?

https://codereview.webrtc.org/2474993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/2474993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:40: // - height
Are any of these also present in cricket::VideoCodec?

[magjed_webrtc, 2016-11-04 13:48:29]

https://codereview.webrtc.org/2474993002/diff/20001/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2474993002/diff/20001/webrtc/DEPS#newcode17
webrtc/DEPS:17: "+webrtc/media/base/codec.h",
On 2016/11/04 10:04:05, hta-webrtc wrote:
> I'd think Tommi would want to check if we can avoid this - alternatively, file
a
> bug to move the type that we need to share out to somewhere else.
> 
> Which type is it that we have to share, and with what?

The type that is needed is cricket::VideoCodec, and it's needed in the static
factory function for the webrtc::VideoEncoder interface in
webrtc/video_encoder.h:
static VideoEncoder* VideoEncoder::Create(const cricket::VideoCodec& codec);

This dependency is quite easy to avoid and I have planned a follow-up CL where I
remove the static factory function in webrtc::VideoEncoder (I think it's ugly
that the interface is aware of the implementations of the interface anyway).
Instead, I plan to expose the internal encoders under
webrtc/modules/video_coding/codecs/ (vp8, vp9, and h264) with an implementation
of WebRtcVideoEncoderFactory that is placed in
webrtc/modules/video_coding/codecs.

That would streamline the implementation in WebRtcVideoEngine2 since we would
then have two encoder factories of the same type (WebRtcVideoEncoderFactory),
one internal and one external. As a bonus, we can move the hardcoded codec
enumeration in WebRtcVideoEngine2 to the internal factory, i.e. move this code
to the codec implementations:

webrtc/media/engine/webrtcvideoengine2.cc:
std::vector<VideoCodec> DefaultVideoCodecList() {
  std::vector<VideoCodec> codecs;
  AddCodecAndMaybeRtxCodec(
      MakeVideoCodecWithDefaultFeedbackParams(kDefaultVp8PlType, kVp8CodecName),
      &codecs);
  if (webrtc::VP9Encoder::IsSupported() && webrtc::VP9Decoder::IsSupported()) {
    AddCodecAndMaybeRtxCodec(MakeVideoCodecWithDefaultFeedbackParams(
                                 kDefaultVp9PlType, kVp9CodecName),
                             &codecs);
  }
  if (webrtc::H264Encoder::IsSupported() &&
      webrtc::H264Decoder::IsSupported()) {
    VideoCodec codec = MakeVideoCodecWithDefaultFeedbackParams(
        kDefaultH264PlType, kH264CodecName);
    // TODO(hta): Move all parameter generation for SDP into the codec
    // implementation, for all codecs and parameters.
    // TODO(hta): Move selection of profile-level-id to H.264 codec
    // implementation.
    // TODO(hta): Set FMTP parameters for all codecs of type H264.
    codec.SetParam(kH264FmtpProfileLevelId,
                   kH264ProfileLevelConstrainedBaseline);
    codec.SetParam(kH264FmtpLevelAsymmetryAllowed, "1");
    codec.SetParam(kH264FmtpPacketizationMode, "1");
    AddCodecAndMaybeRtxCodec(codec, &codecs);
  }
  AddCodecAndMaybeRtxCodec(VideoCodec(kDefaultRedPlType, kRedCodecName),
                           &codecs);
  codecs.push_back(VideoCodec(kDefaultUlpfecType, kUlpfecCodecName));
  return codecs;
}

Actually, I think I can include that change into this CL without the CL getting
too big. I will try to upload a new patch set next week.

https://codereview.webrtc.org/2474993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/2474993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:40: // - height
On 2016/11/04 10:04:05, hta-webrtc wrote:
> Are any of these also present in cricket::VideoCodec?

codecType is deducible from cricket::VideoCodec. I think it's possible to set
maxFramerate in cricket::VideoCodec as well. I don't think  any of the other
parameters are possible to set in cricket::VideoCodec.

[magjed_webrtc, 2016-11-17 16:16:41]

Description was changed from

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

Pass the selected cricket::VideoCodec from
WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder down to
H264EncoderImpl::H264EncoderImpl. The cricket::VideoCodec contains relevant
information for H264 about selected profile and packetization mode.

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

to

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

Pass the selected cricket::VideoCodec to H264EncoderImpl::H264EncoderImpl. The
cricket::VideoCodec contains relevant information for H264 about selected
profile and packetization mode.

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

[magjed_webrtc, 2016-11-17 16:16:58]

Please take another look. The diff is a lot smaller now.

[hta-webrtc, 2016-11-23 14:08:53]

lgtm

still a few lines to revert?

https://codereview.webrtc.org/2474993002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/2474993002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:27: #include
"webrtc/modules/video_coding/include/video_codec_interface.h"
Are these includes needed, given no other changes?

https://codereview.webrtc.org/2474993002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/2474993002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:24: #include
"webrtc/modules/video_coding/include/video_codec_interface.h"
Removable?

[magjed_webrtc, 2016-11-25 12:09:36]

https://codereview.webrtc.org/2474993002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/2474993002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:27: #include
"webrtc/modules/video_coding/include/video_codec_interface.h"
On 2016/11/23 14:08:53, hta-webrtc wrote:
> Are these includes needed, given no other changes?

I have just moved it from webrtc/modules/video_coding/codecs/h264/include/h264.h
to here. I can't remove it completely. Same for encode .cc file.

[magjed_webrtc, 2016-11-25 12:09:40]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-11-25 12:09:53]

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-25 12:12:00]

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

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

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/12793)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12814)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15166)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20379)

[magjed_webrtc, 2016-11-28 09:57:12]

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

[commit-bot: I haz the power, 2016-11-28 09:57: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 10:20:13]

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

[commit-bot: I haz the power, 2016-11-28 10:20:13]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2016-11-28 10:20:17]

magjed@webrtc.org changed reviewers:
+ stefan@webrtc.org

[magjed_webrtc, 2016-11-28 10:20:30]

Stefan - please take a look.

https://codereview.webrtc.org/2474993002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/2474993002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:27: #include
"webrtc/modules/video_coding/include/video_codec_interface.h"
On 2016/11/25 12:09:36, magjed_webrtc wrote:
> On 2016/11/23 14:08:53, hta-webrtc wrote:
> > Are these includes needed, given no other changes?
> 
> I have just moved it from
webrtc/modules/video_coding/codecs/h264/include/h264.h
> to here. I can't remove it completely. Same for encode .cc file.

I ended up reverting these unrelated include changes.

[stefan-webrtc, 2016-11-28 15:04:14]

It's actually being used in a separate CL I suppose?

lgtm

https://codereview.webrtc.org/2474993002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2474993002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1012:
video_encoder_.reset(H264Encoder::Create(cricket::VideoCodec("H264")));
Maybe create a VideoCodec on line 1010 and pass in params_.video.codec instead?

[hta-webrtc, 2016-11-28 15:14:47]

On 2016/11/28 15:04:14, stefan-webrtc (holmer) wrote:
> It's actually being used in a separate CL I suppose?
> 

I'm planning to rebase the packetization mode patch on top of it once it lands.
https://codereview.webrtc.org/2528343002/ is the new number.

[magjed_webrtc, 2016-11-28 15:18:13]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-11-28 15:18:14]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2474993002/#ps60001 (title:
"Fix includes.")

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

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 15:20:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480346293890920,
"parent_rev": "cd602b185a517adf61d7e9c19ba9c0ce849180f0", "commit_rev":
"b57cbc591cf551dfb0ea104cedfabc2c08012eb9"}

[commit-bot: I haz the power, 2016-11-28 15:20:25]

Description was changed from

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

Pass the selected cricket::VideoCodec to H264EncoderImpl::H264EncoderImpl. The
cricket::VideoCodec contains relevant information for H264 about selected
profile and packetization mode.

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

to

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

Pass the selected cricket::VideoCodec to H264EncoderImpl::H264EncoderImpl. The
cricket::VideoCodec contains relevant information for H264 about selected
profile and packetization mode.

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

[commit-bot: I haz the power, 2016-11-28 15:20:27]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-28 15:20:34]

Description was changed from

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

Pass the selected cricket::VideoCodec to H264EncoderImpl::H264EncoderImpl. The
cricket::VideoCodec contains relevant information for H264 about selected
profile and packetization mode.

BUG=chromium:600254,webrtc:6402, webrtc:6337
==========

to

==========
Pass selected cricket::VideoCodec down to internal H264 encoder

Pass the selected cricket::VideoCodec to H264EncoderImpl::H264EncoderImpl. The
cricket::VideoCodec contains relevant information for H264 about selected
profile and packetization mode.

BUG=chromium:600254,webrtc:6402, webrtc:6337

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

[commit-bot: I haz the power, 2016-11-28 15:20:36]

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