Enabling spatial layers in VP9Impl. Filter layers in the loopback test.

Enabling spatial layers in VP9Impl. Filter layers in the loopback test.

Handling the case when encoder drops only the higher layer.
Added options to screenshare loopback test to discard high temporal or spatial layers (to view the lower layers).

Committed: https://crrev.com/7f6a6fc0b23795cd4f0aacbf707618c1f3d0a878
Cr-Commit-Position: refs/heads/master@{#9883}

[ivica, 2015-08-11 15:49:36]

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

[ivica, 2015-08-11 15:49:37]

Enabling multiple spatial layers in VP9Impl. (at least for testing purposes)

Things I wasn't sure about:
1) VP9 codec doesn't yet support more than 2 spatial layers. Should I check the
number of SLs in VP9Impl?
2) Pending frame part in VP9Impl might be confusing, especially handling
frag_info_ and making sure there is no memory leak. If you have any suggestions
on how to simplify it, let me know.

[ivica, 2015-08-12 07:22:45]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-12 07:22:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/20001

[commit-bot: I haz the power, 2015-08-12 07:28:39]

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

[commit-bot: I haz the power, 2015-08-12 07:28:40]

Dry run: Try jobs failed on following builders:
  linux_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/8792)

[ivica, 2015-08-12 08:16:34]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-12 08:16:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/40001

[commit-bot: I haz the power, 2015-08-12 08:22:13]

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

[commit-bot: I haz the power, 2015-08-12 08:22:14]

Dry run: Try jobs failed on following builders:
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/8753)

[ivica, 2015-08-12 08:46:24]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-12 08:46:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/60001

[commit-bot: I haz the power, 2015-08-12 10:17:09]

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

[commit-bot: I haz the power, 2015-08-12 10:17:10]

Dry run: This issue passed the CQ dry run.

[ivica, 2015-08-13 09:17:55]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 09:18:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/80001

[commit-bot: I haz the power, 2015-08-13 09:27:51]

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

[commit-bot: I haz the power, 2015-08-13 09:27:52]

Dry run: Try jobs failed on following builders:
  linux on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux/builds/8974)

[ivica, 2015-08-13 11:29:00]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 11:29:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/100001

[commit-bot: I haz the power, 2015-08-13 12:12:23]

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

[commit-bot: I haz the power, 2015-08-13 12:12:24]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2015-08-13 13:52:06]

sprang@webrtc.org changed reviewers:
+ sprang@webrtc.org

[sprang_webrtc, 2015-08-13 13:52:07]

