Verify returned frames from media::VideoFrame::Wrap*() methods

Verify returned frames from media::VideoFrame::Wrap*() methods

BUG=552112
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8428d41278c107c107736454dc4fdddbd14969bd
Cr-Commit-Position: refs/heads/master@{#369705}

[emircan, 2015-11-25 19:09:09]

Description was changed from

==========
cleanup

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

to

==========
Verify returned frames from media::VideoFrame::Wrap*() methods

BUG=552112
==========

[emircan, 2015-11-25 19:09:09]

emircan@chromium.org changed reviewers:
+ posciak@chromium.org

[emircan, 2015-11-25 23:15:29]

Patchset #1 (id:1) has been deleted

[emircan, 2015-11-25 23:15:37]

PTAL.

[Pawel Osciak, 2015-11-26 01:19:03]

A few additional locations that seem also not to check:

- cc/layers/video_layer_impl_unittest.cc:344
- cc/resources/video_resource_updater_unittest.cc and users of frames created
there
- cc/trees/layer_tree_host_unittest_context.cc:1029
- chromecast/renderer/media/hole_frame_factory.cc:64
- content/browser/renderer_host/media/video_capture_controller_unittest.cc
- content/common/gpu/media/v4l2_image_processor.cc:564
- content/common/gpu/media/video_encode_accelerator_unittest.cc:1335
- content/renderer/media/android/webmediaplayer_android.cc:1245
- content/renderer/media/media_stream_video_track.cc:184
- content/renderer/media/webrtc/media_stream_remote_video_source.cc:119
- content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:98
- media/capture/content/thread_safe_capture_oracle.cc:123
- media/cast/sender/video_sender_unittest.cc:136
- media/cast/test/fake_media_source.cc:553
- users of test frames in
media/video/gpu_memory_buffer_video_frame_pool_unittest.cc

https://codereview.chromium.org/1476523005/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_encode_accelerator.cc:279: if (!frame.get())
{
if (!frame) ?

https://codereview.chromium.org/1476523005/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_encode_accelerator.cc:281: "VideoFrame for
frame_id=" << params.frame_id;
s/"Video/<< "Video/

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:421: if (frame &&
picture.allow_overlay()) {
Could we perhaps have an error path here logging the error and allow_overlay in
a separate if please?

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_video_track_host.cc:360:
frame_deliverer_->DeliverVideoFrame(frame);
Please empty line above.

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_video_encoder_host.cc:639: } else {
Personally I'd prefer a clear error path, but up to you:

if (!frame) {
  DLOG(ERROR)
  NotifyPE();
  return nullptr;
}

frame->AddDO();

return frame;

https://codereview.chromium.org/1476523005/diff/20001/media/filters/vpx_video...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/media/filters/vpx_video...
media/filters/vpx_video_decoder.cc:599: *video_frame = frame_pool_.CreateFrame(
I think this may also fail and return nullptr.

https://codereview.chromium.org/1476523005/diff/20001/media/video/gpu_memory_...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:584:
video_frame->metadata()->IsTrue(VideoFrameMetadata::ALLOW_OVERLAY))
I'm wondering why this check, I don't think WrapYUV420NativeTextures() would
ever set it to true from what I can see in the code?

Also, wondering why here we need to check (and likely always fail since I think
it looks like it would always be false), but not check for NV12 and UYVY below?

Perhaps we should extract this and l.594 out of switch and always set allow
overlay.

[miu, 2015-11-26 20:28:05]

Drive-by comment: IMO, all the DLOG(ERROR) statements should be LOG(DFATAL). 
The reason is that: 1) in debug builds, a failure to create a frame might
indicate a bug introduced by changed code and this should crash the process with
a stack trace logged; and 2) in release builds, we shouldn't hide the fact that
some environmental condition is present and causing frames to be dropped.  Note
that, in many places, DCHECKs were already there (and you removed them), which
would do #1.

[emircan, 2015-12-04 04:13:44]

Patchset #4 (id:80001) has been deleted

[emircan, 2015-12-04 04:20:09]

Sorry for the delay on this. I was busy with a blink CL, but I will have time
now.

@posciak 
Thanks for pointing the missing files, I will go through them. But, I was
leaving out unittests intentionally as they would (consistently) fail if
video_frame returns nullptr. WDYT?

@miu
I changed them as you suggested. I was trying to follow the error handling
structure of each file I am touching, but it makes sense to notify if we are
ever dropping frames.

https://codereview.chromium.org/1476523005/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_encode_accelerator.cc:279: if (!frame.get())
{
On 2015/11/26 01:19:03, Pawel Osciak wrote:
> if (!frame) ?

Done.

https://codereview.chromium.org/1476523005/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_encode_accelerator.cc:281: "VideoFrame for
frame_id=" << params.frame_id;
On 2015/11/26 01:19:03, Pawel Osciak wrote:
> s/"Video/<< "Video/

Done.

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:421: if (frame &&
picture.allow_overlay()) {
On 2015/11/26 01:19:03, Pawel Osciak wrote:
> Could we perhaps have an error path here logging the error and allow_overlay
in
> a separate if please?

Done. It returns a nullptr and l.374 logs it as well.

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_video_track_host.cc:360:
frame_deliverer_->DeliverVideoFrame(frame);
On 2015/11/26 01:19:03, Pawel Osciak wrote:
> Please empty line above.

Done.

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_video_encoder_host.cc:639: } else {
On 2015/11/26 01:19:03, Pawel Osciak wrote:
> Personally I'd prefer a clear error path, but up to you:
> 
> if (!frame) {
>   DLOG(ERROR)
>   NotifyPE();
>   return nullptr;
> }
> 
> frame->AddDO();
> 
> return frame;

Done.

https://codereview.chromium.org/1476523005/diff/20001/media/filters/vpx_video...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/media/filters/vpx_video...
media/filters/vpx_video_decoder.cc:599: *video_frame = frame_pool_.CreateFrame(
On 2015/11/26 01:19:03, Pawel Osciak wrote:
> I think this may also fail and return nullptr.

My bad. I was trying to handle nullptr in the return on l.467. Changing it.

https://codereview.chromium.org/1476523005/diff/20001/media/video/gpu_memory_...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1476523005/diff/20001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:584:
video_frame->metadata()->IsTrue(VideoFrameMetadata::ALLOW_OVERLAY))
On 2015/11/26 01:19:03, Pawel Osciak wrote:
> I'm wondering why this check, I don't think WrapYUV420NativeTextures() would
> ever set it to true from what I can see in the code?
> 
> Also, wondering why here we need to check (and likely always fail since I
think
> it looks like it would always be false), but not check for NV12 and UYVY
below?
> 
> Perhaps we should extract this and l.594 out of switch and always set allow
> overlay.

It is checking the |video_frame| metadata rather than the new |frame| metadata.
|video_frame| might have it as true.

[emircan, 2015-12-05 01:09:03]

Patchset #3 (id:60001) has been deleted

[emircan, 2015-12-15 09:42:03]

Description was changed from

==========
Verify returned frames from media::VideoFrame::Wrap*() methods

BUG=552112
==========

to

==========
Verify returned frames from media::VideoFrame::Wrap*() methods

BUG=552112
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[emircan, 2015-12-15 09:42:24]

Patchset #4 (id:120001) has been deleted

[emircan, 2015-12-15 09:49:19]

Patchset #4 (id:140001) has been deleted

[emircan, 2015-12-15 09:52:36]

I restarted the CL on PS#4 after offline discussions:
- We use LOG(DFATAL) instead of DLOG(ERROR) (as miu@ suggested) inside Wrap*
function of video_frame 
- Unit tests have CHECK(video_frame)
- Other classes are handled case by case (early return, NOTIFY_ERROR)

PTAL again.

[emircan, 2015-12-15 10:22:16]

Patchset #4 (id:160001) has been deleted

[mcasas, 2015-12-15 17:14:33]

mcasas@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2015-12-15 17:14:33]

https://codereview.chromium.org/1476523005/diff/180001/cc/resources/video_res...
File cc/resources/video_resource_updater_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/180001/cc/resources/video_res...
cc/resources/video_resource_updater_unittest.cc:152: CHECK(video_frame);
Prefer ASSERT_xx() to CHECK (see last
entry in [1]): 
"Don't use these macros in tests, as they
crash the test binary and leave bots in a 
bad state.  Use the ASSERT_xx() and 
EXPECT_xx() family of macros, which 
report failures gracefully and can 
continue running other tests"

[1]
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

[emircan, 2015-12-16 15:57:10]

https://codereview.chromium.org/1476523005/diff/180001/cc/resources/video_res...
File cc/resources/video_resource_updater_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/180001/cc/resources/video_res...
cc/resources/video_resource_updater_unittest.cc:152: CHECK(video_frame);
On 2015/12/15 17:14:33, mcasas wrote:
> Prefer ASSERT_xx() to CHECK (see last
> entry in [1]): 
> "Don't use these macros in tests, as they
> crash the test binary and leave bots in a 
> bad state.  Use the ASSERT_xx() and 
> EXPECT_xx() family of macros, which 
> report failures gracefully and can 
> continue running other tests"
> 
> [1]
>
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

Thanks, changing all to ASSERT_TRUE.

[emircan, 2016-01-09 02:04:22]

Patchset #6 (id:220001) has been deleted

[emircan, 2016-01-09 02:38:41]

Friendly PING.

[mcasas, 2016-01-12 17:46:16]

LGTM % some small comments 
and a few pedantic nits.

https://codereview.chromium.org/1476523005/diff/240001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/1476523005/diff/240001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:131:
EXPECT_NE(video_frame.get(), nullptr);
nit: ASSERT_TRUE(video_frame) ?
probably doesn't make sense to continue the 
test if |video_frame| does not verify.
Same for the other EXPECT_NEs in this file below.

https://codereview.chromium.org/1476523005/diff/240001/content/common/gpu/med...
File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/1476523005/diff/240001/content/common/gpu/med...
content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:261:
base::Bind(DecodeFinished, base::Passed(&output_shm)));
nit: s/base::Passed/std::move/

https://codereview.chromium.org/1476523005/diff/240001/content/common/gpu/med...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/240001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:1334:
EXPECT_NE(video_frame.get(), nullptr);
nit: Same here, ASSERT(video_frame) for tests.

https://codereview.chromium.org/1476523005/diff/240001/media/filters/vpx_vide...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/1476523005/diff/240001/media/filters/vpx_vide...
media/filters/vpx_video_decoder.cc:597:
memory_pool_->CreateFrameCallback(vpx_image->fb_priv));
Needs {}
You can test if (video_frame) directly.

