Integration of VP9 packetization.

Integration of VP9 packetization.

Supports running 1 spatial and 1-3 temporal layers in non-flexible mode.

BUG=webrtc:4148, webrtc:4168, chromium:500602
TBR=mflodman

Committed: https://crrev.com/a9455ab235e1169572f9eae3175cd9310d6729e2
Cr-Commit-Position: refs/heads/master@{#9665}

[åsapersson, 2015-06-26 13:18:24]

Patchset #7 (id:110001) has been deleted

[åsapersson, 2015-06-26 13:37:04]

Patchset #8 (id:150001) has been deleted

[åsapersson, 2015-06-30 14:52:37]

Patchset #13 (id:270001) has been deleted

[åsapersson, 2015-06-30 15:40:50]

Patchset #13 (id:290001) has been deleted

[åsapersson, 2015-07-01 09:56:47]

Patchset #14 (id:330001) has been deleted

[åsapersson, 2015-07-07 10:36:07]

Patchset #21 (id:480001) has been deleted

[åsapersson, 2015-07-09 11:23:43]

Patchset #25 (id:560001) has been deleted

[åsapersson, 2015-07-09 11:23:55]

Patchset #24 (id:360029) has been deleted

[åsapersson, 2015-07-09 11:24:18]

Patchset #24 (id:580001) has been deleted

[åsapersson, 2015-07-09 11:31:04]

Patchset #22 (id:510001) has been deleted

[åsapersson, 2015-07-09 11:31:31]

Patchset #20 (id:460001) has been deleted

[åsapersson, 2015-07-09 11:31:47]

Patchset #20 (id:500001) has been deleted

[åsapersson, 2015-07-09 11:32:08]

Patchset #19 (id:440001) has been deleted

[åsapersson, 2015-07-09 11:32:28]

Patchset #8 (id:170001) has been deleted

[åsapersson, 2015-07-09 11:32:39]

Patchset #8 (id:190001) has been deleted

[åsapersson, 2015-07-09 11:33:03]

Patchset #9 (id:230001) has been deleted

[åsapersson, 2015-07-09 11:33:16]

Patchset #11 (id:350001) has been deleted

[åsapersson, 2015-07-09 11:34:09]

Patchset #12 (id:390001) has been deleted

[åsapersson, 2015-07-09 12:03:39]

Patchset #16 (id:620001) has been deleted

[åsapersson, 2015-07-09 12:04:12]

Patchset #15 (id:600001) has been deleted

[åsapersson, 2015-07-09 12:19:25]

asapersson@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2015-07-09 14:49:00]

