VaapiVEA: Handle bitrate and frame rate negotiation from webrtc

VaapiVEA: Handle bitrate and frame rate negotiation from webrtc

The webrtc video encoder path will call into RTCVideoEncode::SetRates
with a zero framerate as part of the video encode setup.

A zero frame rate is not accepted by the VAAPI encoder and so we need to
bump the framerate up to a minimum of 1.

This change mimics the behavior of the V4L2 based encoder which does the
same thing to the framerate and bitrate.

TEST=Run chrome --enable-vaapi-accelerated-video-encode and use the
Google Cast extension to share the tab to the Chrome Cast.
BUG=391445
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284792

[Pawel Osciak, 2014-07-06 00:32:37]

lgtm

Looks like we should probably have this in RTCVE though...

[robert.bradford, 2014-07-07 10:29:30]

The CQ bit was checked by robert.bradford@intel.com

[commit-bot: I haz the power, 2014-07-07 10:30:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/robert.bradford@intel.com/369923008/1

[robert.bradford, 2014-07-07 12:25:43]

On 2014/07/06 00:32:37, Pawel Osciak wrote:
> lgtm
> 
> Looks like we should probably have this in RTCVE though...

I thought about that too, but it could be the case that the underlying encoder
could handle the zero framerate and use the bitrate for encode (or vice versa).
In my debugging zero framerate is only fed in when there is a valid bitrate
too...

[commit-bot: I haz the power, 2014-07-07 13:49:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-07 13:52:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-07 13:52:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[robert.bradford, 2014-07-07 14:11:06]

On 2014/07/07 13:52:34, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

Oops my mistake. Pawel isn't an OWNER here. xhwang@ can you take a look at this
change?

[xhwang, 2014-07-07 16:30:51]

https://codereview.chromium.org/369923008/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right):

https://codereview.chromium.org/369923008/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:751: framerate = 1;
I am not really familiar with this code, so some newbie questions:

1, Why can't webrtc change the default framerate to 1 so that VEAs don't need
this hack anymore. Otherwise, all VEAs need to do the same hack which sounds
bad.

2, What's the unit of |framerate| here, frame per second? If so, is 1 frame per
second a good choice for a default value?

[Pawel Osciak, 2014-07-08 08:18:32]

On 2014/07/07 16:30:51, xhwang wrote:
>
https://codereview.chromium.org/369923008/diff/1/content/common/gpu/media/vaa...
> File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/369923008/diff/1/content/common/gpu/media/vaa...
> content/common/gpu/media/vaapi_video_encode_accelerator.cc:751: framerate = 1;
> I am not really familiar with this code, so some newbie questions:
> 
> 1, Why can't webrtc change the default framerate to 1 so that VEAs don't need
> this hack anymore. Otherwise, all VEAs need to do the same hack which sounds
> bad.

I submitted issue 392086 for this. But it seems like there is also a bug in the
driver, which seems to accept 0 without returning an error. So this is guarding
against that as well. Robert, could this be fixed in the driver as well please? 

I think the long-term solution should be return an error to the client from VEAs
if we ever get 0 from it as an argument. We can do that once this is fixed.

> 2, What's the unit of |framerate| here, frame per second? If so, is 1 frame
per
> second a good choice for a default value?

Yes, frames per second. This is VideoEncoder interface in webrtc
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...),
which doesn't specify if "0" is a bad value, or means "choose your default". But
I personally think the latter doesn't make sense, if anything, webrtc should
provide its own default, so "1" is just saving ourselves from exploding on a
webrtc bug...

[robert.bradford, 2014-07-08 12:17:06]

> > 1, Why can't webrtc change the default framerate to 1 so that VEAs don't
need
> > this hack anymore. Otherwise, all VEAs need to do the same hack which sounds
> > bad.
> 
> I submitted issue 392086 for this. But it seems like there is also a bug in
the
> driver, which seems to accept 0 without returning an error. So this is
guarding
> against that as well. Robert, could this be fixed in the driver as well
please?

I don't think I can see that issue, 392086.

Looking at the driver, it ignores all VaEncMiscParameters except
VAEncMiscParameterTypeHRD and VAEncMiscParameterTypeQualityLevel. Perhaps we
should just drop the use of VAEncMiscParameterTypeRateControl and
VAEncMiscParameterTypeFrameRate from the VaapiVideoEncoder?

The framerate and bitrate is still used via the H264 specific parameter:
VAEncSequenceParameterBufferH264

> I think the long-term solution should be return an error to the client from
VEAs
> if we ever get 0 from it as an argument. We can do that once this is fixed.

Shall I work up a CL to do that for the VAAPI encoder?

[Pawel Osciak, 2014-07-09 02:31:45]

On 2014/07/08 12:17:06, robert.bradford wrote:
> > > 1, Why can't webrtc change the default framerate to 1 so that VEAs don't
> need
> > > this hack anymore. Otherwise, all VEAs need to do the same hack which
sounds
> > > bad.
> > 
> > I submitted issue 392086 for this. But it seems like there is also a bug in
> the
> > driver, which seems to accept 0 without returning an error. So this is
> guarding
> > against that as well. Robert, could this be fixed in the driver as well
> please?
> 
> I don't think I can see that issue, 392086.
> 
> Looking at the driver, it ignores all VaEncMiscParameters except
> VAEncMiscParameterTypeHRD and VAEncMiscParameterTypeQualityLevel. Perhaps we
> should just drop the use of VAEncMiscParameterTypeRateControl and
> VAEncMiscParameterTypeFrameRate from the VaapiVideoEncoder?
> 
> The framerate and bitrate is still used via the H264 specific parameter:
> VAEncSequenceParameterBufferH264
> 

If the driver implementation is incomplete, I would prefer the driver to be
fixed to be honest.

> > I think the long-term solution should be return an error to the client from
> VEAs
> > if we ever get 0 from it as an argument. We can do that once this is fixed.
> 
> Shall I work up a CL to do that for the VAAPI encoder?

This would break cast so please don't. We will fix this once the issue is
addressed.

[Pawel Osciak, 2014-07-20 01:44:10]

xhwang: ping please.

Please see my previous responses to your questions.

[xhwang, 2014-07-21 16:29:02]

On 2014/07/20 01:44:10, Pawel Osciak wrote:
> xhwang: ping please.
> 
> Please see my previous responses to your questions.

Pawel: You responses totally make sense to me. But I am not clear what the plan
is now. Are we going to land this CL as is and do what we mentioned as TODO in
the future, or are we going to fix the long term issue so that this CL can be of
less duplication?

[Pawel Osciak, 2014-07-22 10:29:38]

On 2014/07/21 16:29:02, xhwang wrote:
> On 2014/07/20 01:44:10, Pawel Osciak wrote:
> > xhwang: ping please.
> > 
> > Please see my previous responses to your questions.
> 
> Pawel: You responses totally make sense to me. But I am not clear what the
plan
> is now. Are we going to land this CL as is and do what we mentioned as TODO in
> the future, or are we going to fix the long term issue so that this CL can be
of
> less duplication?

This CL should land, I don't think this can be fixed soon enough for 38 and then
we have to remove it after 392086 is fixed.

[xhwang, 2014-07-22 16:37:33]

Pawel: Thanks for the explanations.

robert.bradford: Can you add a comment here about why we choose these default
values and possibly a TODO about fix the more general issue.

Otherwise, LGTM!

[robert.bradford, 2014-07-22 18:24:56]

The CQ bit was checked by robert.bradford@intel.com

[commit-bot: I haz the power, 2014-07-22 18:28:11]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/robert.bradford@intel.com/369923008/20001

[commit-bot: I haz the power, 2014-07-22 21:49:19]

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

[commit-bot: I haz the power, 2014-07-22 23:12:59]

Change committed as 284792