Ensure internal_source is false for internal encoders.

Ensure internal_source is false for internal encoders.

webrtcvideoengine2.cc uses a field for parameters_, and doesn't empty
out the current state in functions like SetCodec. In the case of
internal_source, SetCodec only set it for external encoders, which
means that in a switch from an internal-source external encoder to an
internal encoder, the internal_source bit would stay set.

(It's plausible that there are other places that are also unsafe and we
just don't notice because codec switches are uncommon in most usage)

In combination with https://codereview.webrtc.org/2574183002/,
generic_encoder.cc now creates 1x1 uninitialized frames as fake frames
for internal_source keyframe requests. The vp8 software encoder doesn't
deal correctly with frames of resolutions that don't match the
configured resolution (besides a DCHECK) and no longer throws these
away (they used to be 0x0 frames), so this results in the VP8
encoder creating a keyframe of the configured send codec size by reading
random memory off the end of the fake I420 frame. This could either
cause crashes or encoding junk data, depending on where the allocation
was.

BUG=webrtc:6957
Review-Url: https://codereview.webrtc.org/2617003003
Cr-Commit-Position: refs/heads/master@{#15969}
Committed: https://chromium.googlesource.com/external/webrtc/+/5d3b28b85393de3c9b81190bf6618966c3c2e227

[noahric, 2017-01-05 20:44:00]

The CQ bit was checked by noahric@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 20:44:03]

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

[noahric, 2017-01-05 20:46:17]

noahric@chromium.org changed reviewers:
+ magjed@webrtc.org, nisse@chromium.org

[commit-bot: I haz the power, 2017-01-05 20:48:53]

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

[commit-bot: I haz the power, 2017-01-05 20:48:54]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11933)

[noahric, 2017-01-05 20:50:25]

Description was changed from

==========
Ensure internal_source is false for internal encoders.

webrtcvideoengine2.cc uses a field for parameters_, and doesn't empty
out the current state in functions like SetCodec. In the case of
internal_source, SetCodec only set it for external encoders, which
means that in a switch from an internal-source external encoder to an
internal encoder, the internal_source bit would stay set.

