Cast: Use fixed bitrate and set it once for hardware encoder

Cast: Use fixed bitrate and set it once for hardware encoder

Some hardware encoder cannot handle bitrate changing too quickly. The
solution is to have a fixed bitrate and only set it once.

BUG=392086
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287721

[miu, 2014-08-05 02:51:00]

IMHO, I feel this approach is "papering over" the real issue.  There was
discussion in the bug about two simple things we could do to resolve the
problem:

1) If the bitrate change is small, we don't change it.
2) Only change the bitrate once per 1-2 seconds, like WebRTC.

For our case, we could do something a little bit smarter.  For example:

1) If the bitrate change is small, we don't change it.
2) Change the bitrate any time we want it to drop "significantly" (i.e., as a
quick, reactive response to the sudden onset of congestion), with no time
restriction.
3) Otherwise, only change the bitrate once per 1-2 seconds.

Also, whatever we do for hardware accelerated encoding, it should apply to the
software encoder also.  That way, we'll know our congestion control works as we
develop w/ the software encoder.

[miu, 2014-08-05 21:20:09]

Okay.  After discussion, I'm okay with this change.  Basically, this is needed
to prevent regressing performance when the hardware encoder is turned on.  The
"right solution" to the problem will be determined at a later time.

[miu, 2014-08-05 21:52:07]

https://codereview.chromium.org/439863003/diff/20001/media/cast/sender/extern...
File media/cast/sender/external_video_encoder_unittest.cc (right):

https://codereview.chromium.org/439863003/diff/20001/media/cast/sender/extern...
media/cast/sender/external_video_encoder_unittest.cc:132: std::vector<uint32>
stored_bitrates_;
This should be checked in one or more unit tests (just like in
video_sender_unittest.cc).

https://codereview.chromium.org/439863003/diff/20001/media/cast/sender/video_...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/439863003/diff/20001/media/cast/sender/video_...
media/cast/sender/video_sender.cc:135: uint32 bitrate = fixed_bitrate_;
nit/bike-shedding: How about killing the extra |bitrate| variable to help
readability?  Meaning:

  if (!fixed_bitrate_) {
    video_encoder_->SetBitRate(congestion_control_.GetBitrate(
        capture_time + target_playout_delay_, target_playout_delay_));
  } else if (last_send_time_.is_null()) {
    // ...comment here...
    video_encoder_->SetBitRate(fixed_bitrate_);
  }

https://codereview.chromium.org/439863003/diff/20001/media/cast/test/fake_vid...
File media/cast/test/fake_video_encode_accelerator.cc (right):

https://codereview.chromium.org/439863003/diff/20001/media/cast/test/fake_vid...
media/cast/test/fake_video_encode_accelerator.cc:23:
stored_bitrates_(stored_bitrates),
For safety, DCHECK(stored_bitrates_), since it is assumed not to be NULL. 
Alternatively, do null checking in RequestEncodingParametersChange().

https://codereview.chromium.org/439863003/diff/20001/media/cast/test/fake_vid...
File media/cast/test/fake_video_encode_accelerator.h (right):

https://codereview.chromium.org/439863003/diff/20001/media/cast/test/fake_vid...
media/cast/test/fake_video_encode_accelerator.h:57:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
Can you make these two members const?

[Alpha Left Google, 2014-08-06 01:24:55]

https://codereview.chromium.org/439863003/diff/20001/media/cast/sender/extern...
File media/cast/sender/external_video_encoder_unittest.cc (right):

https://codereview.chromium.org/439863003/diff/20001/media/cast/sender/extern...
media/cast/sender/external_video_encoder_unittest.cc:132: std::vector<uint32>
stored_bitrates_;
On 2014/08/05 21:52:07, miu wrote:
> This should be checked in one or more unit tests (just like in
> video_sender_unittest.cc).

Done.

https://codereview.chromium.org/439863003/diff/20001/media/cast/sender/video_...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/439863003/diff/20001/media/cast/sender/video_...
media/cast/sender/video_sender.cc:135: uint32 bitrate = fixed_bitrate_;
On 2014/08/05 21:52:07, miu wrote:
> nit/bike-shedding: How about killing the extra |bitrate| variable to help
> readability?  Meaning:
> 
>   if (!fixed_bitrate_) {
>     video_encoder_->SetBitRate(congestion_control_.GetBitrate(
>         capture_time + target_playout_delay_, target_playout_delay_));
>   } else if (last_send_time_.is_null()) {
>     // ...comment here...
>     video_encoder_->SetBitRate(fixed_bitrate_);
>   }

bitrate is used in line 153.

https://codereview.chromium.org/439863003/diff/20001/media/cast/test/fake_vid...
File media/cast/test/fake_video_encode_accelerator.cc (right):

https://codereview.chromium.org/439863003/diff/20001/media/cast/test/fake_vid...
media/cast/test/fake_video_encode_accelerator.cc:23:
stored_bitrates_(stored_bitrates),
On 2014/08/05 21:52:07, miu wrote:
> For safety, DCHECK(stored_bitrates_), since it is assumed not to be NULL. 
> Alternatively, do null checking in RequestEncodingParametersChange().

Done.

https://codereview.chromium.org/439863003/diff/20001/media/cast/test/fake_vid...
File media/cast/test/fake_video_encode_accelerator.h (right):

https://codereview.chromium.org/439863003/diff/20001/media/cast/test/fake_vid...
media/cast/test/fake_video_encode_accelerator.h:57:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2014/08/05 21:52:07, miu wrote:
> Can you make these two members const?

Done.

[miu, 2014-08-06 01:26:58]

lgtm

[commit-bot: I haz the power, 2014-08-06 01:30:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/439863003/40001

[commit-bot: I haz the power, 2014-08-06 04:48:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-06 07:06:09]

Change committed as 287721