Don't re-randomize picture_id/tl0_pic_idx when re-initializing internal encoders.

webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc

 /*
  *  Copyright 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 // NOTICE: androidmediaencoder_jni.h must be included before
 // androidmediacodeccommon.h to avoid build errors.
 #include "webrtc/sdk/android/src/jni/androidmediaencoder_jni.h"
 
 #include <algorithm>
 #include <list>
 #include <memory>
 #include <string>
 #include <utility>
 
 #include "third_party/libyuv/include/libyuv/convert.h"
 #include "third_party/libyuv/include/libyuv/convert_from.h"
 #include "third_party/libyuv/include/libyuv/video_common.h"
 #include "webrtc/api/video_codecs/video_encoder.h"
 #include "webrtc/base/bind.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/base/random.h"
 #include "webrtc/base/sequenced_task_checker.h"
 #include "webrtc/base/task_queue.h"
 #include "webrtc/base/thread.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/base/weak_ptr.h"
 #include "webrtc/common_types.h"
 #include "webrtc/common_video/h264/h264_bitstream_parser.h"
 #include "webrtc/common_video/h264/h264_common.h"
 #include "webrtc/common_video/h264/profile_level_id.h"
 #include "webrtc/media/engine/internalencoderfactory.h"
@@ skipping 177 matching lines @@
   jfieldID j_info_index_field_;
   jfieldID j_info_buffer_field_;
   jfieldID j_info_is_key_frame_field_;
   jfieldID j_info_presentation_timestamp_us_field_;
 
   // State that is valid only between InitEncode() and the next Release().
   int width_;   // Frame width in pixels.
   int height_;  // Frame height in pixels.
   bool inited_;
   bool use_surface_;
-  uint16_t picture_id_;
   enum libyuv::FourCC encoder_fourcc_;  // Encoder color space format.
   int last_set_bitrate_kbps_;  // Last-requested bitrate in kbps.
   int last_set_fps_;  // Last-requested frame rate.
   int64_t current_timestamp_us_;  // Current frame timestamps in us.
   int frames_received_;  // Number of frames received by encoder.
   int frames_encoded_;  // Number of frames encoded by encoder.
   int frames_dropped_media_encoder_;  // Number of frames dropped by encoder.
   // Number of dropped frames caused by full queue.
   int consecutive_full_queue_frame_drops_;
   int64_t stat_start_time_ms_;  // Start time for statistics.
@@ skipping 23 matching lines @@
     const int64_t frame_render_time_ms;
     const webrtc::VideoRotation rotation;
   };
   std::list<InputFrameInfo> input_frame_infos_;
   int32_t output_timestamp_;       // Last output frame timestamp from
                                    // |input_frame_infos_|.
   int64_t output_render_time_ms_;  // Last output frame render time from
                                    // |input_frame_infos_|.
   webrtc::VideoRotation output_rotation_;  // Last output frame rotation from
                                            // |input_frame_infos_|.
+
   // Frame size in bytes fed to MediaCodec.
   int yuv_size_;
   // True only when between a callback_->OnEncodedImage() call return a positive
   // value and the next Encode() call being ignored.
   bool drop_next_input_frame_;
   bool scale_;
   // Global references; must be deleted in Release().
   std::vector<jobject> input_buffers_;
   webrtc::H264BitstreamParser h264_bitstream_parser_;
 
   // VP9 variables to populate codec specific structure.
   webrtc::GofInfoVP9 gof_;  // Contains each frame's temporal information for
                             // non-flexible VP9 mode.
-  uint8_t tl0_pic_idx_;
   size_t gof_idx_;
 
   // EGL context - owned by factory, should not be allocated/destroyed
   // by MediaCodecVideoEncoder.
   jobject egl_context_;
 
   // Temporary fix for VP8.
   // Sends a key frame if frames are largely spaced apart (possibly
   // corresponding to a large image change).
   int64_t last_frame_received_ms_;
   int frames_received_since_last_key_;
   webrtc::VideoCodecMode codec_mode_;
 
+  // Used to randomize the initial |picture_id_| and |tl0_pic_idx_|.
+  webrtc::Random random_;

[magjed_webrtc, 2017/04/21 15:11:18]

I would like to not keep this as a member variable since it's only used in the
ctor, so I prefer to move the initialization of picture_id_ and tl0_pic_idx_
into the ctor body and having random_ as a local variable.

[brandtr, 2017/04/24 07:53:47]

Good idea. Done.

+
+  // RTP state.
+  uint16_t picture_id_;
+  uint8_t tl0_pic_idx_;
+
   bool sw_fallback_required_;
 
   // All other member variables should be before WeakPtrFactory. Valid only from
   // InitEncode to Release.
   std::unique_ptr<rtc::WeakPtrFactory<MediaCodecVideoEncoder>> weak_factory_;
 };
 
 MediaCodecVideoEncoder::~MediaCodecVideoEncoder() {
 #if RTC_DCHECK_IS_ON
   rtc::CritScope lock(&inited_crit_);
@@ skipping 11 matching lines @@
           FindClass(jni, "org/webrtc/MediaCodecVideoEncoder")),
       j_media_codec_video_encoder_(
           jni,
           jni->NewObject(*j_media_codec_video_encoder_class_,
                          GetMethodID(jni,
                                      *j_media_codec_video_encoder_class_,
                                      "<init>",
                                      "()V"))),
       inited_(false),
       use_surface_(false),
-      picture_id_(0),
       egl_context_(egl_context),
+      random_(rtc::TimeMicros()),
+      picture_id_(random_.Rand<uint16_t>() & 0x7FFF),

[magjed_webrtc, 2017/04/20 12:39:12]

Is it possible to pass in the start |picture_id_| and |tl0_pic_idx_| to this
class instead of using a random number generator?

[brandtr, 2017/04/21 08:29:31]

That is something that we maybe want to do long-term. Then we would treat
PictureID and Tl0PicIdx as part of the RTP state, which is retained at the Call
level. Making that change would be a medium-sized refactoring however, including
changing the encoder interface.

For now, I think the change in this CL is an improvement over the current
behaviour however. Together with https://codereview.webrtc.org/2830793005/, we
will stop re-randomizing PictureID during a call, thus making live easier for
the jitter buffer.

According to https://tools.ietf.org/html/rfc7741#section-4.2, we don't
necessarily have to randomize the initial PictureID at all. I'm not sure if
there is any benefit from not randomizing it, except for not having to
instantiate a PRNG here.

[magjed_webrtc, 2017/04/21 15:11:18]

Acknowledged.

+      tl0_pic_idx_(random_.Rand<uint8_t>()),
       sw_fallback_required_(false) {
   encoder_queue_checker_.Detach();
 
   jclass j_output_buffer_info_class =
       FindClass(jni, "org/webrtc/MediaCodecVideoEncoder$OutputBufferInfo");
   j_init_encode_method_ = GetMethodID(
       jni,
       *j_media_codec_video_encoder_class_,
       "initEncode",
       "(Lorg/webrtc/MediaCodecVideoEncoder$VideoCodecType;"
@@ skipping 29 matching lines @@
   j_info_buffer_field_ = GetFieldID(
       jni, j_output_buffer_info_class, "buffer", "Ljava/nio/ByteBuffer;");
   j_info_is_key_frame_field_ =
       GetFieldID(jni, j_output_buffer_info_class, "isKeyFrame", "Z");
   j_info_presentation_timestamp_us_field_ = GetFieldID(
       jni, j_output_buffer_info_class, "presentationTimestampUs", "J");
   if (CheckException(jni)) {
     ALOGW << "MediaCodecVideoEncoder ctor failed.";
     ProcessHWError(true /* reset_if_fallback_unavailable */);
   }