Looks good. A few comments.

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

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:581: // If SVC used,
num_spatial_layers must be populated for each packet.
This comment is a bit confusing. Do you mean that num_spatial_layers may change
from packet to packet? Or from frame to frame?

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

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:215: if
(inst->codecSpecific.VP9.numberOfSpatialLayers > 2) {
Add comment that libvpx currently only supports two layers.

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/test/layer_filter...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:40: if ((tl_discard_threshold_ > 0 ||
sl_discard_threshold_ > 0) &&
Can't we just call test::DirectTransport::SendRtp(new_packet, length) and return
early if this is the case?

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:78: uint8_t* new_packet = new
uint8_t[length];
Stack allocate a temporary instead.
uint8_t temp_buffer[IP_PACKET_SIZE];

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:83: }
ByteWriter<uint16_t>::WriteBigEndian(&new_packet[2], current_seq_num_);
Also add a comment why you're updating the headers here.

[ivica, 2015-08-13 15:20:30]

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

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:581: // If SVC used,
num_spatial_layers must be populated for each packet.
On 2015/08/13 13:52:07, språng wrote:
> This comment is a bit confusing. Do you mean that num_spatial_layers may
change
> from packet to packet? Or from frame to frame?

Yeah, it really is confusing. I removed it, it's clear from the code itself what
is going on.

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

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:215: if
(inst->codecSpecific.VP9.numberOfSpatialLayers > 2) {
On 2015/08/13 13:52:07, språng wrote:
> Add comment that libvpx currently only supports two layers.

Done.

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/test/layer_filter...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:40: if ((tl_discard_threshold_ > 0 ||
sl_discard_threshold_ > 0) &&
On 2015/08/13 13:52:07, språng wrote:
> Can't we just call test::DirectTransport::SendRtp(new_packet, length) and
return
> early if this is the case?

I've changed the code to immediately call SendRtp if both tl_discard_threshold
and sl_discard_threshold are zero. Otherwise, we must fix the sequence number,
so we have to process the packet.

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:78: uint8_t* new_packet = new
uint8_t[length];
On 2015/08/13 13:52:07, språng wrote:
> Stack allocate a temporary instead.
> uint8_t temp_buffer[IP_PACKET_SIZE];

Done.

https://codereview.webrtc.org/1287643002/diff/100001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:83: }
On 2015/08/13 13:52:07, språng wrote:
> ByteWriter<uint16_t>::WriteBigEndian(&new_packet[2], current_seq_num_);
> Also add a comment why you're updating the headers here.

Done.

[sprang_webrtc, 2015-08-13 16:28:31]

lgtm
mflodman, ptal

[mflodman, 2015-08-25 09:38:29]

On 2015/08/13 16:28:31, språng wrote:
> lgtm
> mflodman, ptal

Any specific files I should look at or the whole CL?

[ivica, 2015-08-25 09:46:15]

On 2015/08/25 09:38:29, mflodman wrote:
> On 2015/08/13 16:28:31, språng wrote:
> > lgtm
> > mflodman, ptal
> 
> Any specific files I should look at or the whole CL?

Most of the changes are few-liners. The only large change is the new file
layer_filtering_transport.cc (and .h), where I discard packets I don't want.
That would be a good thing to review.

Some tricky changes:
- I use different payload types for VP8 and VP9 to distinguish them in
LayerFilteringTransport.
- Updated gyp file to include layer_filter_transport.cc
- I always populate num_spatial_layers element in RTPVideoHeaderVP9 and
CodecSpecificInfoVP9, although that was only supposed to be used if
ss_data_available flag is true. I need that for packetizer to correctly set the
marker bit.

[ivica, 2015-08-25 12:43:57]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-25 12:44:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/160001

[commit-bot: I haz the power, 2015-08-25 14:44:13]

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

[commit-bot: I haz the power, 2015-08-25 14:44:14]

Dry run: Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[mflodman, 2015-08-27 07:27:49]

LG, just a few minor comments.

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

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:356: if (num_spatial_layers_
> 1) {
I don't know the background for when to set min qp to 0 or 2, but should there
be an else setting 'rc_min_qualtizer' to 2 if 'num_spatial_layers_ == 1'?

I guess we can toggle the number of layers during a call, at least in theory.

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:358:
config_->rc_max_quantizer = 52;
AFAICT this isn't needed, the max quantizer is already set to 52 and never
changed. Right?

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/test/layer_filter...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:71: 
nit: remove empty line to follow style above?

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:83: uint8_t
temp_buffer[IP_PACKET_SIZE];
Should include 'webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h' to
explicitly get this definition.

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/video/loopback.h
File webrtc/video/loopback.h (right):

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/video/loopback.h#...
webrtc/video/loopback.h:37: size_t tl_discard_threshold;
Can you add a comment for this or maybe rename to tell it is what layers to
discard?

[ivica, 2015-08-27 08:49:25]

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

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:356: if (num_spatial_layers_
> 1) {
On 2015/08/27 07:27:48, mflodman wrote:
> I don't know the background for when to set min qp to 0 or 2, but should there
> be an else setting 'rc_min_qualtizer' to 2 if 'num_spatial_layers_ == 1'?
> 
> I guess we can toggle the number of layers during a call, at least in theory.

I don't know why was it set to 0 here. I removed this 'if' completely for now.

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:358:
config_->rc_max_quantizer = 52;
On 2015/08/27 07:27:48, mflodman wrote:
> AFAICT this isn't needed, the max quantizer is already set to 52 and never
> changed. Right?

I changed from 63 to 52, because 63 looks really bad. I removed this anyway now.

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/test/layer_filter...
File webrtc/test/layer_filtering_transport.cc (right):

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:71: 
On 2015/08/27 07:27:48, mflodman wrote:
> nit: remove empty line to follow style above?

Done.

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/test/layer_filter...
webrtc/test/layer_filtering_transport.cc:83: uint8_t
temp_buffer[IP_PACKET_SIZE];
On 2015/08/27 07:27:48, mflodman wrote:
> Should include 'webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h' to
> explicitly get this definition.

Done.

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/video/loopback.h
File webrtc/video/loopback.h (right):

https://codereview.webrtc.org/1287643002/diff/160001/webrtc/video/loopback.h#...
webrtc/video/loopback.h:37: size_t tl_discard_threshold;
On 2015/08/27 07:27:48, mflodman wrote:
> Can you add a comment for this or maybe rename to tell it is what layers to
> discard?

Added a comment. If we think of any better name, I can easily change it.

[ivica, 2015-08-27 14:17:28]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 14:17:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/200001

[commit-bot: I haz the power, 2015-08-27 16:17:46]

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

[commit-bot: I haz the power, 2015-08-27 16:17:47]

Dry run: Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[philipel, 2015-09-01 12:27:02]

philipel@webrtc.org changed reviewers:
+ philipel@webrtc.org

[philipel, 2015-09-01 12:27:03]

https://codereview.webrtc.org/1287643002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/codec_database.cc (right):

https://codereview.webrtc.org/1287643002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/codec_database.cc:60:
vp9_settings.numberOfSpatialLayers = 1;
Already defined on line 64 in old, 65 in new.

[ivica, 2015-09-03 08:55:37]

https://codereview.webrtc.org/1287643002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/codec_database.cc (right):

https://codereview.webrtc.org/1287643002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/codec_database.cc:60:
vp9_settings.numberOfSpatialLayers = 1;
On 2015/09/01 12:27:03, philipel wrote:
> Already defined on line 64 in old, 65 in new.

Fixed, thanks.

[ivica, 2015-09-07 09:12:53]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 09:13:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/220001

[commit-bot: I haz the power, 2015-09-07 09:13:43]

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

[commit-bot: I haz the power, 2015-09-07 09:13:44]

Dry run: Try jobs failed on following builders:
  android_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/5474)
  ios on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios/builds/9344)
  ios_arm64 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64/builds/4213)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/4146)
  linux_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/9165)
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/6578)
  mac_x64 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64/builds/3129)
  mac_x64_gn on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn/builds/3878)
  win on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win/builds/9516)
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/9387)
  win_x64_gn on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn/builds/3828)
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/9198)