https://codereview.chromium.org/1476523005/diff/240001/media/filters/vpx_vide...
media/filters/vpx_video_decoder.cc:614: if (!(*video_frame))
if (!video_frame) ?

https://codereview.chromium.org/1476523005/diff/240001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/240001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:88:
EXPECT_NE(video_frame.get(), nullptr);
ASSERT_TRUE(video_frame);

[emircan, 2016-01-13 00:52:20]

Patchset #7 (id:260001) has been deleted

[Pawel Osciak, 2016-01-13 07:53:25]

I think in some places outside of content/common/gpu/media we don't trigger an
error, just returning instead. I'll leave it to respective owners to comment if
that is ok in those specific places. For c/c/g/m one such place I'd prefer to
please return error from to prevent stalls.

https://chromiumcodereview.appspot.com/1476523005/diff/280001/content/common/...
File content/common/gpu/media/v4l2_image_processor.cc (right):

https://chromiumcodereview.appspot.com/1476523005/diff/280001/content/common/...
content/common/gpu/media/v4l2_image_processor.cc:565: continue;
We should NOTIFY_ERROR from here instead. Otherwise we are dropping the buffer
and will stall eventually.

https://chromiumcodereview.appspot.com/1476523005/diff/280001/media/base/vide...
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/1476523005/diff/280001/media/base/vide...
media/base/video_frame.cc:495: LOG(DFATAL) << __FUNCTION__ << " Invalid
config.";
We should probably say something like "failed to duplicate file descriptors"
instead. But we already have logs on errors in DuplicateFileDescriptors(), so
I'm not sure if a log is required here if we DFATAL in DFD().

