Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1328113004:
Work on flexible mode and screen sharing. (Closed)
|
Created:
5 years, 3 months ago by philipel Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, mflodman, perkj_webrtc, andresp Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWork on flexible mode and screen sharing.
Implement VP8 style screensharing but with spatial layers.
Implement flexible mode.
Files from other patches:
generic_encoder.cc
layer_filtering_transport.cc
BUG=webrtc:4914
Committed: https://crrev.com/77ccfb4d16c148e61a316746bb5d9705e8b39f4a
Cr-Commit-Position: refs/heads/master@{#10572}
Patch Set 1 #Patch Set 2 : Undo renaming. #Patch Set 3 : Undo renaming #
Total comments: 47
Patch Set 4 : Spatial layers when screensharing. #
Total comments: 54
Patch Set 5 : Fixing comments. #Patch Set 6 : Bug fix screenshare_layers. #Patch Set 7 : Implement general screenshare layer logic and unittests. #
Total comments: 16
Patch Set 8 : Fix comments. #Patch Set 9 : Changed interface for spatial layer encoding. #
Total comments: 30
Patch Set 10 : Feedback fixes. #Patch Set 11 : Unit test fixes. #Patch Set 12 : Rebase #Patch Set 13 : Rebase #Patch Set 14 : Rebase trickery #Patch Set 15 : Use alt_fb_idx for keyframes, fully initialize structure. #
Total comments: 21
Patch Set 16 : Rebase + comments. #Patch Set 17 : static cast #Patch Set 18 : Rebase. #Patch Set 19 : Rebase magic #Patch Set 20 : Conflict confussion #Patch Set 21 : Newline before EOF in video_send_stream_tests.cc #Patch Set 22 : Rebase + Comments #
Total comments: 4
Patch Set 23 : Bracers initialization fix #Patch Set 24 : Initialization compile fix android #
Total comments: 4
Patch Set 25 : Comments #
Total comments: 8
Patch Set 26 : Rebase + comments #
Total comments: 12
Patch Set 27 : Comment fix #Patch Set 28 : Unittest fix #Messages
Total messages: 78 (24 generated)
philipel@webrtc.org changed reviewers: + sprang@webrtc.org
Sorry for large CL :(
https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc (right): https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:1: /* Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. 2015? :) https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:22: return true; Why do we return a bool here? https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:26: sl0_bits_used_ += current_sl_ == 0 ? size_bytes * 8 : 0; Would prefer if (current_sl_ == 0) sl0_bits_used_ += size_bytes * 8; https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:29: int8_t ScreenshareLayersVP9::CurrentLayer() { const? https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:33: // TODO(philipel): suppose to be used for different spatial layers, Can we refer to a libvpx issue or some other reference point? https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:37: std::array<int8_t, 4> res; Please comment how this res (result?) is to be interpreted and/or rename it. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:45: // (diff becomes to big). or too big? https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:49: This means you are looking at the average bitrate over the entire lifetime of this object? The bitrate may be changed at runtime due to bandwidth estimates... Could you instead reuse this part form the vp8 screenshare layers class? Minus the qp boost and mode, which probably don't make much sense here... https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h (right): https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:23: std::array<int8_t, 4> BufferArguments(uint32_t timestamp); c++11 std-libs not yet allowed :( https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:29: uint32_t last_ts_; Please briefly comment what these do and/or don't abbreviate as much. kbps is a commonly accepted one, but we generally try to be more descriptive. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:304: assert(inst->codecSpecific.VP9.numberOfTemporalLayers == 2); CHECK_EQ(2, ...) https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:313: temporal_layer_.ConfigureBitrate(1000); Why not set to start bitrate from VideoCodec paramter and update in SetRates()? https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:662: } Remove {} https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:689: DCHECK(upd_buffer < 3); DCHECK_LT https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:698: if (upd_buffer == -2) { Have a named constant for -2? https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:699: #ifdef NDEBUG Can't just always do this? memset it? https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:705: // Keyframe, always stored in BUF0. Why? Because of line 161 in This comment may grow stale quickly :) "Keyframe always stored in BUF0 by libvpx." is probably sufficient. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:710: int8_t refs[3] = {ref_buf1, ref_buf2, ref_buf3}; I'd probably add a named constant for 3 and use here and below. const size_t kNumReferences = 3; or so... https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:714: goto done; Ugh! How bout: i = kNumReferences; continue; Perhaps a comment of when this would actually happen as well. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:728: DCHECK(false); RTC_NOTREACHED(); https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:739: // Everybody love gotos! No. :) https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:759: DCHECK(false); RTC_NOTREACHED(); https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/generic_encoder.cc (right): https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/generic_encoder.cc:40: RTPVideoHeaderVP9& vp9toHdr = rtp->codecHeader.VP9; We usually use references only const objects. The "to" in the name doesn't really help readability imo. I would probably use something like: RTPVideoHeaderVP9* rtp_header = &rtp->codecHeader.VP9; const CodecSpecificInfoVP9& codec_specific = info->codecSpecific.VP9; https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/generic_encoder.cc:72: } Remove {}
Much of the code has changed, but many of the comments are still relevant and has been considered when writing the new code. Wont upload new CL until https://chromium-review.googlesource.com/#/c/296994/ is commited and the new codec control (VP9E_SET_FIRST_SPATIAL_LAYER, there is no CL for it yet) is commited. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc (right): https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:22: return true; On 2015/09/07 14:39:04, språng wrote: > Why do we return a bool here? Because I copied the interface from the temporal layers in VP8, now changed to void. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:26: sl0_bits_used_ += current_sl_ == 0 ? size_bytes * 8 : 0; On 2015/09/07 14:39:04, språng wrote: > Would prefer > if (current_sl_ == 0) > sl0_bits_used_ += size_bytes * 8; Done. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:29: int8_t ScreenshareLayersVP9::CurrentLayer() { On 2015/09/07 14:39:04, språng wrote: > const? Done. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:33: // TODO(philipel): suppose to be used for different spatial layers, On 2015/09/07 14:39:04, språng wrote: > Can we refer to a libvpx issue or some other reference point? Comment removed. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:37: std::array<int8_t, 4> res; On 2015/09/07 14:39:04, språng wrote: > Please comment how this res (result?) is to be interpreted and/or rename it. Changed to new data structure https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:45: // (diff becomes to big). On 2015/09/07 14:39:04, språng wrote: > or too big? Removed check and comment. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:49: On 2015/09/07 14:39:04, språng wrote: > This means you are looking at the average bitrate over the entire lifetime of > this object? > The bitrate may be changed at runtime due to bandwidth estimates... > > Could you instead reuse this part form the vp8 screenshare layers class? Minus > the qp boost and mode, which probably don't make much sense here... No, this is not the bitrate over the entire lifetime of the object, if the threshold_kbps_ is changed than so is the rate at which sl0_bits_used_ is decreased. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h (right): https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:23: std::array<int8_t, 4> BufferArguments(uint32_t timestamp); On 2015/09/07 14:39:04, språng wrote: > c++11 std-libs not yet allowed :( Changed to a struct instead. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:29: uint32_t last_ts_; On 2015/09/07 14:39:04, språng wrote: > Please briefly comment what these do and/or don't abbreviate as much. > kbps is a commonly accepted one, but we generally try to be more descriptive. Done. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:304: assert(inst->codecSpecific.VP9.numberOfTemporalLayers == 2); On 2015/09/07 14:39:05, språng wrote: > CHECK_EQ(2, ...) Code removed, now use spatial layers instead. Hurra! https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:313: temporal_layer_.ConfigureBitrate(1000); On 2015/09/07 14:39:05, språng wrote: > Why not set to start bitrate from VideoCodec paramter and update in SetRates()? Done. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:662: } On 2015/09/07 14:39:05, språng wrote: > Remove {} Done. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:689: DCHECK(upd_buffer < 3); On 2015/09/07 14:39:05, språng wrote: > DCHECK_LT Code removed. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:698: if (upd_buffer == -2) { On 2015/09/07 14:39:05, språng wrote: > Have a named constant for -2? Code removed. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:699: #ifdef NDEBUG On 2015/09/07 14:39:05, språng wrote: > Can't just always do this? memset it? Done. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:705: // Keyframe, always stored in BUF0. Why? Because of line 161 in On 2015/09/07 14:39:05, språng wrote: > This comment may grow stale quickly :) > "Keyframe always stored in BUF0 by libvpx." is probably sufficient. Changed so that the keyframes are stored in the buffer specified by the parameters to the function. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:710: int8_t refs[3] = {ref_buf1, ref_buf2, ref_buf3}; On 2015/09/07 14:39:05, språng wrote: > I'd probably add a named constant for 3 and use here and below. > const size_t kNumReferences = 3; > or so... Now use the kMaxVp9RefPics instead of '3'. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:714: goto done; On 2015/09/07 14:39:05, språng wrote: > Ugh! How bout: > > i = kNumReferences; > continue; > > Perhaps a comment of when this would actually happen as well. Code removed. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:728: DCHECK(false); On 2015/09/07 14:39:05, språng wrote: > RTC_NOTREACHED(); Code removed. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:739: // Everybody love gotos! On 2015/09/07 14:39:05, språng wrote: > No. :) Yes! https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:759: DCHECK(false); On 2015/09/07 14:39:05, språng wrote: > RTC_NOTREACHED(); Code removed. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/generic_encoder.cc (right): https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/generic_encoder.cc:40: RTPVideoHeaderVP9& vp9toHdr = rtp->codecHeader.VP9; On 2015/09/07 14:39:05, språng wrote: > We usually use references only const objects. > The "to" in the name doesn't really help readability imo. I would probably use > something like: > RTPVideoHeaderVP9* rtp_header = &rtp->codecHeader.VP9; > const CodecSpecificInfoVP9& codec_specific = info->codecSpecific.VP9; Acknowledged. https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/generic_encoder.cc:72: } On 2015/09/07 14:39:05, språng wrote: > Remove {} Done.
On 2015/09/10 14:47:02, philipel wrote: > Much of the code has changed, but many of the comments are still relevant and > has been considered when writing the new code. > > Wont upload new CL until > https://chromium-review.googlesource.com/#/c/296994/ > is commited and the new codec control (VP9E_SET_FIRST_SPATIAL_LAYER, there is no > CL for it yet) is commited. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc (right): > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:22: return true; > On 2015/09/07 14:39:04, språng wrote: > > Why do we return a bool here? > > Because I copied the interface from the temporal layers in VP8, now changed to > void. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:26: sl0_bits_used_ > += current_sl_ == 0 ? size_bytes * 8 : 0; > On 2015/09/07 14:39:04, språng wrote: > > Would prefer > > if (current_sl_ == 0) > > sl0_bits_used_ += size_bytes * 8; > > Done. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:29: int8_t > ScreenshareLayersVP9::CurrentLayer() { > On 2015/09/07 14:39:04, språng wrote: > > const? > > Done. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:33: // > TODO(philipel): suppose to be used for different spatial layers, > On 2015/09/07 14:39:04, språng wrote: > > Can we refer to a libvpx issue or some other reference point? > > Comment removed. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:37: > std::array<int8_t, 4> res; > On 2015/09/07 14:39:04, språng wrote: > > Please comment how this res (result?) is to be interpreted and/or rename it. > > Changed to new data structure > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:45: // (diff > becomes to big). > On 2015/09/07 14:39:04, språng wrote: > > or too big? > > Removed check and comment. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:49: > On 2015/09/07 14:39:04, språng wrote: > > This means you are looking at the average bitrate over the entire lifetime of > > this object? > > The bitrate may be changed at runtime due to bandwidth estimates... > > > > Could you instead reuse this part form the vp8 screenshare layers class? Minus > > the qp boost and mode, which probably don't make much sense here... > > No, this is not the bitrate over the entire lifetime of the object, if the > threshold_kbps_ is changed than so is the rate at which sl0_bits_used_ is > decreased. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h (right): > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:23: > std::array<int8_t, 4> BufferArguments(uint32_t timestamp); > On 2015/09/07 14:39:04, språng wrote: > > c++11 std-libs not yet allowed :( > > Changed to a struct instead. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:29: uint32_t > last_ts_; > On 2015/09/07 14:39:04, språng wrote: > > Please briefly comment what these do and/or don't abbreviate as much. > > kbps is a commonly accepted one, but we generally try to be more descriptive. > > Done. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:304: > assert(inst->codecSpecific.VP9.numberOfTemporalLayers == 2); > On 2015/09/07 14:39:05, språng wrote: > > CHECK_EQ(2, ...) > > Code removed, now use spatial layers instead. Hurra! > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:313: > temporal_layer_.ConfigureBitrate(1000); > On 2015/09/07 14:39:05, språng wrote: > > Why not set to start bitrate from VideoCodec paramter and update in > SetRates()? > > Done. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:662: } > On 2015/09/07 14:39:05, språng wrote: > > Remove {} > > Done. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:689: DCHECK(upd_buffer < 3); > On 2015/09/07 14:39:05, språng wrote: > > DCHECK_LT > > Code removed. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:698: if (upd_buffer == -2) { > On 2015/09/07 14:39:05, språng wrote: > > Have a named constant for -2? > > Code removed. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:699: #ifdef NDEBUG > On 2015/09/07 14:39:05, språng wrote: > > Can't just always do this? memset it? > > Done. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:705: // Keyframe, always > stored in BUF0. Why? Because of line 161 in > On 2015/09/07 14:39:05, språng wrote: > > This comment may grow stale quickly :) > > "Keyframe always stored in BUF0 by libvpx." is probably sufficient. > > Changed so that the keyframes are stored in the buffer specified by the > parameters to the function. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:710: int8_t refs[3] = > {ref_buf1, ref_buf2, ref_buf3}; > On 2015/09/07 14:39:05, språng wrote: > > I'd probably add a named constant for 3 and use here and below. > > const size_t kNumReferences = 3; > > or so... > > Now use the kMaxVp9RefPics instead of '3'. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:714: goto done; > On 2015/09/07 14:39:05, språng wrote: > > Ugh! How bout: > > > > i = kNumReferences; > > continue; > > > > Perhaps a comment of when this would actually happen as well. > > Code removed. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:728: DCHECK(false); > On 2015/09/07 14:39:05, språng wrote: > > RTC_NOTREACHED(); > > Code removed. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:739: // Everybody love gotos! > On 2015/09/07 14:39:05, språng wrote: > > No. :) > > Yes! > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:759: DCHECK(false); > On 2015/09/07 14:39:05, språng wrote: > > RTC_NOTREACHED(); > > Code removed. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/main/source/generic_encoder.cc (right): > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/main/source/generic_encoder.cc:40: > RTPVideoHeaderVP9& vp9toHdr = rtp->codecHeader.VP9; > On 2015/09/07 14:39:05, språng wrote: > > We usually use references only const objects. > > The "to" in the name doesn't really help readability imo. I would probably use > > something like: > > RTPVideoHeaderVP9* rtp_header = &rtp->codecHeader.VP9; > > const CodecSpecificInfoVP9& codec_specific = info->codecSpecific.VP9; > > Acknowledged. > > https://codereview.webrtc.org/1328113004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/main/source/generic_encoder.cc:72: } > On 2015/09/07 14:39:05, språng wrote: > > Remove {} > > Done. Um, did you upload a new patch set as well?
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328113004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328113004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/8086) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/5498)
Good job getting this to work. I know things are messy and/or missing... Some observations: In general, try to have more descriptive names/fewer abbreviations. I'd also like more test cases. Especially add a screenshare_layers_unittest for the new parts. Wherever possible also for any other code that isn't absolutely non-trivial. Are the things in this diff coming from other CLs? If so please say so in the description. filtering_transport for instance? https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/interface/video_codec_interface.h (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/interface/video_codec_interface.h:72: // Frame reference data . https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:1: /* Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. 2015 https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:34: } remove {} https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:53: if (l0_bits_used_ > threshold_kbps_ * 1000 && !is_keyframe) { is_keyframe should always be false here? https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:58: } As noted in a previous comment, this measures average bitrate since the first frame - but the bitrate can dynamically change... I think you can bring over some parts of screenshare_layers for vp8 related to this (minus the qp boost things etc) https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:1: /* Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. 2015 https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:21: void ConfigureBitrate(int threshold_kbps); Rename and/or comment to make it clear that this is the threshold above which packets will be pushed to the higher layer. This also assumes we have just two layers. Since we probably want to use more than two spatial layers when libvpx supports it, perhaps this interface should be made flexible enough now? https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:25: bool is_keyframe); Pls avoid abbreviations whenever possible! How 'bout GetSuperFrameSettings() ? https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:28: // Target kbps for layer 0 Always end comments with a period. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:36: // Timestamp of last frame . https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:376: if (!is_flexible_mode_) This seems very specific. This only works because of the assumption that it's flexible mode iff it's screenshare... I think we need to extend the api for the vp9 wrapper so that we can configure these things explicitly. Check if there is a bug for this, add a todo and reference. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:489: CHECK(false); RTC_NOTREACHED(); https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:661: for (int l = settings.start_layer; l <= settings.stop_layer; ++l) { More descriptive names please! layer instead of l? same below. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:672: for (unsigned int r = 0; r < kMaxVp9RefPics; ++r) { name https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:681: layer_flags ^= VP8_EFLAG_NO_REF_LAST; Not sure I follow why you xor this flag. Comment? https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:721: goto done; Please no goto's! Use a descriptively named temp bool flag instead to determine what to do after the break. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:60: uint8_t stop_layer = 0; // Not used anywhere right now Either don't include it or comment on what it should do when it works. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:122: // Used for flexible mode . https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:124: int64_t buf_upd_at_frame_[8]; Use a named constant instead of 8. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.cc:28: memset(frame_decoded_, 0, sizeof(frame_decoded_)); Don't think you need this? https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.cc:253: continuous &= frame_decoded_[frame_ref % kFrameDecodedLength]; Just return false early here, and true below. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/decoding_state.h (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.h:26: static const uint16_t kNoRefBits = 14; kNumRefBits or kMaxRefBits. Otherwise it sounds like a special value used to indicates we have no reference bits. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.h:76: uint16_t fd_cleared_to_; which file descriptor is that? https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/decoding_state_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state_unittest.cc:582: Any chance we can break this out into several smaller tests? https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/generic_encoder.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/generic_encoder.cc:40: RTPVideoHeaderVP9& vp9toHdr = rtp->codecHeader.VP9; Don't use references for mutable objects. Suggest RTPVideoHeaderVP9* video_header; const CodecSpecificInfoVP9& codec_specific; https://codereview.webrtc.org/1328113004/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1910: EXPECT_TRUE(vp9videoHeader->picture_id != kNoPictureId); EXPECT_NE(kNoPictureId, vp9videoHeader->picture_id);
https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/interface/video_codec_interface.h (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/interface/video_codec_interface.h:72: // Frame reference data On 2015/09/15 15:41:21, språng wrote: > . Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:1: /* Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. On 2015/09/15 15:41:21, språng wrote: > 2015 Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:34: } On 2015/09/15 15:41:21, språng wrote: > remove {} Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:53: if (l0_bits_used_ > threshold_kbps_ * 1000 && !is_keyframe) { On 2015/09/15 15:41:21, språng wrote: > is_keyframe should always be false here? If it is a keyframe then we must encode both layers. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:58: } On 2015/09/15 15:41:21, språng wrote: > As noted in a previous comment, this measures average bitrate since the first > frame - but the bitrate can dynamically change... > I think you can bring over some parts of screenshare_layers for vp8 related to > this (minus the qp boost things etc) It is NOT true that it measures the average bitrate since the first frame. If the threshold_kbps_ change than so does the rate at which l0_bits_used_ is decreased. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:1: /* Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. On 2015/09/15 15:41:21, språng wrote: > 2015 Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:21: void ConfigureBitrate(int threshold_kbps); On 2015/09/15 15:41:22, språng wrote: > Rename and/or comment to make it clear that this is the threshold above which > packets will be pushed to the higher layer. > This also assumes we have just two layers. Since we probably want to use more > than two spatial layers when libvpx supports it, perhaps this interface should > be made flexible enough now? Acknowledged. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:25: bool is_keyframe); On 2015/09/15 15:41:22, språng wrote: > Pls avoid abbreviations whenever possible! How 'bout GetSuperFrameSettings() ? Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:28: // Target kbps for layer 0 On 2015/09/15 15:41:21, språng wrote: > Always end comments with a period. Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:36: // Timestamp of last frame On 2015/09/15 15:41:22, språng wrote: > . Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:376: if (!is_flexible_mode_) On 2015/09/15 15:41:22, språng wrote: > This seems very specific. This only works because of the assumption that it's > flexible mode iff it's screenshare... > > I think we need to extend the api for the vp9 wrapper so that we can configure > these things explicitly. Check if there is a bug for this, add a todo and > reference. Yes, this assumption is completely wrong. It should check if we are using screensharing or not, which I have now done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:489: CHECK(false); On 2015/09/15 15:41:22, språng wrote: > RTC_NOTREACHED(); Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:661: for (int l = settings.start_layer; l <= settings.stop_layer; ++l) { On 2015/09/15 15:41:22, språng wrote: > More descriptive names please! layer instead of l? same below. Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:672: for (unsigned int r = 0; r < kMaxVp9RefPics; ++r) { On 2015/09/15 15:41:22, språng wrote: > name Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:681: layer_flags ^= VP8_EFLAG_NO_REF_LAST; On 2015/09/15 15:41:22, språng wrote: > Not sure I follow why you xor this flag. Comment? If we want to reference the LAST buffer then we should use the flags VP8_EFLAG_NO_REF_GF and VP8_EFLAG_NO_REF_ARF. So what I do is to start with all the flags set (line 667) and then simply xor out the flag for the buffer that should be referenced. Lets say we want to reference two buffers, only looking at the reference flags the following will happen: Before first iteration: layer_flags = NO_REF_LAST ^ NO_REF_GF ^ NO_REF_ARF After first iteration: layer_flags = (NO_REF_LAST ^ NO_REF_LAST) ^ NO_REF_GF ^ NO_REF_ARF = NO_REF_GF ^ NO_REF_ARF Before second iteration: layer_flags = NO_REF_GF ^ NO_REF_ARF After second iteration: layer_flags = (NO_REF_GF ^ NO_REF_GF) ^ NO_REF_ARF = NO_REF_ARF https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:721: goto done; On 2015/09/15 15:41:22, språng wrote: > Please no goto's! > > Use a descriptively named temp bool flag instead to determine what to do after > the break. Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:60: uint8_t stop_layer = 0; // Not used anywhere right now On 2015/09/15 15:41:22, språng wrote: > Either don't include it or comment on what it should do when it works. Hmmm... tricky situation where the field is used by the GenerateRefsAndFlags function but not by the encoder. It can easily confuse someone to think that only the frames [start_layer, stop_layer] will be encoded. Removed the comment for now. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:122: // Used for flexible mode On 2015/09/15 15:41:22, språng wrote: > . Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:124: int64_t buf_upd_at_frame_[8]; On 2015/09/15 15:41:22, språng wrote: > Use a named constant instead of 8. Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.cc:28: memset(frame_decoded_, 0, sizeof(frame_decoded_)); On 2015/09/15 15:41:22, språng wrote: > Don't think you need this? I think I do: http://stackoverflow.com/questions/7760291/default-initialization-of-c-member... https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.cc:253: continuous &= frame_decoded_[frame_ref % kFrameDecodedLength]; On 2015/09/15 15:41:22, språng wrote: > Just return false early here, and true below. Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/decoding_state.h (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.h:26: static const uint16_t kNoRefBits = 14; On 2015/09/15 15:41:22, språng wrote: > kNumRefBits or kMaxRefBits. Otherwise it sounds like a special value used to > indicates we have no reference bits. Done. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.h:76: uint16_t fd_cleared_to_; On 2015/09/15 15:41:22, språng wrote: > which file descriptor is that? Renamed to frame_decoded_cleared_to_. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/generic_encoder.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/generic_encoder.cc:40: RTPVideoHeaderVP9& vp9toHdr = rtp->codecHeader.VP9; On 2015/09/15 15:41:22, språng wrote: > Don't use references for mutable objects. Suggest > RTPVideoHeaderVP9* video_header; > const CodecSpecificInfoVP9& codec_specific; This part of the code is in a funny state right now. Ivica has made the relevant changes without using a reference, and will be part of his CL later. When rebasing there will be a conflict at which point I will use his code. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1910: EXPECT_TRUE(vp9videoHeader->picture_id != kNoPictureId); On 2015/09/15 15:41:22, språng wrote: > EXPECT_NE(kNoPictureId, vp9videoHeader->picture_id); Done.
General fixes and improvements in regards to the comments. I have not included any major changes (more general screensharing, unit tests for screensharing) but I will work on that and upload it as the next CL. I will also add which files are "mine" and which comes from other patches.
https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:53: if (l0_bits_used_ > threshold_kbps_ * 1000 && !is_keyframe) { On 2015/09/16 09:35:53, philipel wrote: > On 2015/09/15 15:41:21, språng wrote: > > is_keyframe should always be false here? > > If it is a keyframe then we must encode both layers. I was more referring to the fact that if is_keyframe was true then would have entered the if on line 47, and not ended up here. But is notice this has changed now so nevermind :) https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:58: } On 2015/09/16 09:35:53, philipel wrote: > On 2015/09/15 15:41:21, språng wrote: > > As noted in a previous comment, this measures average bitrate since the first > > frame - but the bitrate can dynamically change... > > I think you can bring over some parts of screenshare_layers for vp8 related to > > this (minus the qp boost things etc) > > It is NOT true that it measures the average bitrate since the first frame. If > the threshold_kbps_ change than so does the rate at which l0_bits_used_ is > decreased. Ah, you're right. https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/decoding_state.cc:28: memset(frame_decoded_, 0, sizeof(frame_decoded_)); On 2015/09/16 09:35:54, philipel wrote: > On 2015/09/15 15:41:22, språng wrote: > > Don't think you need this? > > I think I do: > http://stackoverflow.com/questions/7760291/default-initialization-of-c-member... You're right. I was drunk. Maybe frame_decoded_{} would work though? https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc:2: * Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. 2015 https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc:42: layers_->LayerFrameEncoded(kilobits * 125, layer_id); Comment on why *125 https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:84: spatial_layer_(new ScreenshareLayersVP9(2)) { Comment on the 2 https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:307: } remove {} https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:376: if (!(codec_.mode == kScreensharing)) if (codec_.mode != kScreensharing) https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:652: static const vpx_enc_frame_flags_t all_flags = kAllFlags https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:682: layer_flags ^= VP8_EFLAG_NO_REF_LAST; Comment that you start with kAllFlags and then mask out using xor. Personally I'd maybe use layer_flags &= ~VP8_EFLAG_NO_REF_LAST; just to make it more clear what's inteded. But I'm fine with either. https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.h (right): https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.h:75: bool frame_decoded_[1 << kNumRefBits]; bool frame_decoded_[kFrameDecodedLength];
https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc:2: * Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. On 2015/09/25 09:53:15, språng wrote: > 2015 Done. https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc:42: layers_->LayerFrameEncoded(kilobits * 125, layer_id); On 2015/09/25 09:53:15, språng wrote: > Comment on why *125 Done. https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:84: spatial_layer_(new ScreenshareLayersVP9(2)) { On 2015/09/25 09:53:15, språng wrote: > Comment on the 2 Done. https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:307: } On 2015/09/25 09:53:15, språng wrote: > remove {} Done. https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:376: if (!(codec_.mode == kScreensharing)) On 2015/09/25 09:53:15, språng wrote: > if (codec_.mode != kScreensharing) Done. https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:652: static const vpx_enc_frame_flags_t all_flags = On 2015/09/25 09:53:15, språng wrote: > kAllFlags Done. https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:682: layer_flags ^= VP8_EFLAG_NO_REF_LAST; On 2015/09/25 09:53:15, språng wrote: > Comment that you start with kAllFlags and then mask out using xor. > Personally I'd maybe use > layer_flags &= ~VP8_EFLAG_NO_REF_LAST; > just to make it more clear what's inteded. But I'm fine with either. Done. https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.h (right): https://codereview.webrtc.org/1328113004/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.h:75: bool frame_decoded_[1 << kNumRefBits]; On 2015/09/25 09:53:15, språng wrote: > bool frame_decoded_[kFrameDecodedLength]; Done.
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328113004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328113004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/5983) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/363) mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/4489)
philipel@google.com changed reviewers: + philipel@google.com, stefan@webrtc.org
Review until vp9_impl.h https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:752: b_bit && (!l_bit || !vp9->inter_layer_predicted); Add a test for this https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:24: uint8_t ScreenshareLayersVP9::GetStartLayer() const { should this be called current_layer()? What is a start layer? https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:31: // and therefore it can't be set. Could you also explain why it can't have a max bitrate? https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:46: if (last_ts_ == 0) 0 isn't safe to use as "not initialized" https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:48: float time_diff = (timestamp - last_ts_) / 90.f; I suspect that this won't handle the case where timestamp < last_ts_, for instance when timestamp is 30 and last_ts_ 0xffffff. I think the subtraction will be promoted to int and done with integers after which the result will be converted to float for division. Should probably add a test to verify this. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:49: float total_bits_used = 0; I don't really think we need to count with fractions of bits, so int is probably good enough for this class. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:55: for (uint8_t layer_id = 0; layer_id < num_layers_ - 1; ++layer_id) { int https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:61: settings.layer[layer_id].ref_buf1 = layer_id; Comment on what this code block does. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:67: if (!is_keyframe) Comment on why !is_keyframe. Why not for key frames? https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:17: class ScreenshareLayersVP9 { Maybe we should add a comment above here to describe in short how this works? https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:48: uint32_t last_ts_; last_timestamp_ https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9.gyp (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9.gyp:32: 'screenshare_layers.h', Where is the unittest added? https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:60: uint8_t stop_layer = 0; What is a start and a stop layer? https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:84: */ C++ comments please https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:124: int64_t buf_upd_at_frame_[kNumVp9Buffers]; I'd prefer a longer variable name here.
I ran the video_engine_tests and many of the VP9 screenshare tests fail because they do not configure the correct number of spatial and temporal layers. This is since I added the sanity check here: https://codereview.webrtc.org/1328113004/diff/160001/webrtc/video/video_send_... Maybe I should just set the correct settings there instead of crashing. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:752: b_bit && (!l_bit || !vp9->inter_layer_predicted); On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > Add a test for this Hmm... if I add a function to test this we either have to reparse the payload (if we only pass the payload as a parameter) or we have to check if the l_bit is true before calling the function with the RTPVideoHeaderVP9 object since RTPVideoHeaderVP9 does not hold that information. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:24: uint8_t ScreenshareLayersVP9::GetStartLayer() const { On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > should this be called current_layer()? What is a start layer? The start layer is the first spatial layer to encode. I have added a comment in the .h file. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:31: // and therefore it can't be set. On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > Could you also explain why it can't have a max bitrate? I guess there is no reason other than that we would call codec_encode and it would encode nothing. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:46: if (last_ts_ == 0) On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > 0 isn't safe to use as "not initialized" Added a timestamp_initialized_ variable for initialization. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:48: float time_diff = (timestamp - last_ts_) / 90.f; On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > I suspect that this won't handle the case where timestamp < last_ts_, for > instance when timestamp is 30 and last_ts_ 0xffffff. I think the subtraction > will be promoted to int and done with integers after which the result will be > converted to float for division. Should probably add a test to verify this. This will use unsigned arithmetics so it's working as inteded. I have added a unittest for this case. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:49: float total_bits_used = 0; On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > I don't really think we need to count with fractions of bits, so int is probably > good enough for this class. I use floats not to count fractions of a bit, but to not loose fractions when the difference in timestamps is divided by 90 :). If we were using an int instead of float we could have a noticeable error in the calculation. For example if we have 60 fps then the average frame tick diff is 90000/60 = 1500, then we could have a rounding error of up to 89, which is 89/1500 = 0.0593, or around 6% off the real time diff. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:55: for (uint8_t layer_id = 0; layer_id < num_layers_ - 1; ++layer_id) { On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > int Done. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:61: settings.layer[layer_id].ref_buf1 = layer_id; On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > Comment on what this code block does. Done. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.cc:67: if (!is_keyframe) On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > Comment on why !is_keyframe. Why not for key frames? Done. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:17: class ScreenshareLayersVP9 { On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > Maybe we should add a comment above here to describe in short how this works? Done. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers.h:48: uint32_t last_ts_; On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > last_timestamp_ Done. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9.gyp (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9.gyp:32: 'screenshare_layers.h', On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > Where is the unittest added? In webrtc/modules/modules.gyp https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right): https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:60: uint8_t stop_layer = 0; On 2015/10/28 13:59:05, stefan-webrtc (holmer) wrote: > What is a start and a stop layer? The first/last spatial layer to be encoded. Added comments to the code. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:84: */ On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > C++ comments please Done. https://codereview.webrtc.org/1328113004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:124: int64_t buf_upd_at_frame_[kNumVp9Buffers]; On 2015/10/28 13:59:04, stefan-webrtc (holmer) wrote: > I'd prefer a longer variable name here. Done.
The CQ bit was checked by philipel@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328113004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328113004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by philipel@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328113004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328113004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
philipel@google.com changed reviewers: + mflodman@webrtc.org
mflodman@webrtc.org changed reviewers: + asapersson@webrtc.org
Illl be OOO the rest of the day and most of tomorrow. Erik and Åsa, Can you help Philip get this reviewed and submitted as soon as possible?
Looks good, just a few nits and questions. I'm relying on Åsa to look more closely on the jitterbuffer-part. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc:156: expected_.start_layer = l; Nit: Please don't use the variable name l. It's far too easy to misread as a 1 :) https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (left): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:389: } Why was this removed? https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:84: // Use two spatial when screensharing with flexible mode. spatial layers Perhaps add a TODO to enable more once supported by libvpx? https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:686: memset(&sf_conf, 0, sizeof(sf_conf)); Can we do vpx_svc_ref_frame_config sf_conf = {}; ? https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:690: for (int l = settings.start_layer; l <= settings.stop_layer; ++l) { s/l/layer
https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc:156: expected_.start_layer = l; On 2015/11/05 12:18:17, språng wrote: > Nit: Please don't use the variable name l. It's far too easy to misread as a 1 > :) Done. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (left): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:389: } On 2015/11/05 12:18:17, språng wrote: > Why was this removed? I got a conflict when rebasing and I included this new chunk of code. After that it wouldn't compile and the ExplicitlyConfiguredSpatialLayers() function was nowhere to be found, so I assumed it was something that had been added and then removed again. But now I can see that it is actually in the main repo so I guess it shouldn't be removed. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:84: // Use two spatial when screensharing with flexible mode. On 2015/11/05 12:18:17, språng wrote: > spatial layers > Perhaps add a TODO to enable more once supported by libvpx? I have talked to Marco and it doesn't seems likely to get the behavior we want. We can still use more than two layers, but I don't think the results will be good. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:686: memset(&sf_conf, 0, sizeof(sf_conf)); On 2015/11/05 12:18:17, språng wrote: > Can we do vpx_svc_ref_frame_config sf_conf = {}; ? Done. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:690: for (int l = settings.start_layer; l <= settings.stop_layer; ++l) { On 2015/11/05 12:18:17, språng wrote: > s/l/layer Done.
lgtm asapersson@, ptal
Rebase magic
Newline before EOF in video_send_stream_tests.cc
https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/interface/video_codec_interface.h (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/interface/video_codec_interface.h:74: uint16_t p_diff[kMaxVp9RefPics]; Max p_diff for flexible mode is now 7 bits, change to uint8_t? https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:69: if (frame->CodecSpecific()->codecType != kVideoCodecVP9) Think this is needed for non-flexible mode, right? https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:84: frame_decoded_cleared_to_ = frame_index; should frame_decoded_[frame_index] be set here? maybe add a unittest for this case https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state_unittest.cc:461: RTPVideoHeaderVP9& Vp9Hdr = packet.codecSpecificHeader.codecHeader.VP9; nit: vp9_hdr https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state_unittest.cc:547: // Frame after wrapping start frame, ref to 0, continuous Would this work if for example pid_diff was 10. Would be continuous but refer to old pid:10 frame?
https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/interface/video_codec_interface.h (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/interface/video_codec_interface.h:74: uint16_t p_diff[kMaxVp9RefPics]; On 2015/11/06 10:19:34, åsapersson wrote: > Max p_diff for flexible mode is now 7 bits, change to uint8_t? Done. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:69: if (frame->CodecSpecific()->codecType != kVideoCodecVP9) On 2015/11/06 10:19:34, åsapersson wrote: > Think this is needed for non-flexible mode, right? I discussed this with Stefan a while back, can't remember the exact reason but we concluded that we didn't need it in the case of VP9. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:84: frame_decoded_cleared_to_ = frame_index; On 2015/11/06 10:19:34, åsapersson wrote: > should frame_decoded_[frame_index] be set here? > > maybe add a unittest for this case You are right, I have also added a unittest for this. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state_unittest.cc:461: RTPVideoHeaderVP9& Vp9Hdr = packet.codecSpecificHeader.codecHeader.VP9; On 2015/11/06 10:19:34, åsapersson wrote: > nit: vp9_hdr Done. https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state_unittest.cc:547: // Frame after wrapping start frame, ref to 0, continuous On 2015/11/06 10:19:34, åsapersson wrote: > Would this work if for example pid_diff was 10. Would be continuous but refer to > old pid:10 frame? Yes, that would incorrectly report the frame as continuous. Fixed now.
Bracers initialization fix
Initialization compile fix android
https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/280001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:69: if (frame->CodecSpecific()->codecType != kVideoCodecVP9) On 2015/11/06 14:35:16, philipel1 wrote: > On 2015/11/06 10:19:34, åsapersson wrote: > > Think this is needed for non-flexible mode, right? > > I discussed this with Stefan a while back, can't remember the exact reason but > we concluded that we didn't need it in the case of VP9. Would be needed for non-flexible mode. https://codereview.webrtc.org/1328113004/diff/420001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/420001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:94: frame_decoded_[frame_index] = true; set once below https://codereview.webrtc.org/1328113004/diff/420001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:280: // since it's 32 bits instead of 7. nit: 32 bits - Picture id is 7 or 15 bits. https://codereview.webrtc.org/1328113004/diff/460001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/460001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:591: only use if screenshare? https://codereview.webrtc.org/1328113004/diff/460001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right): https://codereview.webrtc.org/1328113004/diff/460001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:126: uint16_t p_diff_[kMaxVp9NumberOfSpatialLayers][kMaxVp9RefPics]; uint8_t
Comments
https://codereview.webrtc.org/1328113004/diff/420001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/420001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:94: frame_decoded_[frame_index] = true; On 2015/11/06 17:08:03, åsapersson wrote: > set once below Done. https://codereview.webrtc.org/1328113004/diff/420001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:280: // since it's 32 bits instead of 7. On 2015/11/06 17:08:03, åsapersson wrote: > nit: 32 bits - Picture id is 7 or 15 bits. Done. https://codereview.webrtc.org/1328113004/diff/460001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/460001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:591: On 2015/11/06 17:08:03, åsapersson wrote: > only use if screenshare? Done. https://codereview.webrtc.org/1328113004/diff/460001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right): https://codereview.webrtc.org/1328113004/diff/460001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:126: uint16_t p_diff_[kMaxVp9NumberOfSpatialLayers][kMaxVp9RefPics]; On 2015/11/06 17:08:03, åsapersson wrote: > uint8_t Done.
https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:69: if (frame->CodecSpecific()->codecType != kVideoCodecVP9) This is needed for non-flexible mode. https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:279: // Could be made more robust using PictureId istead nit: instead https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:280: // since it's 32 bits instead of 7. Is 32 bits referring to the picture id? The picture id is 7 or 15 bits. https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state_unittest.cc:626: // Key Frame, continious nit: continuous
Rebase + comments
https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state.cc (right): https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:69: if (frame->CodecSpecific()->codecType != kVideoCodecVP9) On 2015/11/09 12:04:22, åsapersson wrote: > This is needed for non-flexible mode. Done. https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:279: // Could be made more robust using PictureId istead On 2015/11/09 12:04:23, åsapersson wrote: > nit: instead I removed the last part of the comments since pictureId can be 7 bits. https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state.cc:280: // since it's 32 bits instead of 7. On 2015/11/09 12:04:22, åsapersson wrote: > Is 32 bits referring to the picture id? The picture id is 7 or 15 bits. Acknowledged. https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/decoding_state_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/460002/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/decoding_state_unittest.cc:626: // Key Frame, continious On 2015/11/09 12:04:23, åsapersson wrote: > nit: continuous Done.
lgtm
Mostly a rubberstamp lgtm, reyling on åsas and eriks reviews here. I found a few nits though. Note that it would be nice to add unittests for the vp9_impl.cc fix(?), do you think that is possible? https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc:45: layers_->LayerFrameEncoded(kilobits * 125, layer_id); Write kilobits * 8 / 1000 and remove the comment. https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:410: if (codec_.mode != kScreensharing) I think we should unittest this, but I guess it might be hard given how the class is currently designed... https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:784: // then that buffer must be the same as one of the three references. reformat this comment https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:797: frames_encoded_++; ++frames_encoded_ https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right): https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:59: uint8_t start_layer = 0; // the first spatial layer to be encoded. The https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:60: uint8_t stop_layer = 0; // the last spatial layer to be encoded. The
The CQ bit was checked by philipel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/1328113004/#ps480001 (title: "Rebase + comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328113004/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328113004/480001
The CQ bit was unchecked by philipel@google.com
Comment fix
Will write unittests for vp9_impl.cc, currently lots of untested code. https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc (right): https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/screenshare_layers_unittest.cc:45: layers_->LayerFrameEncoded(kilobits * 125, layer_id); On 2015/11/09 17:01:27, stefan-webrtc (holmer) wrote: > Write kilobits * 8 / 1000 and remove the comment. Done. https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:410: if (codec_.mode != kScreensharing) On 2015/11/09 17:01:27, stefan-webrtc (holmer) wrote: > I think we should unittest this, but I guess it might be hard given how the > class is currently designed... Acknowledged. https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:784: // then that buffer must be the same as one of the three references. On 2015/11/09 17:01:27, stefan-webrtc (holmer) wrote: > reformat this comment Done. https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:797: frames_encoded_++; On 2015/11/09 17:01:27, stefan-webrtc (holmer) wrote: > ++frames_encoded_ Done. https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.h (right): https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:59: uint8_t start_layer = 0; // the first spatial layer to be encoded. On 2015/11/09 17:01:27, stefan-webrtc (holmer) wrote: > The Done. https://codereview.webrtc.org/1328113004/diff/480001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.h:60: uint8_t stop_layer = 0; // the last spatial layer to be encoded. On 2015/11/09 17:01:27, stefan-webrtc (holmer) wrote: > The Done.
The CQ bit was checked by philipel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from asapersson@webrtc.org, stefan@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/1328113004/#ps500001 (title: "Comment fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328113004/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328113004/500001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1625)
Unittest fix
The CQ bit was checked by philipel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from asapersson@webrtc.org, stefan@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/1328113004/#ps520001 (title: "Unittest fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328113004/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328113004/520001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1628)
philipel@google.com changed reviewers: + kjellander@webrtc.org
philipel@google.com changed reviewers: - kjellander@webrtc.org
LGTM
The CQ bit was checked by philipel@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1328113004/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1328113004/520001
Message was sent while issue was closed.
Committed patchset #28 (id:520001)
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/77ccfb4d16c148e61a316746bb5d9705e8b39f4a Cr-Commit-Position: refs/heads/master@{#10572}
Message was sent while issue was closed.
A revert of this CL (patchset #28 id:520001) has been created in https://codereview.webrtc.org/1438543002/ by terelius@webrtc.org. The reason for reverting is: Seems to break VideoSendStreamTest.ReconfigureBitratesSetsEncoderBitratesCorrectly on Linux Memcheck buildbot.. |