[ivica, 2015-09-07 09:24:45]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 09:24:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/240001

[commit-bot: I haz the power, 2015-09-07 09:27:43]

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

[commit-bot: I haz the power, 2015-09-07 09:27:44]

Dry run: Try jobs failed on following builders:
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/6579)

[ivica, 2015-09-07 09:33:30]

The CQ bit was checked by ivica@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-07 09:33:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287643002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287643002/260001

[commit-bot: I haz the power, 2015-09-07 10:20:53]

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

[commit-bot: I haz the power, 2015-09-07 10:20:54]

Dry run: This issue passed the CQ dry run.

[mflodman, 2015-09-08 06:01:36]

LGTM

+Stefan as a video_coding/ owner.

[stefan-webrtc, 2015-09-08 07:24:54]

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

[stefan-webrtc, 2015-09-08 07:24:55]

lgtm

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

https://codereview.webrtc.org/1287643002/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:547: // Always populate this,
so that packetizer can properly set the marker bit.
so that the packetizer

[ivica, 2015-09-08 07:33:38]

The CQ bit was checked by ivica@webrtc.org

[ivica, 2015-09-08 07:33:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
stefan@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1287643002/#ps270014 (title:
"rebase master + fixing the comment")

[commit-bot: I haz the power, 2015-09-08 07:33:45]

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

[ivica, 2015-09-08 07:34:34]

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

https://codereview.webrtc.org/1287643002/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:547: // Always populate this,
so that packetizer can properly set the marker bit.
On 2015/09/08 07:24:55, stefan-webrtc (holmer) wrote:
> so that the packetizer

Done.

[commit-bot: I haz the power, 2015-09-08 09:40:36]

Committed patchset #15 (id:270014)

[commit-bot: I haz the power, 2015-09-08 09:40:46]

Patchset 15 (id:??) landed as
https://crrev.com/7f6a6fc0b23795cd4f0aacbf707618c1f3d0a878
Cr-Commit-Position: refs/heads/master@{#9883}