A first pass

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/interface...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:134: uint8_t numFramesInGof;
Might want to make this and the other types below int, except for the bool.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:173: uint8_t spatialIdx;     //
Spatial layer index, or kNoSpatialIdx.
WDYT of making these indices int? Same below for gofIdx for instance.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:188: uint8_t numSpatialLayers;
int

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:191: uint16_t
height[kMaxVp9NumberOfSpatialLayers];
int

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:722: // Is it per super frame
or layer frame?
b_bit is the first packet of a layer frame, so assuming the spatialIdx is 0 then
it should be the first packet of the super frame.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:98: float
sRateRatio[VPX_MAX_LAYERS] = {0};
No camelCase.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:105:
!svc_internal_.svc_params.scaling_factor_den[i]) {
svc_internal_.svc_params.scaling_factor_den[i] <= 0?

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:109:
(float)(svc_internal_.svc_params.scaling_factor_num[i] * 1.0 /
static_cast. I don't think the * 1.0 matters.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:116: 
Assert that total = 1?

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:119: (unsigned
int)(config_->rc_target_bitrate * sRateRatio[i] / total);
static_cast

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:143:
config_->ts_target_bitrate[i] = config_->layer_target_bitrate[i];
Are all VPX_TS_MAX_LAYERS elements of layer_target_bitrate set? Shouldn't there
be a length field we can read instead?

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:147: return
WEBRTC_VIDEO_CODEC_OK;
Make this method return bool instead.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:214: // have number of
spatial layers and temporal structure mode set.
Not sure if this comment is useful?

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:344: /* 1:2 scaling in each
dimension */
Fix this comment.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:346: 256 >>
(number_of_spatial_layers_ - i - 1);
Can't we divide instead of shifting? I think it makes it a lot easier to read.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:479: vpx_svc_layer_id_t lyr =
{0};
layer_id

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:61: // Callback function for
outputting packets per spatial layer.
Newline above.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:62: static void
EncocderOutputCodedPacketCb(vpx_codec_cx_pkt* pkt,
Cb -> Callback.

Maybe move this to the .cc file? I also don't think it needs to be a static
method of the class, just make it a function.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:93: uint8_t
number_of_spatial_layers_;
num_temporal_layers_
num_spatial_layers_

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/encoded_frame.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/encoded_frame.cc:186: for (uint8_t s =
0; s < header->codecHeader.VP9.numSpatialLayers;
Index i instead of s

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/encoded_frame.cc:187: s++) {
++i

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:636:
packet.codecSpecificHeader.codecHeader.VP9.gof);
This gof info is necessary to know if a frame is decodable or not, so therefore
I think it should be stored in decoding_state.h, and should be necessary to use
by that class to figure out decodability.

Right now I don't think this code will be able to continue decoding if it loses
a packet in TL1 if it only has two layers, right? Or maybe GOF is only needed
for handling spatial layers and packet loss?

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/session_info.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/session_info.cc:68: else
This starts to grow out of control... We should think about a way of making this
code more generic. Do you have any suggestions up front?

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/session_info.cc:98: return kNoTl0PicIdx;
{} for all multi-line if-statements.

[åsapersson, 2015-07-10 11:56:12]

Patchset #16 (id:660001) has been deleted

[åsapersson, 2015-07-28 15:22:36]

Patchset #20 (id:760001) has been deleted

[åsapersson, 2015-07-29 08:44:14]

Patchset #18 (id:720001) has been deleted

[åsapersson, 2015-07-29 12:10:13]

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:722: // Is it per super frame
or layer frame?
On 2015/07/09 14:48:58, stefan-webrtc (holmer) wrote:
> b_bit is the first packet of a layer frame, so assuming the spatialIdx is 0
then
> it should be the first packet of the super frame.

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:98: float
sRateRatio[VPX_MAX_LAYERS] = {0};
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> No camelCase.

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:105:
!svc_internal_.svc_params.scaling_factor_den[i]) {
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> svc_internal_.svc_params.scaling_factor_den[i] <= 0?

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:109:
(float)(svc_internal_.svc_params.scaling_factor_num[i] * 1.0 /
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> static_cast. I don't think the * 1.0 matters.

Changed parenthesis -> removed 1.0.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:116: 
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> Assert that total = 1?

added assert >=1?

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:119: (unsigned
int)(config_->rc_target_bitrate * sRateRatio[i] / total);
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> static_cast

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:143:
config_->ts_target_bitrate[i] = config_->layer_target_bitrate[i];
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> Are all VPX_TS_MAX_LAYERS elements of layer_target_bitrate set? Shouldn't
there
> be a length field we can read instead?

Changed to number of temporal layers.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:147: return
WEBRTC_VIDEO_CODEC_OK;
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> Make this method return bool instead.

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:214: // have number of
spatial layers and temporal structure mode set.
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> Not sure if this comment is useful?

Removed.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:344: /* 1:2 scaling in each
dimension */
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> Fix this comment.

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:346: 256 >>
(number_of_spatial_layers_ - i - 1);
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> Can't we divide instead of shifting? I think it makes it a lot easier to read.

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:479: vpx_svc_layer_id_t lyr =
{0};
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> layer_id

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:61: // Callback function for
outputting packets per spatial layer.
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> Newline above.

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:62: static void
EncocderOutputCodedPacketCb(vpx_codec_cx_pkt* pkt,
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> Cb -> Callback.
> 
> Maybe move this to the .cc file? I also don't think it needs to be a static
> method of the class, just make it a function.

Renamed and moved to cc-file.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:93: uint8_t
number_of_spatial_layers_;
On 2015/07/09 14:48:59, stefan-webrtc (holmer) wrote:
> num_temporal_layers_
> num_spatial_layers_

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/encoded_frame.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/encoded_frame.cc:186: for (uint8_t s =
0; s < header->codecHeader.VP9.numSpatialLayers;
On 2015/07/09 14:49:00, stefan-webrtc (holmer) wrote:
> Index i instead of s

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/encoded_frame.cc:187: s++) {
On 2015/07/09 14:49:00, stefan-webrtc (holmer) wrote:
> ++i

Done.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:636:
packet.codecSpecificHeader.codecHeader.VP9.gof);
On 2015/07/09 14:49:00, stefan-webrtc (holmer) wrote:
> This gof info is necessary to know if a frame is decodable or not, so
therefore
> I think it should be stored in decoding_state.h, and should be necessary to
use
> by that class to figure out decodability.
> 
> Right now I don't think this code will be able to continue decoding if it
loses
> a packet in TL1 if it only has two layers, right? Or maybe GOF is only needed
> for handling spatial layers and packet loss?

Decoding state looks at frame's TL, etc. Shouldn't the frame be updated with
correct values before inserted to frame buffer? Added a comment that this code
should be moved.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/session_info.cc (right):

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/session_info.cc:68: else
On 2015/07/09 14:49:00, stefan-webrtc (holmer) wrote:
> This starts to grow out of control... We should think about a way of making
this
> code more generic. Do you have any suggestions up front?

Agree. Maybe in a separate cl.

https://codereview.webrtc.org/1211353002/diff/640001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/session_info.cc:98: return kNoTl0PicIdx;
On 2015/07/09 14:49:00, stefan-webrtc (holmer) wrote:
> {} for all multi-line if-statements.

Done.

[stefan-webrtc, 2015-07-29 13:13:05]

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/interface...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:100: case
kTemporalStructureMode3:
We probably want to change this to make the pattern 8 frames rather than 4. But
let's do that in a follow-up.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/interface/video_codec_interface.h
(right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/interface/video_codec_interface.h:52: bool
inter_pic_predicted;
I think it would be good to have comments describing this flag and the
inter_layer_predicted flag.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:134: assert(total >= 1.0f);
Hm, seems like there could be a risk that this doesn't sum up to 1.0f exactly
due to precision? Might be better to not do this assert.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:138:
config_->rc_target_bitrate * rate_ratio[i] / total);
Should this really divide by total? total is expected to be 1, isn't it?

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:159: if (num_spatial_layers_
== 1) {
Comment: Currently only support temporal layers if spatial layers are disabled?
Or do you think this means one spatial layer + base layer?

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:234: if (num_spatial_layers_
!= 1) {
Does this mean one more than the base layer?

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:323: }
Ideally I would like this code to be shared with the code we're adding in
module_common_types.h. It is basically the same thing, right? Can we reuse some
of it?

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:360: for (int i =
num_spatial_layers_ - 1; i >= 0; --i, scaling_factor_num /= 2) {
Move scaling_factor_num /= 2 to line 366 instead.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:386:
&svc_internal_.svc_params);
Should this not be called if num_temporal_layers_ > 1? Seems a bit confusing
that VP9E_SET_SVC is enabled in that case, but the params are not set?

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:486: vp9_info->end_of_frame =
true;
Seems like these two flags aren't really needed on this level. We always know we
are having full frames here?

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:491:
vpx_codec_control(encoder_, VP9E_GET_SVC_LAYER_ID, &layer_id);
Do you know how this works? Does it return the spatial layer and temporal layer
ids for the last encoded frame?

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:509: }
Seems like we should DCHECK(layer_id.spatial_layer_id == 0 ||
!vp9_info->ss_data_available) instead

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:541: for (uint8_t i = 0; i <
vp9_info->num_spatial_layers; i++) {
++i

and make i an int.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:577: if
((pkt->data.frame.flags & VPX_FRAME_IS_FRAGMENT) == 0) {
I doubt this check is needed. Feel free to try to remove it. We should never be
getting fragments from the vp9 encoder as far as I know.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/encoded_frame.cc (right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/encoded_frame.cc:183:
_codecSpecificInfo.codecSpecific.VP9.spatial_layer_resolution_present=
Space before =

[åsapersson, 2015-07-30 09:17:33]

Patchset #21 (id:820001) has been deleted

[åsapersson, 2015-07-30 11:42:20]

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/interface...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:100: case
kTemporalStructureMode3:
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> We probably want to change this to make the pattern 8 frames rather than 4.
But
> let's do that in a follow-up.

Added a comment in vp9_impl.cc.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/interface/video_codec_interface.h
(right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/interface/video_codec_interface.h:52: bool
inter_pic_predicted;
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> I think it would be good to have comments describing this flag and the
> inter_layer_predicted flag.

Done.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:134: assert(total >= 1.0f);
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Hm, seems like there could be a risk that this doesn't sum up to 1.0f exactly
> due to precision? Might be better to not do this assert.

Done.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:138:
config_->rc_target_bitrate * rate_ratio[i] / total);
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Should this really divide by total? total is expected to be 1, isn't it?

total will be >1 for num_spatial_layers_ > 1...

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:159: if (num_spatial_layers_
== 1) {
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Comment: Currently only support temporal layers if spatial layers are
disabled?
> Or do you think this means one spatial layer + base layer?

Added a comment.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:234: if (num_spatial_layers_
!= 1) {
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Does this mean one more than the base layer?

Only one spatial layer.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:323: }
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Ideally I would like this code to be shared with the code we're adding in
> module_common_types.h. It is basically the same thing, right? Can we reuse
some
> of it?

Leave for follow up cl?

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:360: for (int i =
num_spatial_layers_ - 1; i >= 0; --i, scaling_factor_num /= 2) {
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Move scaling_factor_num /= 2 to line 366 instead.

Done.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:386:
&svc_internal_.svc_params);
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Should this not be called if num_temporal_layers_ > 1? Seems a bit confusing
> that VP9E_SET_SVC is enabled in that case, but the params are not set?

Done.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:486: vp9_info->end_of_frame =
true;
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Seems like these two flags aren't really needed on this level. We always know
we
> are having full frames here?

Removed these settings from CodecSpecificInfo and the usage of corresponding
fields in RTPVideoHeaderVP9 when packetizing payload.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:491:
vpx_codec_control(encoder_, VP9E_GET_SVC_LAYER_ID, &layer_id);
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Do you know how this works? Does it return the spatial layer and temporal
layer
> ids for the last encoded frame?

Yes it looks so.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:509: }
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Seems like we should DCHECK(layer_id.spatial_layer_id == 0 ||
> !vp9_info->ss_data_available) instead

Skipped this as discussed offline.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:541: for (uint8_t i = 0; i <
vp9_info->num_spatial_layers; i++) {
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> ++i
> 
> and make i an int.

Changed to size_t which num_spatial_layers is.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:577: if
((pkt->data.frame.flags & VPX_FRAME_IS_FRAGMENT) == 0) {
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> I doubt this check is needed. Feel free to try to remove it. We should never
be
> getting fragments from the vp9 encoder as far as I know.

Seems ok to remove, done.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/encoded_frame.cc (right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/encoded_frame.cc:183:
_codecSpecificInfo.codecSpecific.VP9.spatial_layer_resolution_present=
On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> Space before =

long line, changed format

[stefan-webrtc, 2015-07-30 12:05:25]

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right):

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:138:
config_->rc_target_bitrate * rate_ratio[i] / total);
On 2015/07/30 11:42:19, asapersson wrote:
> On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> > Should this really divide by total? total is expected to be 1, isn't it?
> 
> total will be >1 for num_spatial_layers_ > 1...

Right, my mistake.

https://codereview.webrtc.org/1211353002/diff/800001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:323: }
On 2015/07/30 11:42:19, asapersson wrote:
> On 2015/07/29 13:13:05, stefan-webrtc (holmer) wrote:
> > Ideally I would like this code to be shared with the code we're adding in
> > module_common_types.h. It is basically the same thing, right? Can we reuse
> some
> > of it?
> 
> Leave for follow up cl?

Acknowledged.

https://codereview.webrtc.org/1211353002/diff/870001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/1211353002/diff/870001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:1369: size_t numTemporalLayer =
(video_codec_type_ == kVideoCodecVP9) ?
num_temporal_layers

[åsapersson, 2015-07-30 16:24:58]

Patchset #24 (id:890001) has been deleted

[åsapersson, 2015-07-30 16:29:26]

https://codereview.webrtc.org/1211353002/diff/870001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/1211353002/diff/870001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:1369: size_t numTemporalLayer =
(video_codec_type_ == kVideoCodecVP9) ?
On 2015/07/30 12:05:25, stefan-webrtc (holmer) wrote:
> num_temporal_layers

Done.

[åsapersson, 2015-07-31 06:28:21]

Patchset #23 (id:870001) has been deleted

[åsapersson, 2015-07-31 08:19:50]

Patchset #26 (id:970001) has been deleted

[holmer, 2015-07-31 09:05:25]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2015-07-31 09:05:26]

lgtm

https://codereview.webrtc.org/1211353002/diff/990001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right):

https://codereview.webrtc.org/1211353002/diff/990001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:212: if
(inst->codecSpecific.VP9.numberOfTemporalLayers < 1 ||
Should 0 be allowed as "no temporal layers"?

[åsapersson, 2015-07-31 10:09:37]

Did some additional changes, ptal.

https://codereview.webrtc.org/1211353002/diff/990001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right):

https://codereview.webrtc.org/1211353002/diff/990001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:212: if
(inst->codecSpecific.VP9.numberOfTemporalLayers < 1 ||
On 2015/07/31 09:05:26, holmer wrote:
> Should 0 be allowed as "no temporal layers"?

See change.

[holmer, 2015-07-31 10:51:36]

https://codereview.webrtc.org/1211353002/diff/1020001/webrtc/modules/video_co...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1211353002/diff/1020001/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:603: if
(first_packet_since_reset_ || last_gof_timestamp_ == 0 ||
 Does this mean uninitialized? Should it in that case be -1 instead?

[åsapersson, 2015-07-31 11:45:07]

https://codereview.webrtc.org/1211353002/diff/1020001/webrtc/modules/video_co...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1211353002/diff/1020001/webrtc/modules/video_co...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:603: if
(first_packet_since_reset_ || last_gof_timestamp_ == 0 ||
On 2015/07/31 10:51:36, holmer wrote:
>  Does this mean uninitialized? Should it in that case be -1 instead?

Changed to check last_gof_idx instead.

[stefan-webrtc, 2015-07-31 12:01:41]

On 2015/07/31 11:45:07, asapersson wrote:
>
https://codereview.webrtc.org/1211353002/diff/1020001/webrtc/modules/video_co...
> File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):
> 
>
https://codereview.webrtc.org/1211353002/diff/1020001/webrtc/modules/video_co...
> webrtc/modules/video_coding/main/source/jitter_buffer.cc:603: if
> (first_packet_since_reset_ || last_gof_timestamp_ == 0 ||
> On 2015/07/31 10:51:36, holmer wrote:
> >  Does this mean uninitialized? Should it in that case be -1 instead?
> 
> Changed to check last_gof_idx instead.

Lgtm

[åsapersson, 2015-07-31 12:25:55]

asapersson@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[åsapersson, 2015-07-31 12:36:26]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2015-07-31 12:36:27]

The patchset sent to the CQ was uploaded after l-g-t-m from holmer@google.com
Link to the patchset: https://codereview.webrtc.org/1211353002/#ps1030001
(title: "set ss data if not set")

[commit-bot: I haz the power, 2015-07-31 12:36:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1211353002/1030001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1211353002/1030001

[commit-bot: I haz the power, 2015-07-31 13:10:13]

Committed patchset #29 (id:1030001)

[commit-bot: I haz the power, 2015-07-31 13:10:28]

Patchset 29 (id:??) landed as
https://crrev.com/a9455ab235e1169572f9eae3175cd9310d6729e2
Cr-Commit-Position: refs/heads/master@{#9665}