-  srand(rtc::Time32());
 }
 
 int32_t MediaCodecVideoEncoder::InitEncode(
     const webrtc::VideoCodec* codec_settings,
     int32_t /* number_of_cores */,
     size_t /* max_payload_size */) {
   RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_checker_);
   if (codec_settings == NULL) {
     ALOGE << "NULL VideoCodec instance";
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
@@ skipping 156 matching lines @@
   current_bytes_ = 0;
   current_acc_qp_ = 0;
   current_encoding_time_ms_ = 0;
   last_input_timestamp_ms_ = -1;
   last_output_timestamp_ms_ = -1;
   output_timestamp_ = 0;
   output_render_time_ms_ = 0;
   input_frame_infos_.clear();
   drop_next_input_frame_ = false;
   use_surface_ = use_surface;
-  // TODO(ilnik): Use rand_r() instead to avoid LINT warnings below.
-  picture_id_ = static_cast<uint16_t>(rand()) & 0x7FFF;  // NOLINT
   gof_.SetGofInfoVP9(webrtc::TemporalStructureMode::kTemporalStructureMode1);
-  tl0_pic_idx_ = static_cast<uint8_t>(rand());  // NOLINT
   gof_idx_ = 0;
   last_frame_received_ms_ = -1;
   frames_received_since_last_key_ = kMinKeyFrameInterval;
 
   // We enforce no extra stride/padding in the format creation step.
   jobject j_video_codec_enum = JavaEnumFromIndexAndClassName(
       jni, "MediaCodecVideoEncoder$VideoCodecType", codec_type);
   const bool encode_status = jni->CallBooleanMethod(
       *j_media_codec_video_encoder_, j_init_encode_method_,
       j_video_codec_enum, width, height, kbps, fps,
@@ skipping 779 matching lines @@
   }
 }
 
 void MediaCodecVideoEncoderFactory::DestroyVideoEncoder(
     webrtc::VideoEncoder* encoder) {
   ALOGD << "Destroy video encoder.";
   delete encoder;
 }
 
 }  // namespace webrtc_jni