(It's plausible that there are other places that are also unsafe and we
just don't notice because codec switches are uncommon in most usage)

In combination with https://codereview.webrtc.org/2574183002/,
generic_encoder.cc now creates 1x1 uninitialized frames as fake frames
for internal_source keyframe requests. The vp8 software encoder doesn't
deal correctly with frames of resolutions that don't match the
configured resolution (besides a DCHECK) and no longer throws these
away (they used to be 0x0 frames), so this results in the VP8
encoder creating a keyframe of the configured send codec size by reading
random memory off the end of the fake I420 frame. This could either
cause crashes or encoding junk data, depending on where the allocation
was.

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

to

==========
Ensure internal_source is false for internal encoders.

webrtcvideoengine2.cc uses a field for parameters_, and doesn't empty
out the current state in functions like SetCodec. In the case of
internal_source, SetCodec only set it for external encoders, which
means that in a switch from an internal-source external encoder to an
internal encoder, the internal_source bit would stay set.

(It's plausible that there are other places that are also unsafe and we
just don't notice because codec switches are uncommon in most usage)

In combination with https://codereview.webrtc.org/2574183002/,
generic_encoder.cc now creates 1x1 uninitialized frames as fake frames
for internal_source keyframe requests. The vp8 software encoder doesn't
deal correctly with frames of resolutions that don't match the
configured resolution (besides a DCHECK) and no longer throws these
away (they used to be 0x0 frames), so this results in the VP8
encoder creating a keyframe of the configured send codec size by reading
random memory off the end of the fake I420 frame. This could either
cause crashes or encoding junk data, depending on where the allocation
was.

BUG=6957
==========

[nisse-chromium (ooo August 14), 2017-01-09 08:27:41]

LGTM.

https://codereview.webrtc.org/2617003003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2617003003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:1772:
parameters_.config.encoder_settings.internal_source = false;
Does this function now set *all* attributes of
parameters_.config.encoder_settings?

[magjed_webrtc, 2017-01-09 10:09:37]

Description was changed from

==========
Ensure internal_source is false for internal encoders.

webrtcvideoengine2.cc uses a field for parameters_, and doesn't empty
out the current state in functions like SetCodec. In the case of
internal_source, SetCodec only set it for external encoders, which
means that in a switch from an internal-source external encoder to an
internal encoder, the internal_source bit would stay set.

(It's plausible that there are other places that are also unsafe and we
just don't notice because codec switches are uncommon in most usage)

In combination with https://codereview.webrtc.org/2574183002/,
generic_encoder.cc now creates 1x1 uninitialized frames as fake frames
for internal_source keyframe requests. The vp8 software encoder doesn't
deal correctly with frames of resolutions that don't match the
configured resolution (besides a DCHECK) and no longer throws these
away (they used to be 0x0 frames), so this results in the VP8
encoder creating a keyframe of the configured send codec size by reading
random memory off the end of the fake I420 frame. This could either
cause crashes or encoding junk data, depending on where the allocation
was.

BUG=6957
==========

to

==========
Ensure internal_source is false for internal encoders.

webrtcvideoengine2.cc uses a field for parameters_, and doesn't empty
out the current state in functions like SetCodec. In the case of
internal_source, SetCodec only set it for external encoders, which
means that in a switch from an internal-source external encoder to an
internal encoder, the internal_source bit would stay set.

(It's plausible that there are other places that are also unsafe and we
just don't notice because codec switches are uncommon in most usage)

In combination with https://codereview.webrtc.org/2574183002/,
generic_encoder.cc now creates 1x1 uninitialized frames as fake frames
for internal_source keyframe requests. The vp8 software encoder doesn't
deal correctly with frames of resolutions that don't match the
configured resolution (besides a DCHECK) and no longer throws these
away (they used to be 0x0 frames), so this results in the VP8
encoder creating a keyframe of the configured send codec size by reading
random memory off the end of the fake I420 frame. This could either
cause crashes or encoding junk data, depending on where the allocation
was.

BUG=webrtc:6957
==========

[magjed_webrtc, 2017-01-09 10:10:38]

lgtm

[noahric, 2017-01-09 17:22:07]

Thanks!

https://codereview.webrtc.org/2617003003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2617003003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:1772:
parameters_.config.encoder_settings.internal_source = false;
On 2017/01/09 08:27:41, nisse-chromium wrote:
> Does this function now set *all* attributes of
> parameters_.config.encoder_settings?

I'm not sure, but that's probably a good question for mflodman@ or team to take
a pass over this code. I'd assume a safer pattern is to do something like
parameters_.config.<stuff> = {0} at the start of this function.

[noahric, 2017-01-09 17:22:13]

The CQ bit was checked by noahric@chromium.org

[commit-bot: I haz the power, 2017-01-09 17:22:23]

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, 2017-01-09 18:06:29]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1483982533509310, "parent_rev":
"c61a948e43401aebdb1487d14bdf3e21c65bf5b0", "commit_rev":
"5d3b28b85393de3c9b81190bf6618966c3c2e227"}

[commit-bot: I haz the power, 2017-01-09 18:06:33]

Description was changed from

==========
Ensure internal_source is false for internal encoders.

webrtcvideoengine2.cc uses a field for parameters_, and doesn't empty
out the current state in functions like SetCodec. In the case of
internal_source, SetCodec only set it for external encoders, which
means that in a switch from an internal-source external encoder to an
internal encoder, the internal_source bit would stay set.

(It's plausible that there are other places that are also unsafe and we
just don't notice because codec switches are uncommon in most usage)

In combination with https://codereview.webrtc.org/2574183002/,
generic_encoder.cc now creates 1x1 uninitialized frames as fake frames
for internal_source keyframe requests. The vp8 software encoder doesn't
deal correctly with frames of resolutions that don't match the
configured resolution (besides a DCHECK) and no longer throws these
away (they used to be 0x0 frames), so this results in the VP8
encoder creating a keyframe of the configured send codec size by reading
random memory off the end of the fake I420 frame. This could either
cause crashes or encoding junk data, depending on where the allocation
was.

BUG=webrtc:6957
==========

to

==========
Ensure internal_source is false for internal encoders.

webrtcvideoengine2.cc uses a field for parameters_, and doesn't empty
out the current state in functions like SetCodec. In the case of
internal_source, SetCodec only set it for external encoders, which
means that in a switch from an internal-source external encoder to an
internal encoder, the internal_source bit would stay set.

(It's plausible that there are other places that are also unsafe and we
just don't notice because codec switches are uncommon in most usage)

In combination with https://codereview.webrtc.org/2574183002/,
generic_encoder.cc now creates 1x1 uninitialized frames as fake frames
for internal_source keyframe requests. The vp8 software encoder doesn't
deal correctly with frames of resolutions that don't match the
configured resolution (besides a DCHECK) and no longer throws these
away (they used to be 0x0 frames), so this results in the VP8
encoder creating a keyframe of the configured send codec size by reading
random memory off the end of the fake I420 frame. This could either
cause crashes or encoding junk data, depending on where the allocation
was.

BUG=webrtc:6957

Review-Url: https://codereview.webrtc.org/2617003003
Cr-Commit-Position: refs/heads/master@{#15969}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5d3b28b85393de3c9b81190bf...
==========

[commit-bot: I haz the power, 2017-01-09 18:06:35]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/5d3b28b85393de3c9b81190bf...

[nisse-webrtc, 2017-01-10 07:53:03]

On 2017/01/09 17:22:07, noahric wrote:
> Thanks!
> 
>
https://codereview.webrtc.org/2617003003/diff/1/webrtc/media/engine/webrtcvid...
> File webrtc/media/engine/webrtcvideoengine2.cc (right):
> 
>
https://codereview.webrtc.org/2617003003/diff/1/webrtc/media/engine/webrtcvid...
> webrtc/media/engine/webrtcvideoengine2.cc:1772:
> parameters_.config.encoder_settings.internal_source = false;
> On 2017/01/09 08:27:41, nisse-chromium wrote:
> > Does this function now set *all* attributes of
> > parameters_.config.encoder_settings?
> 
> I'm not sure, but that's probably a good question for mflodman@ or team to
take
> a pass over this code. I'd assume a safer pattern is to do something like
> parameters_.config.<stuff> = {0} at the start of this function.

I've had a look, and it seems all attributes are assigned (EncoderSettings
currently has the following five attributes: payload_name, payload_type,
internal_source, full_overuse_time, encoder).