Fix the issue of undefined-shift in VP8GetBit.

webrtc/modules/video_coding/utility/vp8_header_parser.cc

 /*
  *  Copyright (c) 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.
  */
+
+#include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"

[pbos-webrtc, 2016/04/18 15:10:14]

These should go below where they were, vp8_header_parser.h should be included
first since it belongs to this .cc file.

[jackychen_, 2016/04/18 19:18:17]

Done.

 #include "webrtc/modules/video_coding/utility/vp8_header_parser.h"
 
-#include "webrtc/base/logging.h"
-
 namespace webrtc {
 
 namespace vp8 {
 namespace {
 const size_t kCommonPayloadHeaderLength = 3;
 const size_t kKeyPayloadHeaderLength = 10;
 }  // namespace
 
 static uint32_t BSwap32(uint32_t x) {
   return (x >> 24) | ((x >> 8) & 0xff00) | ((x << 8) & 0xff0000) | (x << 24);
@@ skipping 44 matching lines @@
   br->eof_ = 0;
   VP8LoadNewBytes(br);
 }
 
 // Read a bit with proba 'prob'.
 static int VP8GetBit(VP8BitReader* const br, int prob) {
   uint8_t range = br->range_;
   if (br->bits_ < 0) {
     VP8LoadNewBytes(br);
   }
-
+  RTC_CHECK_GE(br->bits_, 0);

[pbos-webrtc, 2016/04/18 15:10:14]

You can't use this for network data. We cannot crash here (which RTC_CHECK makes
us do). We need to be able to abort gracefully here.

Perhaps this function should be:

static bool VP8GetBit(VP8BitReader* br, int* out_bit, int prob), and return
true/false based on success. Then VP8LoadNewBytes should be able to pass/fail.

[jackychen_, 2016/04/18 19:18:17]

When VP8GetBit read invalid bitstream, it should pass the failure status all the
way up to GetQp to indicate the QP read for this frame is not usable. I think
your comment in patch #1 is right, the status should be a property of bitReader.
I misunderstood your comment about "abort", I thought you mean "crash", my bad!

   const int pos = br->bits_;
   const uint8_t split = (range * prob) >> 8;
   const uint8_t value = static_cast<uint8_t>(br->value_ >> pos);
   int bit;
   if (value > split) {
     range -= split + 1;
     br->value_ -= static_cast<uint32_t>(split + 1) << pos;
     bit = 1;
   } else {
     range = split;
@@ skipping 106 matching lines @@
     LOG(LS_WARNING) << "Failed to get QP, end of file reached.";
     return false;
   }
   *qp = base_q0;
   return true;
 }
 
 }  // namespace vp8
 
 }  // namespace webrtc