https://chromiumcodereview.appspot.com/1476523005/diff/280001/media/filters/v...
File media/filters/vpx_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/1476523005/diff/280001/media/filters/v...
media/filters/vpx_video_decoder.cc:595: if (video_frame) {
Should this be if (*video_frame)?

https://chromiumcodereview.appspot.com/1476523005/diff/280001/media/filters/v...
media/filters/vpx_video_decoder.cc:615: if (!video_frame)
Also if (*video_frame) I believe.

[emircan, 2016-01-14 23:07:41]

https://codereview.chromium.org/1476523005/diff/240001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/1476523005/diff/240001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:131:
EXPECT_NE(video_frame.get(), nullptr);
On 2016/01/12 17:46:16, mcasas wrote:
> nit: ASSERT_TRUE(video_frame) ?
> probably doesn't make sense to continue the 
> test if |video_frame| does not verify.
> Same for the other EXPECT_NEs in this file below.

I cannot use ASSERT_TRUE(video_frame) when there is a non-void return from the
function, i.e. here. I will check them accordingly and add
ASSERT_TRUE(video_frame) where I can.

https://codereview.chromium.org/1476523005/diff/240001/content/common/gpu/med...
File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/1476523005/diff/240001/content/common/gpu/med...
content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:261:
base::Bind(DecodeFinished, base::Passed(&output_shm)));
On 2016/01/12 17:46:16, mcasas wrote:
> nit: s/base::Passed/std::move/

