Use webrtc::VideoFrame timestamp in RTCVideoEncoder

media/gpu/video_encode_accelerator_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <inttypes.h>
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 546 matching lines @@
   bool seen_sps_;
   bool seen_pps_;
   bool seen_idr_;
 
   H264Parser h264_parser_;
 };
 
 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;
 
     result = h264_parser_.AdvanceToNextNALU(&nalu);
     if (result == H264Parser::kEOStream)
       break;
 
     ASSERT_EQ(H264Parser::kOk, result);
 
     bool keyframe = false;
 
     switch (nalu.nal_unit_type) {
       case H264NALU::kIDRSlice:
         ASSERT_TRUE(seen_sps_);
         ASSERT_TRUE(seen_pps_);
         seen_idr_ = true;
         keyframe = true;
       // 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;
       }
 
       case H264NALU::kSPS: {
         int sps_id;
         ASSERT_EQ(H264Parser::kOk, h264_parser_.ParseSPS(&sps_id));
         seen_sps_ = true;
         break;
       }
 
@@ skipping 1229 matching lines @@
                         VideoEncodeAcceleratorTest,
                         ::testing::Values(std::make_tuple(3,
                                                           false,
                                                           0,
                                                           false,
                                                           false,
                                                           false,
                                                           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,
     VideoEncodeAcceleratorTest,
     ::testing::Values(
         std::make_tuple(1, false, 0, true, false, false, false, false, false)));
 #endif  // defined(OS_WIN)
 
 #endif  // defined(OS_CHROMEOS)
 
@@ skipping 85 matching lines @@
 
   media::g_env =
       reinterpret_cast<media::VideoEncodeAcceleratorTestEnvironment*>(
           testing::AddGlobalTestEnvironment(
               new media::VideoEncodeAcceleratorTestEnvironment(
                   std::move(test_stream_data), log_path, run_at_fps,
                   needs_encode_latency, verify_all_output)));
 
   return RUN_ALL_TESTS();
 }