Use webrtc::VideoFrame timestamp in RTCVideoEncoder

media/gpu/video_encode_accelerator_unittest.cc

Index: media/gpu/video_encode_accelerator_unittest.cc
diff --git a/media/gpu/video_encode_accelerator_unittest.cc b/media/gpu/video_encode_accelerator_unittest.cc
index 7ba3fe8d83e6ef8be99a1efb7b94f6d108a7a73a..5f582b053f51af9de5e0eda777927bd6bbcafe6e 100644
--- a/media/gpu/video_encode_accelerator_unittest.cc
+++ b/media/gpu/video_encode_accelerator_unittest.cc
@@ -564,6 +564,9 @@ class H264Validator : public StreamValidator {
 void H264Validator::ProcessStreamBuffer(const uint8_t* stream, size_t size) {
   h264_parser_.SetStream(stream, static_cast<off_t>(size));
 
+  // Run |frame_cb_| for only first nalu.
+  bool seen_nonidr = false;
+
   while (1) {
     H264NALU nalu;
     H264Parser::Result result;
@@ -585,8 +588,11 @@ void H264Validator::ProcessStreamBuffer(const uint8_t* stream, size_t size) {
       // fallthrough

[wuchengli, 2016/08/11 06:06:04]

This is fallthough. If there's only one kIDRSlice in the buffer, it's strange to
set |seen_nonidr| to true when actually only idr is seen.

       case H264NALU::kNonIDRSlice: {
         ASSERT_TRUE(seen_idr_);

[wuchengli, 2016/08/12 15:12:24]

Please add some comments to explain the assumption here. We assume one
BitstreamBufferReady can contain zero or one frame, not no more than one. So we
only call frame callback once.

FYI. Pawel suggested we could follow the code in
H264Decoder::IsNewPrimaryCodedPicture (#0) to decide if a slice is the start of
a new frame. That will solve the case when one BitstreamBufferReady contains
multiple slices or multiple frame. I think it's complicated and it's ok to just
have the assumption.

#0:
https://cs.chromium.org/chromium/src/media/gpu/h264_decoder.cc?rcl=0&l=1191

[emircan, 2016/08/24 21:29:17]

I am adding a comment about the assumption. 

-        if (!frame_cb_.Run(keyframe))
-          return;
+        if (!seen_nonidr) {
+          seen_nonidr = true;
+          if (!frame_cb_.Run(keyframe))

[wuchengli, 2016/08/11 06:06:04]

If we only call frame_cb_ once, we should probably do it after switch clause in
line 617. So we can pass keyframe==true if there's kIDRSlice. Otherwise, suppose
the first NALU is kNonIDRSlice and the second NALU is kIDRSlice. We will call
|frame_cb_| with false in that case. But I'd like to check with Pawel. He's OOO
today and will be back tomorrow. Let me get back to you tomorrow.

[emircan, 2016/08/11 16:49:15]

I thought of that case, but |seen_idr_| is only set when kIDRSlice is seen and
we have an assert here. Therefore, it should be fine to assume kNonIDRSlice does
not come before kIDRSlice. WDYT?

[wuchengli, 2016/08/12 15:12:25]

Yes. You are right. I didn't see the assertion.

Suppose the firts NALU is kIDRSlice and the second is kNonIDRSlice.
|seen_nonidr| will be true after the first NALU, but nonidr is not seen. How
about renaming |seen_nonidr| to |seen_slice| or |first_slice| or
|frame_cb_called|?

[emircan, 2016/08/24 21:29:17]

I agree |frame_cb_called| sounds better. Done.

+            return;
+        }
         break;
       }
 
@@ -1836,6 +1842,15 @@ INSTANTIATE_TEST_CASE_P(MultipleEncoders,
                                                           false,
                                                           false,
                                                           false)));
+
+#if defined(OS_MACOSX)
+INSTANTIATE_TEST_CASE_P(
+    VerifyTimestamp,
+    VideoEncodeAcceleratorTest,
+    ::testing::Values(
+        std::make_tuple(1, false, 0, false, false, false, false, false, true)));
+#endif  // defined(OS_MACOSX)
+
 #if defined(OS_WIN)
 INSTANTIATE_TEST_CASE_P(
     ForceBitrate,