It causes a compile error as it is inside a base::Bind statement.

https://codereview.chromium.org/1476523005/diff/240001/content/common/gpu/med...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/240001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:1334:
EXPECT_NE(video_frame.get(), nullptr);
On 2016/01/12 17:46:16, mcasas wrote:
> nit: Same here, ASSERT(video_frame) for tests.

ASSERT has the same problem as above.

https://codereview.chromium.org/1476523005/diff/240001/media/filters/vpx_vide...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/1476523005/diff/240001/media/filters/vpx_vide...
media/filters/vpx_video_decoder.cc:597:
memory_pool_->CreateFrameCallback(vpx_image->fb_priv));
On 2016/01/12 17:46:16, mcasas wrote:
> Needs {}
> You can test if (video_frame) directly.

Done.

https://codereview.chromium.org/1476523005/diff/240001/media/filters/vpx_vide...
media/filters/vpx_video_decoder.cc:614: if (!(*video_frame))
On 2016/01/12 17:46:16, mcasas wrote:
> if (!video_frame) ?

Done.

https://codereview.chromium.org/1476523005/diff/240001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/240001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:88:
EXPECT_NE(video_frame.get(), nullptr);
On 2016/01/12 17:46:16, mcasas wrote:
> ASSERT_TRUE(video_frame);

ASSERT has the same problem as above.

[emircan, 2016-01-14 23:13:54]

https://codereview.chromium.org/1476523005/diff/280001/content/common/gpu/med...
File content/common/gpu/media/v4l2_image_processor.cc (right):

https://codereview.chromium.org/1476523005/diff/280001/content/common/gpu/med...
content/common/gpu/media/v4l2_image_processor.cc:565: continue;
On 2016/01/13 07:53:25, Pawel Osciak wrote:
> We should NOTIFY_ERROR from here instead. Otherwise we are dropping the buffer
> and will stall eventually.

Changing to NOTIFY_ERROR and return. The logs would come from video_frame.cc.

https://codereview.chromium.org/1476523005/diff/280001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1476523005/diff/280001/media/base/video_frame...
media/base/video_frame.cc:495: LOG(DFATAL) << __FUNCTION__ << " Invalid
config.";
On 2016/01/13 07:53:25, Pawel Osciak wrote:
> We should probably say something like "failed to duplicate file descriptors"
> instead. But we already have logs on errors in DuplicateFileDescriptors(), so
> I'm not sure if a log is required here if we DFATAL in DFD().

Done.

