Delete VideoFrame default constructor, and IsZeroSize method.

Delete VideoFrame default constructor, and IsZeroSize method.

This ensures that the video_frame_buffer method never can return a
null pointer.

BUG=webrtc:6591
Committed: https://crrev.com/bfcf561923a42005e4c7d66d8e72e5932155f997
Cr-Commit-Position: refs/heads/master@{#15574}

[nisse-webrtc, 2016-11-30 16:09:24]

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

[nisse-webrtc, 2016-11-30 16:09:25]

This change is seems easy, at least for webrtc itself (some need for downstream
updates could be expected).

This cl supersedes https://codereview.webrtc.org/2286633002/, which I'm going to
close.

[magjed_webrtc, 2016-11-30 16:23:56]

https://codereview.webrtc.org/2541863002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2541863002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.cc:130: return
encoder_->Encode(VideoFrame(I420Buffer::Create(1, 1),
This looks very strange. What's the purpose here?

[nisse-webrtc, 2016-11-30 16:36:00]

https://codereview.webrtc.org/2541863002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2541863002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.cc:130: return
encoder_->Encode(VideoFrame(I420Buffer::Create(1, 1),
On 2016/11/30 16:23:56, magjed_webrtc wrote:
> This looks very strange. What's the purpose here?

The way I understand it, when the "internal source" hack is used, normal video
frames are never sent to the Encode method, because the encoder is hooked up
directly to the camera in some way, perhaps the source produces encoded frames
directly.

But since that's the only way to get the key frame request signal across the
encoder interface, the encoder object for that case implements an Encode method
which examines the frame_type argument, and ignores the frame.

This was the smallest change to make that ugly hack still work (it used to pass
a VideoFrame where video_frame_buffer() is null)). The right way would probably
be to add an encoder method specifically for keyframe request signalling. But
that's a refactoring unrelated to this cl.

https://codereview.webrtc.org/2541863002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.cc:133: return 0;
But this extra return statement looks very wrong.

[magjed_webrtc, 2016-12-01 11:46:38]

lgtm

https://codereview.webrtc.org/2541863002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_encoder.cc (right):

https://codereview.webrtc.org/2541863002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_encoder.cc:130: return
encoder_->Encode(VideoFrame(I420Buffer::Create(1, 1),
On 2016/11/30 16:36:00, nisse-webrtc wrote:
> On 2016/11/30 16:23:56, magjed_webrtc wrote:
> > This looks very strange. What's the purpose here?
> 
> The way I understand it, when the "internal source" hack is used, normal video
> frames are never sent to the Encode method, because the encoder is hooked up
> directly to the camera in some way, perhaps the source produces encoded frames
> directly.
> 
> But since that's the only way to get the key frame request signal across the
> encoder interface, the encoder object for that case implements an Encode
method
> which examines the frame_type argument, and ignores the frame.
> 
> This was the smallest change to make that ugly hack still work (it used to
pass
> a VideoFrame where video_frame_buffer() is null)). The right way would
probably
> be to add an encoder method specifically for keyframe request signalling. But
> that's a refactoring unrelated to this cl.

Acknowledged.

[stefan-webrtc, 2016-12-08 13:33:30]

lgtm

[nisse-webrtc, 2016-12-13 12:19:02]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-12-13 12:19:02]

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

[commit-bot: I haz the power, 2016-12-13 12:19:11]

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-13 12:42:13]

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

[commit-bot: I haz the power, 2016-12-13 12:42:14]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/15999)

[nisse-webrtc, 2016-12-13 13:19:18]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-12-13 13:19:18]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2541863002/#ps80001 (title:
"Initialize pixel data in VideoBroadcasterTest.OnFrame test.")

[commit-bot: I haz the power, 2016-12-13 13:19:27]

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

[nisse-webrtc, 2016-12-13 14:04:36]

Description was changed from

==========
Delete VideoFrame default constructor, and IsZeroSize method.

This ensures that the video_frame_buffer method never can return a
null pointer.

BUG=None.
==========

to

==========
Delete VideoFrame default constructor, and IsZeroSize method.

This ensures that the video_frame_buffer method never can return a
null pointer.

BUG=webrtc:6591
==========

[commit-bot: I haz the power, 2016-12-13 14:08:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481635158472780,
"parent_rev": "27e4c9b0e82df2205015f0cd1a589424fa14f276", "commit_rev":
"dc1dbdf5b86ebeaab900cd54dbaec131879010ac"}

[commit-bot: I haz the power, 2016-12-13 14:08:35]

Description was changed from

==========
Delete VideoFrame default constructor, and IsZeroSize method.

This ensures that the video_frame_buffer method never can return a
null pointer.

BUG=webrtc:6591
==========

to

==========
Delete VideoFrame default constructor, and IsZeroSize method.

This ensures that the video_frame_buffer method never can return a
null pointer.

BUG=webrtc:6591

Review-Url: https://codereview.webrtc.org/2541863002
==========

[commit-bot: I haz the power, 2016-12-13 14:08:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-13 14:08:44]

Description was changed from

==========
Delete VideoFrame default constructor, and IsZeroSize method.

This ensures that the video_frame_buffer method never can return a
null pointer.

BUG=webrtc:6591

Review-Url: https://codereview.webrtc.org/2541863002
==========

to

==========
Delete VideoFrame default constructor, and IsZeroSize method.

This ensures that the video_frame_buffer method never can return a
null pointer.

BUG=webrtc:6591

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

[commit-bot: I haz the power, 2016-12-13 14:08:45]

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

[nisse-webrtc, 2016-12-14 10:06:34]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/2574123002/ by nisse@webrtc.org.

The reason for reverting is: Crashes perf tests, e.g., 

./out/Debug/webrtc_perf_tests
--gtest_filter='FullStackTest.ScreenshareSlidesVP8_2TL_VeryLossyNet'

dies with an assert related to rtc::Optional..