https://codereview.chromium.org/1476523005/diff/280001/media/filters/vpx_vide...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/1476523005/diff/280001/media/filters/vpx_vide...
media/filters/vpx_video_decoder.cc:595: if (video_frame) {
On 2016/01/13 07:53:25, Pawel Osciak wrote:
> Should this be if (*video_frame)?

I changed it after mcasas@ suggestion. But looking back at this (*video_frame)
would make more sense as video frame is a scoped_refptr<VideoFrame>*.

https://codereview.chromium.org/1476523005/diff/280001/media/filters/vpx_vide...
media/filters/vpx_video_decoder.cc:615: if (!video_frame)
On 2016/01/13 07:53:25, Pawel Osciak wrote:
> Also if (*video_frame) I believe.

Done.

[emircan, 2016-01-14 23:19:38]

emircan@chromium.org changed reviewers:
+ aelias@chromium.org, dalecurtis@chromium.org, sievers@chromium.org

[emircan, 2016-01-14 23:19:38]

aelias@chromium.org: Please review changes in  cc/*

sievers@chromium.org: Please review changes in content/renderer/pepper/*

dalecurtis@chromium.org: Please review changes in media/*

[aelias_OOO_until_Jul13, 2016-01-14 23:21:22]

cc/ test changes lgtm

[DaleCurtis, 2016-01-14 23:27:05]

lgtm % consistency nits.

https://codereview.chromium.org/1476523005/diff/300001/cc/resources/video_res...
File cc/resources/video_resource_updater_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/300001/cc/resources/video_res...
cc/resources/video_resource_updater_unittest.cc:103: EXPECT_NE(nullptr,
video_frame.get());
EXPECT_TRUE like you've used in most other places. Remove get.

https://codereview.chromium.org/1476523005/diff/300001/cc/resources/video_res...
cc/resources/video_resource_updater_unittest.cc:127: EXPECT_NE(nullptr,
video_frame.get());
EXPECT_TRUE, remove get.

https://codereview.chromium.org/1476523005/diff/300001/cc/resources/video_res...
cc/resources/video_resource_updater_unittest.cc:155: EXPECT_NE(nullptr,
video_frame.get());
EXPECT_TRUE, remove get.

https://codereview.chromium.org/1476523005/diff/300001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/1476523005/diff/300001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:131:
EXPECT_NE(nullptr, video_frame.get());
EXPECT_TRUE, remove get

https://codereview.chromium.org/1476523005/diff/300001/content/common/gpu/med...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/300001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:1334:
EXPECT_NE(nullptr, video_frame.get());
EXPECT_TRUE, remove get.

https://codereview.chromium.org/1476523005/diff/300001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:1345:
EXPECT_NE(nullptr, frame.get());
EXPECT_TRUE, remove get.

https://codereview.chromium.org/1476523005/diff/300001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/300001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:88:
EXPECT_NE(nullptr, video_frame.get());
EXPECT_TRUE, remove get.

[no sievers, 2016-01-14 23:42:04]

On 2016/01/14 23:19:38, emircan wrote:
> mailto:aelias@chromium.org: Please review changes in  cc/*
> 
> mailto:sievers@chromium.org: Please review changes in
content/renderer/pepper/*
> 
lgtm

> mailto:dalecurtis@chromium.org: Please review changes in media/*

[emircan, 2016-01-15 00:28:28]

Thanks.

https://codereview.chromium.org/1476523005/diff/300001/cc/resources/video_res...
File cc/resources/video_resource_updater_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/300001/cc/resources/video_res...
cc/resources/video_resource_updater_unittest.cc:103: EXPECT_NE(nullptr,
video_frame.get());
On 2016/01/14 23:27:04, DaleCurtis wrote:
> EXPECT_TRUE like you've used in most other places. Remove get.

Done.

https://codereview.chromium.org/1476523005/diff/300001/cc/resources/video_res...
cc/resources/video_resource_updater_unittest.cc:127: EXPECT_NE(nullptr,
video_frame.get());
On 2016/01/14 23:27:04, DaleCurtis wrote:
> EXPECT_TRUE, remove get.

Done.

https://codereview.chromium.org/1476523005/diff/300001/cc/resources/video_res...
cc/resources/video_resource_updater_unittest.cc:155: EXPECT_NE(nullptr,
video_frame.get());
On 2016/01/14 23:27:04, DaleCurtis wrote:
> EXPECT_TRUE, remove get.

Done.

https://codereview.chromium.org/1476523005/diff/300001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/1476523005/diff/300001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:131:
EXPECT_NE(nullptr, video_frame.get());
On 2016/01/14 23:27:04, DaleCurtis wrote:
> EXPECT_TRUE, remove get

Done.

https://codereview.chromium.org/1476523005/diff/300001/content/common/gpu/med...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/300001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:1334:
EXPECT_NE(nullptr, video_frame.get());
On 2016/01/14 23:27:04, DaleCurtis wrote:
> EXPECT_TRUE, remove get.

Done.

https://codereview.chromium.org/1476523005/diff/300001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:1345:
EXPECT_NE(nullptr, frame.get());
On 2016/01/14 23:27:04, DaleCurtis wrote:
> EXPECT_TRUE, remove get.

Done.

https://codereview.chromium.org/1476523005/diff/300001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/1476523005/diff/300001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:88:
EXPECT_NE(nullptr, video_frame.get());
On 2016/01/14 23:27:05, DaleCurtis wrote:
> EXPECT_TRUE, remove get.

Done.

[emircan, 2016-01-15 00:29:38]

emircan@chromium.org changed reviewers:
+ lcwu@chromium.org

[emircan, 2016-01-15 00:29:39]

lcwu@chromium.org: Please review changes in
chromecast/renderer/media/hole_frame_factory.cc

[Pawel Osciak, 2016-01-15 01:00:56]

https://chromiumcodereview.appspot.com/1476523005/diff/340001/media/filters/v...
File media/filters/vpx_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/1476523005/diff/340001/media/filters/v...
media/filters/vpx_video_decoder.cc:606: return true;
Should we be returning true if !*video_frame (and counting the histograms)?

[emircan, 2016-01-15 01:18:57]

https://codereview.chromium.org/1476523005/diff/340001/media/filters/vpx_vide...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/1476523005/diff/340001/media/filters/vpx_vide...
media/filters/vpx_video_decoder.cc:606: return true;
On 2016/01/15 01:00:55, Pawel Osciak wrote:
> Should we be returning true if !*video_frame (and counting the histograms)?

Early return false and not adding to UMAs makes more sense. Thanks.

[lcwu1, 2016-01-15 01:26:28]

lcwu@chromium.org changed reviewers:
+ halliwell@chromium.org

[lcwu1, 2016-01-15 01:26:29]

Luke can comment on whether it's safe to return nullptr in chromecast's
CreateHoleFrame.

[Pawel Osciak, 2016-01-15 01:28:25]

content/common/gpu/media lgtm

[halliwell, 2016-01-15 01:52:05]

https://codereview.chromium.org/1476523005/diff/360001/chromecast/renderer/me...
File chromecast/renderer/media/hole_frame_factory.cc (right):

https://codereview.chromium.org/1476523005/diff/360001/chromecast/renderer/me...
chromecast/renderer/media/hole_frame_factory.cc:68: return nullptr;
I'm not sure how useful this is.  If there is a problem here, it just pushes the
problem out a level.

This is a hardcoded simple placeholder frame used only to carry the overlay
information to the browser compositor.  It does not carry any actual video data.

If we get a nullptr here then nothing is going to function correctly.  I would
say let's CHECK that frame is valid instead.

[emircan, 2016-01-15 02:04:10]

https://codereview.chromium.org/1476523005/diff/360001/chromecast/renderer/me...
File chromecast/renderer/media/hole_frame_factory.cc (right):

https://codereview.chromium.org/1476523005/diff/360001/chromecast/renderer/me...
chromecast/renderer/media/hole_frame_factory.cc:68: return nullptr;
On 2016/01/15 01:52:04, halliwell wrote:
> I'm not sure how useful this is.  If there is a problem here, it just pushes
the
> problem out a level.
> 
> This is a hardcoded simple placeholder frame used only to carry the overlay
> information to the browser compositor.  It does not carry any actual video
data.
> 
> If we get a nullptr here then nothing is going to function correctly.  I would
> say let's CHECK that frame is valid instead.

Done.

[halliwell, 2016-01-15 02:04:54]

On 2016/01/15 02:04:10, emircan wrote:
>
https://codereview.chromium.org/1476523005/diff/360001/chromecast/renderer/me...
> File chromecast/renderer/media/hole_frame_factory.cc (right):
> 
>
https://codereview.chromium.org/1476523005/diff/360001/chromecast/renderer/me...
> chromecast/renderer/media/hole_frame_factory.cc:68: return nullptr;
> On 2016/01/15 01:52:04, halliwell wrote:
> > I'm not sure how useful this is.  If there is a problem here, it just pushes
> the
> > problem out a level.
> > 
> > This is a hardcoded simple placeholder frame used only to carry the overlay
> > information to the browser compositor.  It does not carry any actual video
> data.
> > 
> > If we get a nullptr here then nothing is going to function correctly.  I
would
> > say let's CHECK that frame is valid instead.
> 
> Done.

lgtm, thanks!

[emircan, 2016-01-15 07:46:03]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-01-15 07:46:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
aelias@chromium.org, dalecurtis@chromium.org, sievers@chromium.org,
posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/1476523005/#ps380001
(title: "halliwell@ comments.")

[commit-bot: I haz the power, 2016-01-15 07:46:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1476523005/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1476523005/380001

[commit-bot: I haz the power, 2016-01-15 08:59:43]

Committed patchset #12 (id:380001)

[commit-bot: I haz the power, 2016-01-15 09:00:45]

Description was changed from

==========
Verify returned frames from media::VideoFrame::Wrap*() methods

BUG=552112
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Verify returned frames from media::VideoFrame::Wrap*() methods

BUG=552112
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8428d41278c107c107736454dc4fdddbd14969bd
Cr-Commit-Position: refs/heads/master@{#369705}
==========

[commit-bot: I haz the power, 2016-01-15 09:00:46]

Patchset 12 (id:??) landed as
https://crrev.com/8428d41278c107c107736454dc4fdddbd14969bd
Cr-Commit-Position: refs/heads/master@{#369705}