Issue 2398963003: Move usage of QualityScaler to ViEEncoder.

kthelgason

https://codereview.webrtc.org/2398963003/diff/1/webrtc/modules/video_coding/utility/quality_scaler.cc File webrtc/modules/video_coding/utility/quality_scaler.cc (left): https://codereview.webrtc.org/2398963003/diff/1/webrtc/modules/video_coding/utility/quality_scaler.cc#oldcode31 webrtc/modules/video_coding/utility/quality_scaler.cc:31: static const int kMeasureSecondsDownscale = 5; Note that there ...

4 years, 2 months ago (2016-10-06 14:34:13 UTC) #1

kthelgason

Description was changed from ========== Move usage of QualityScaler to ViEEncoder. This brings QualityScaler much ...

4 years, 2 months ago (2016-10-06 14:34:53 UTC) #2

kthelgason

kthelgason@webrtc.org changed reviewers: + magjed@webrtc.org, perkj@webrtc.org

4 years, 2 months ago (2016-10-06 14:34:54 UTC) #3

kthelgason

https://codereview.webrtc.org/2398963003/diff/80001/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h (left): https://codereview.webrtc.org/2398963003/diff/80001/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h#oldcode91 webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h:91: bool enable_scaling_; TODO: remove

4 years, 2 months ago (2016-10-06 18:05:47 UTC) #4

perkj_webrtc

https://codereview.webrtc.org/2398963003/diff/80001/webrtc/api/android/jni/androidmediaencoder_jni.cc File webrtc/api/android/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2398963003/diff/80001/webrtc/api/android/jni/androidmediaencoder_jni.cc#oldcode990 webrtc/api/android/jni/androidmediaencoder_jni.cc:990: image->adapt_reason_.quality_resolution_downscales = does this cl take stats into account? ...

4 years, 2 months ago (2016-10-07 07:41:16 UTC) #5

kthelgason

https://codereview.webrtc.org/2398963003/diff/80001/webrtc/api/android/jni/androidmediaencoder_jni.cc File webrtc/api/android/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2398963003/diff/80001/webrtc/api/android/jni/androidmediaencoder_jni.cc#oldcode990 webrtc/api/android/jni/androidmediaencoder_jni.cc:990: image->adapt_reason_.quality_resolution_downscales = On 2016/10/07 07:41:15, perkj_webrtc wrote: > does ...

4 years, 2 months ago (2016-10-07 09:34:48 UTC) #6

magjed_webrtc

First pass. It looks like this will be a nice cleanup! https://codereview.webrtc.org/2398963003/diff/120001/webrtc/api/android/jni/androidmediaencoder_jni.cc File webrtc/api/android/jni/androidmediaencoder_jni.cc (left): ...

4 years, 2 months ago (2016-10-07 12:02:54 UTC) #8

kthelgason

https://codereview.webrtc.org/2398963003/diff/120001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (left): https://codereview.webrtc.org/2398963003/diff/120001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc#oldcode375 webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:375: int qp = -1; On 2016/10/07 12:02:54, magjed_webrtc wrote: ...

4 years, 2 months ago (2016-10-07 13:54:48 UTC) #9

kthelgason

Description was changed from ========== Move usage of QualityScaler to ViEEncoder. This brings QualityScaler much ...

4 years, 2 months ago (2016-10-18 14:17:49 UTC) #10

kthelgason

I think this is ready for review. I updated the description with some points that ...

4 years, 2 months ago (2016-10-18 14:18:41 UTC) #11

magjed_webrtc

https://codereview.webrtc.org/2398963003/diff/200001/webrtc/api/android/jni/androidmediaencoder_jni.cc File webrtc/api/android/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2398963003/diff/200001/webrtc/api/android/jni/androidmediaencoder_jni.cc#oldcode389 webrtc/api/android/jni/androidmediaencoder_jni.cc:389: scale_ = codec_settings->codecSpecific.VP8.automaticResizeOn; I don't see that you have ...

4 years, 2 months ago (2016-10-19 13:00:12 UTC) #12

kthelgason

https://codereview.webrtc.org/2398963003/diff/200001/webrtc/api/android/jni/androidmediaencoder_jni.cc File webrtc/api/android/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2398963003/diff/200001/webrtc/api/android/jni/androidmediaencoder_jni.cc#oldcode389 webrtc/api/android/jni/androidmediaencoder_jni.cc:389: scale_ = codec_settings->codecSpecific.VP8.automaticResizeOn; On 2016/10/19 13:00:12, magjed_webrtc wrote: > ...

4 years, 2 months ago (2016-10-19 13:07:27 UTC) #13

magjed_webrtc

https://codereview.webrtc.org/2398963003/diff/200001/webrtc/api/android/jni/androidmediaencoder_jni.cc File webrtc/api/android/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2398963003/diff/200001/webrtc/api/android/jni/androidmediaencoder_jni.cc#oldcode389 webrtc/api/android/jni/androidmediaencoder_jni.cc:389: scale_ = codec_settings->codecSpecific.VP8.automaticResizeOn; On 2016/10/19 13:07:27, kthelgason wrote: > ...

4 years, 2 months ago (2016-10-19 13:55:07 UTC) #14

https://codereview.webrtc.org/2398963003/diff/200001/webrtc/api/android/jni/a...
File webrtc/api/android/jni/androidmediaencoder_jni.cc (left):

https://codereview.webrtc.org/2398963003/diff/200001/webrtc/api/android/jni/a...
webrtc/api/android/jni/androidmediaencoder_jni.cc:389: scale_ =
codec_settings->codecSpecific.VP8.automaticResizeOn;
On 2016/10/19 13:07:27, kthelgason wrote:
> On 2016/10/19 13:00:12, magjed_webrtc wrote:
> > I don't see that you have preserved this logic. If you can't easily move it
to
> > e.g. vie_encoder.cc then keep the logic in this CL. It's easier for everyone
> the
> > more you split up a big refactoring into separate self contained changes.
> 
> You're right. This is one of the items mentioned as to TODO in the
description.
> I think for now I will just keep this evaluation in the encoder and modify the
> value returned in GetQPThresholds.

Sorry, I didn't see that comment in the description.

https://codereview.webrtc.org/2398963003/diff/200001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2398963003/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:177: bool disable_quality_scaling_
ACCESS_ON(&encoder_queue_);
On 2016/10/19 13:07:27, kthelgason wrote:
> On 2016/10/19 13:00:12, magjed_webrtc wrote:
> > I still want the quality scaler to be in an unique_ptr instead and change
> > QualityScaler::Init to a ctor in order to reduce the amount of mutable
state.
> 
> Allright. My reasoning here is just keeping the implementation as similar to
the
> overuse detector as possible. Perhaps this could be done in a follow-up CL
where
> we change both of them?

I wouldn't ask you to do this if you hadn't already rewritten
QualityScaler::Init() and QualityScaler::Stop(). But since you have, I think we
should take the opportunity and make QualityScaler RAII instead. I don't see any
reason for mimicking the way overuse detector splits up lifetime management in
two variables. It will still be conceptually similar to overuse detector, just
cleaner. Negating the boolean variable name is also a bad practice.

kthelgason

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

4 years, 1 month ago (2016-10-24 10:41:08 UTC) #15

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/260001

4 years, 1 month ago (2016-10-24 10:41:23 UTC) #16

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-24 10:42:29 UTC) #17

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11783) ios64_gyp_dbg on ...

4 years, 1 month ago (2016-10-24 10:42:30 UTC) #18

kthelgason

Updated. PTAL. FYI the bots are unhappy because the patch fails to apply, not because ...

4 years, 1 month ago (2016-10-24 10:48:57 UTC) #19

kthelgason

Description was changed from ========== Move usage of QualityScaler to ViEEncoder. This brings QualityScaler much ...

4 years, 1 month ago (2016-10-25 12:16:10 UTC) #20

Description was changed from

==========
Move usage of QualityScaler to ViEEncoder.

This brings QualityScaler much more in line with OveruseFrameDetector.
The two classes are conceptually similar, and should be used in the
same way. The biggest changes in this CL are:
- Quality scaling is now only done in ViEEncoder and not in each
  encoder implementation separately.
- QualityScaler now checks the average QP asynchronously, instead of
  having to be polled on each frame.
- QualityScaler is no longer responsible for actually scaling the frames,
  but has a callback to ViEEncoder that it uses to express it's desire
  for lower resolution.

This is still not completely done, the questions that remain are:
- Where to report dropped frames to the quality scaler
- Some encoders want to have scaling disabled in certain cases
- what to do about stats? Do we want to include a scaling reason in the callback
- What to do in the case of simulcast? Right now I turned scaling off unless
there is just one stream

BUG=webrtc:6495
==========

to

==========
Move usage of QualityScaler to ViEEncoder.

This brings QualityScaler much more in line with OveruseFrameDetector.
The two classes are conceptually similar, and should be used in the
same way. The biggest changes in this CL are:
- Quality scaling is now only done in ViEEncoder and not in each
  encoder implementation separately.
- QualityScaler now checks the average QP asynchronously, instead of
  having to be polled on each frame.
- QualityScaler is no longer responsible for actually scaling the frames,
  but has a callback to ViEEncoder that it uses to express it's desire
  for lower resolution.

This is still not completely done, the questions that remain are:
- Where to report dropped frames to the quality scaler
- ~Some encoders want to have scaling disabled in certain cases~
  + DONE
- ~what to do about stats? Do we want to include a scaling reason in the
callback~
  + DONE
- What to do in the case of simulcast? Right now I turned scaling off unless
there is just one stream

BUG=webrtc:6495
==========

magjed_webrtc

https://codereview.webrtc.org/2398963003/diff/280001/webrtc/api/android/jni/androidmediaencoder_jni.cc File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2398963003/diff/280001/webrtc/api/android/jni/androidmediaencoder_jni.cc#newcode34 webrtc/api/android/jni/androidmediaencoder_jni.cc:34: #include "webrtc/modules/video_coding/utility/vp8_header_parser.h" nit: revert this change. It was right ...

4 years, 1 month ago (2016-10-26 14:28:37 UTC) #21

perkj_webrtc

https://codereview.webrtc.org/2398963003/diff/280001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2398963003/diff/280001/webrtc/video/vie_encoder.cc#newcode701 webrtc/video/vie_encoder.cc:701: encoder_queue_.PostTask([this, timestamp, time_sent, encoded_image] { This may contain invalid ...

4 years, 1 month ago (2016-10-26 15:53:51 UTC) #22

perkj_webrtc

perkj@webrtc.org changed reviewers: + sprang@google.com

4 years, 1 month ago (2016-10-26 16:45:16 UTC) #23

kthelgason

https://codereview.webrtc.org/2398963003/diff/280001/webrtc/api/android/jni/androidmediaencoder_jni.cc File webrtc/api/android/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2398963003/diff/280001/webrtc/api/android/jni/androidmediaencoder_jni.cc#newcode34 webrtc/api/android/jni/androidmediaencoder_jni.cc:34: #include "webrtc/modules/video_coding/utility/vp8_header_parser.h" On 2016/10/26 14:28:36, magjed_webrtc wrote: > nit: ...

4 years, 1 month ago (2016-10-26 19:02:50 UTC) #25

magjed_webrtc

https://codereview.webrtc.org/2398963003/diff/280001/webrtc/video/overuse_frame_detector.h File webrtc/video/overuse_frame_detector.h (right): https://codereview.webrtc.org/2398963003/diff/280001/webrtc/video/overuse_frame_detector.h#newcode149 webrtc/video/overuse_frame_detector.h:149: static const auto scale_reason_ = ScalingObserverInterface::kCpu; On 2016/10/26 19:02:50, ...

4 years, 1 month ago (2016-10-27 11:45:58 UTC) #26

https://codereview.webrtc.org/2398963003/diff/280001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.h (right):

https://codereview.webrtc.org/2398963003/diff/280001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.h:149: static const auto scale_reason_ =
ScalingObserverInterface::kCpu;
On 2016/10/26 19:02:50, kthelgason wrote:
> On 2016/10/26 14:28:37, magjed_webrtc wrote:
> > Why do you need this as a const class static constant? Why not use
> > ScalingObserverInterface::kCpu directly?
> 
> We don't need to, I just prefer it over using the constant all over the place.
> Seemed easier to change by doing it like this.

Ok. I would go with ScalingObserverInterface::kCpu directly, but if you want to
use a constant for this, then you should name it kScaleReason, make it a
non-class variable, and move the declaration to the cc file in an anonymous
namespace.

https://codereview.webrtc.org/2398963003/diff/280001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2398963003/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:176: bool enable_quality_scaling_
ACCESS_ON(&encoder_queue_);
On 2016/10/26 19:02:50, kthelgason wrote:
> On 2016/10/26 14:28:37, magjed_webrtc wrote:
> > I think you misunderstood me about the RAII change.
> 
> So the issue with doing what you describe comes down to task queue usage. In
> order for this to work without needing to guard access to members with a lock
> all methods must be called sequentially. If the dtor is responsible for
> deinitialisation we can't make that guarantee.

I don't follow, for me it looks perfectly fine to remove QualityScaler::Stop()
and move that code to QualityScaler::~QualityScaler() instead. All methods will
still be called sequentially on the task queue, including the dtor. Can you
elaborate why this would be a problem?

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.cc (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:48: const auto q =
rtc::TaskQueue::Current();
nit: I would not use auto here and the style guide says: "Use auto to avoid type
names that are noisy, obvious, or unimportant - cases where the type doesn't aid
in clarity for the reader." and at least for me the return type of
rtc::TaskQueue::Current() is not obvious or unimportant, and the type
rtc::TaskQueue* is not noisy. For example, I was concerned if you were making an
unintentional copy-by-value of something expensive since you are not capturing
by-ref, so I had to look the specific type up in task_queue.h. Let's keep the
use of auto to the specific cases where it's allowed or encouraged according to
the style guide. For this case, you can also just inline q, i.e. write
rtc::TaskQueue::Current()->PostDelayedTask(...).

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:54: friend class
QualityScaler;
Why is QualityScaler a friend? Can't you just make Stop() public instead?

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:95: int low, high;
nit: You should only have one declaration per line.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.h (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:17: #include
"webrtc/video_frame.h"
Remove this unused include as well as the i420_buffer_pool.h include.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:71: virtual int64_t
GetTimeoutMs();
nit: I don't think the name 'Timeout' is appropriate. Can you rename it
something like GetSamplingPeriodMs()?

As a sidenote, I think it would have been better to be able to pass in an
explicit TaskQueue to QualityScaler as an argument, so that we can pass in a
fake/mock TaskQueue when testing, so that the tests become completely
deterministic and we don't have to make virtual functions like this. That would
require TaskQueue to be an interface, which is not the case, so I think this is
an ok approach for now.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:85: int64_t
measure_interval_ GUARDED_BY(&task_checker_);
nit: I would like to add a 'sec' suffix to the variable name to document the
time unit.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:17: #include
"webrtc/common_video/rotation.h"
This include looks unused.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:18: #include
"webrtc/video_frame.h"
This include looks unused as well.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:105: do {        
                  \
Why a do-while block and not just a block?
{
  rtc::Event ev(false, false);
  ev.Wait(x);
}

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:111:
EXPECT_CALL(*observer_, ScaleDown(reason_)).Times(1);
How do we know that it will be called exactly once? Is that determined by
'kFramerate * 5' in TriggerScale? Then maybe this test is overspecified and we
should use Times(AtLeast(1)) instead.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:113:
WAIT_FOR_MS(50);
Will this sleep for 50ms unconditionally? Can't we use gmock to wait for events
somehow? If it's not possible to wait for events with gmock, then I think it's
better to mock manually with events that we wait for.

sprang_webrtc

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

4 years, 1 month ago (2016-10-27 13:43:47 UTC) #27

sprang_webrtc

mostly some style nits https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm (right): https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm#newcode254 webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:254: // input_frame size should always ...

4 years, 1 month ago (2016-10-27 13:43:48 UTC) #28

kthelgason

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm (right): https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm#newcode254 webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:254: // input_frame size should always match codec settings On ...

4 years, 1 month ago (2016-10-28 13:20:16 UTC) #29

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm
(right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:254: //
input_frame size should always match codec settings
On 2016/10/27 13:43:48, språng wrote:
> nit: // Input_frame size should always match codec settings.

I feel weird capitalizing a variable name that is not capitalized in the code. I
added || around it to make it clearer that it's a variable.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:475: // Turn
off quality scaling for simulcast
On 2016/10/27 13:43:48, språng wrote:
> nit: End comments with a period.

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:479: return
QualityScaler::Settings(false);
On 2016/10/27 13:43:48, språng wrote:
> nit: use {} brackets with if+else even if single line statements.

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.cc (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:33: static const int
kMeasureSeconds = 5;
On 2016/10/27 13:43:48, språng wrote:
> I think this is the first time I've seen seconds used as a unit in out code.
I'm
> afraid of change, so would like this to be in ms too :)

That sounds good to me. This is in seconds because the previous code had it
specified in seconds, but I can of course change it.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:48: const auto q =
rtc::TaskQueue::Current();
On 2016/10/27 11:45:57, magjed_webrtc wrote:
> nit: I would not use auto here and the style guide says: "Use auto to avoid
type
> names that are noisy, obvious, or unimportant - cases where the type doesn't
aid
> in clarity for the reader." and at least for me the return type of
> rtc::TaskQueue::Current() is not obvious or unimportant, and the type
> rtc::TaskQueue* is not noisy. For example, I was concerned if you were making
an
> unintentional copy-by-value of something expensive since you are not capturing
> by-ref, so I had to look the specific type up in task_queue.h. Let's keep the
> use of auto to the specific cases where it's allowed or encouraged according
to
> the style guide. For this case, you can also just inline q, i.e. write
> rtc::TaskQueue::Current()->PostDelayedTask(...).

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:54: friend class
QualityScaler;
On 2016/10/27 11:45:57, magjed_webrtc wrote:
> Why is QualityScaler a friend? Can't you just make Stop() public instead?

I think it's cleaner to enforce the fact that this method should only ever be
called by QualityScaler this way. Maybe that's stupid since the class is not
declared in a header, but that is my reasoning at least :)

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:58: }
On 2016/10/27 13:43:48, språng wrote:
> nit: \n

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:95: int low, high;
On 2016/10/27 11:45:57, magjed_webrtc wrote:
> nit: You should only have one declaration per line.

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:156: const auto avg_qp =
average_qp_.GetAverage();
On 2016/10/27 13:43:48, språng wrote:
> Would prefer the actual type here.

I'm not sure I agree. The type is rtc::Optional<int>, and typing that out does
not add much clarity, at least for me. But my preference is always towards more
auto, not less. Feel free to reiterate this comment if you feel strongly about
it.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.h (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:17: #include
"webrtc/video_frame.h"
On 2016/10/27 11:45:57, magjed_webrtc wrote:
> Remove this unused include as well as the i420_buffer_pool.h include.

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:28: enum ScaleReason {
kQuality, kCpu };
On 2016/10/27 13:43:48, språng wrote:
> prefer enum class

I get that strongly-typed enums have several benefits, however I'm using this
enum as an array index and thus the implicit integer conversion is actually
desired behaviour.
I can change that of course, if you think it's unsafe.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:29: static const size_t
ScaleReasonSize = 2;
On 2016/10/27 13:43:48, språng wrote:
> nit: kScaleReasonSize or SCALE_REASON_SIZE

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:44: typedef std::pair<int,
int> QPThresholds;
On 2016/10/27 13:43:48, språng wrote:
> I'd prefer a simple struct, just because threshold.high is more explicit than
> threshold.second.

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:71: virtual int64_t
GetTimeoutMs();
On 2016/10/27 11:45:57, magjed_webrtc wrote:
> nit: I don't think the name 'Timeout' is appropriate. Can you rename it
> something like GetSamplingPeriodMs()?
> 
> As a sidenote, I think it would have been better to be able to pass in an
> explicit TaskQueue to QualityScaler as an argument, so that we can pass in a
> fake/mock TaskQueue when testing, so that the tests become completely
> deterministic and we don't have to make virtual functions like this. That
would
> require TaskQueue to be an interface, which is not the case, so I think this
is
> an ok approach for now.

I agree that is a better approach. I didn't see a way to make it work though.
Changed the name of the method in line with your suggestion.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:80:
ScalingObserverInterface* observer_ GUARDED_BY(&task_checker_);
On 2016/10/27 13:43:48, språng wrote:
> ScalingObserverInterface* const observer_

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:85: int64_t
measure_interval_ GUARDED_BY(&task_checker_);
On 2016/10/27 11:45:57, magjed_webrtc wrote:
> nit: I would like to add a 'sec' suffix to the variable name to document the
> time unit.

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:90: int high_qp_threshold_
GUARDED_BY(&task_checker_);
On 2016/10/27 13:43:48, språng wrote:
> Maybe a single QPThresholds member?

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:92: static const auto
scale_reason_ = ScalingObserverInterface::kQuality;
On 2016/10/27 13:43:48, språng wrote:
> Prefer not to use auto here.

Why? the type should be completely obvious from the rhs of the assignment.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:17: #include
"webrtc/common_video/rotation.h"
On 2016/10/27 11:45:58, magjed_webrtc wrote:
> This include looks unused.

Acknowledged.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:18: #include
"webrtc/video_frame.h"
On 2016/10/27 11:45:58, magjed_webrtc wrote:
> This include looks unused as well.

Acknowledged.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:66:
EXPECT_TRUE(event.Wait(1000));
On 2016/10/27 13:43:48, språng wrote:
> Perhaps a kDefaultTimeoutMs constant somewhere?

Done.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:105: do {        
                  \
On 2016/10/27 13:43:48, språng wrote:
> On 2016/10/27 11:45:58, magjed_webrtc wrote:
> > Why a do-while block and not just a block?
> > {
> >   rtc::Event ev(false, false);
> >   ev.Wait(x);
> > }
> 
> This is good practice. See for instance:
>
http://stackoverflow.com/questions/1067226/c-multi-line-macro-do-while0-vs-sc...
> 
> That said, macros aren't generally good practice in themselves and I'm not
sure
> it's really needed here?

Sure, I would not use this in production code, but for a test I think it's fine.
It saves a little typing and adds a little clarity IMO.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:111:
EXPECT_CALL(*observer_, ScaleDown(reason_)).Times(1);
On 2016/10/27 11:45:58, magjed_webrtc wrote:
> How do we know that it will be called exactly once? Is that determined by
> 'kFramerate * 5' in TriggerScale? Then maybe this test is overspecified and we
> should use Times(AtLeast(1)) instead.

After a scaledown event stats get flushed. If ScaleDown were called more than
once that indicates a bug.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:113:
WAIT_FOR_MS(50);
On 2016/10/27 11:45:58, magjed_webrtc wrote:
> Will this sleep for 50ms unconditionally? Can't we use gmock to wait for
events
> somehow? If it's not possible to wait for events with gmock, then I think it's
> better to mock manually with events that we wait for.

Yes, for now this is an unconditional wait. I don't have any experience with
gmock, but I did try to find a way to do this and came up with nothing. I will
just mock these manually to fix this.

magjed_webrtc

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/utility/quality_scaler.cc File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/utility/quality_scaler.cc#newcode54 webrtc/modules/video_coding/utility/quality_scaler.cc:54: friend class QualityScaler; On 2016/10/28 13:20:15, kthelgason wrote: > ...

4 years, 1 month ago (2016-10-28 15:56:00 UTC) #30

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.cc (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:54: friend class
QualityScaler;
On 2016/10/28 13:20:15, kthelgason wrote:
> On 2016/10/27 11:45:57, magjed_webrtc wrote:
> > Why is QualityScaler a friend? Can't you just make Stop() public instead?
> 
> I think it's cleaner to enforce the fact that this method should only ever be
> called by QualityScaler this way. Maybe that's stupid since the class is not
> declared in a header, but that is my reasoning at least :)

But the class is unusable without access to the Stop function and it's
impossible to kill it. I don't feel strongly about one way over the other so do
what you see fit.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:156: const auto avg_qp =
average_qp_.GetAverage();
On 2016/10/27 13:43:48, språng wrote:
> Would prefer the actual type here.

+1. rtc::Optional<int> adds clarity for me. It's not possible to know the type
from the local context and it's necessary to look up the declaration of
GetAverage. The type declarations add more clarity for me than comments. Please
only use auto in these cases according to the style guide:
* (Encouraged) For iterators and other long/cluttery type names, particularly
when the type is clear from context (calls to find, begin, or end for instance).
* (Allowed) When the type is clear from local context (in the same expression or
within a few lines). Initialization of a pointer or smart pointer with calls to
new commonly falls into this category, as does use of auto in a range-based loop
over a container whose type is spelled out nearby.
* (Allowed) When the type doesn't matter because it isn't being used for
anything other than equality comparison.
* (Encouraged) When iterating over a map with a range-based loop (because it is
often assumed that the correct type is pair<KeyType,ValueType> whereas it is
actually pair<const KeyType,ValueType>). This is particularly well paired with
local key and value aliases for .first and .second (often const-ref).

sprang_webrtc

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm (right): https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm#newcode254 webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:254: // input_frame size should always match codec settings On ...

4 years, 1 month ago (2016-10-31 10:24:32 UTC) #31

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm
(right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:254: //
input_frame size should always match codec settings
On 2016/10/28 13:20:14, kthelgason wrote:
> On 2016/10/27 13:43:48, språng wrote:
> > nit: // Input_frame size should always match codec settings.
> 
> I feel weird capitalizing a variable name that is not capitalized in the code.
I
> added || around it to make it clearer that it's a variable.

Right, I missed the '_' so didn't notice it was the variable name. :D 
|input_frame| sounds good to me.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.h (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:28: enum ScaleReason {
kQuality, kCpu };
On 2016/10/28 13:20:15, kthelgason wrote:
> On 2016/10/27 13:43:48, språng wrote:
> > prefer enum class
> 
> I get that strongly-typed enums have several benefits, however I'm using this
> enum as an array index and thus the implicit integer conversion is actually
> desired behaviour.
> I can change that of course, if you think it's unsafe.

I'd still go with an enum class, and be more explicit. Something like:
enum class ScaleReason : size_t { kQuality = 0, kCpu = 1 };
If you really want to use them as index then just static_cast<size_t>()

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:92: static const auto
scale_reason_ = ScalingObserverInterface::kQuality;
On 2016/10/28 13:20:15, kthelgason wrote:
> On 2016/10/27 13:43:48, språng wrote:
> > Prefer not to use auto here.
> 
> Why? the type should be completely obvious from the rhs of the assignment. 

If you change to an enum class, maybe. Otherwise some glancing at the code for
the first time will have to scroll up and read again to realize kQuality is a
part of ScaleReason.
I personally prefer using auto only for ridiculously long std container iterator
types and the like...

kthelgason

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

4 years, 1 month ago (2016-11-07 14:42:11 UTC) #32

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/340001

4 years, 1 month ago (2016-11-07 14:42:23 UTC) #33

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-07 14:43:32 UTC) #34

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12256) ios_arm64_dbg on ...

4 years, 1 month ago (2016-11-07 14:43:33 UTC) #35

kthelgason

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

4 years, 1 month ago (2016-11-07 15:02:05 UTC) #36

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/360001

4 years, 1 month ago (2016-11-07 15:02:12 UTC) #37

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-07 15:04:23 UTC) #38

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12240) ios_arm64_dbg on ...

4 years, 1 month ago (2016-11-07 15:04:24 UTC) #39

kthelgason

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

4 years, 1 month ago (2016-11-07 15:19:05 UTC) #40

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/380001

4 years, 1 month ago (2016-11-07 15:19:13 UTC) #41

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-07 15:21:22 UTC) #42

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg/builds/8574) ios32_sim_dbg on ...

4 years, 1 month ago (2016-11-07 15:21:23 UTC) #43

kthelgason

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

4 years, 1 month ago (2016-11-07 16:00:59 UTC) #45

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/440001

4 years, 1 month ago (2016-11-07 16:01:06 UTC) #46

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-07 16:03:27 UTC) #47

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/builds/12614) android_compile_mips_dbg on ...

4 years, 1 month ago (2016-11-07 16:03:28 UTC) #48

kthelgason

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

4 years, 1 month ago (2016-11-07 16:14:11 UTC) #49

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/460001

4 years, 1 month ago (2016-11-07 16:14:16 UTC) #50

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-07 16:16:06 UTC) #51

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12246) ios64_sim_dbg on ...

4 years, 1 month ago (2016-11-07 16:16:07 UTC) #52

kthelgason

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

4 years, 1 month ago (2016-11-07 16:18:23 UTC) #54

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/480001

4 years, 1 month ago (2016-11-07 16:18:27 UTC) #55

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-07 16:26:58 UTC) #56

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/19260) linux_ubsan_vptr on ...

4 years, 1 month ago (2016-11-07 16:26:59 UTC) #57

kthelgason

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

4 years, 1 month ago (2016-11-08 12:31:48 UTC) #58

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/500001

4 years, 1 month ago (2016-11-08 12:31:59 UTC) #59

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 12:46:09 UTC) #60

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 12:46:10 UTC) #61

kthelgason

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

4 years, 1 month ago (2016-11-08 13:28:16 UTC) #62

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/520001

4 years, 1 month ago (2016-11-08 13:28:27 UTC) #63

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 13:39:40 UTC) #64

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 13:39:41 UTC) #65

magjed_webrtc

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/utility/quality_scaler.cc File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/utility/quality_scaler.cc#newcode48 webrtc/modules/video_coding/utility/quality_scaler.cc:48: const auto q = rtc::TaskQueue::Current(); On 2016/10/28 13:20:15, kthelgason ...

4 years, 1 month ago (2016-11-10 13:07:00 UTC) #66

kthelgason

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/DEPS File webrtc/DEPS (right): https://codereview.webrtc.org/2398963003/diff/520001/webrtc/DEPS#newcode31 webrtc/DEPS:31: "+webrtc/modules/video_coding/utility", # TODO(kthelgason): move this On 2016/11/10 13:06:59, magjed_webrtc ...

4 years, 1 month ago (2016-11-10 15:38:28 UTC) #67

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/DEPS#newcode31
webrtc/DEPS:31: "+webrtc/modules/video_coding/utility", # TODO(kthelgason): move
this
On 2016/11/10 13:06:59, magjed_webrtc wrote:
> In what file is this used? Can't you move this to a DEPS file close to where
> it's used?
This is used in webrtc/video_encoder.h so unfortunately it can't really be any
closer.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.cc (right):

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:42: const auto
scale_reason_ = ScalingObserverInterface::ScaleReason::kQuality;
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> Not allowed to use auto for non-local variables

Done.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:103:
check_qp_task_->Stop();
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> I would like to avoid the 'check_qp_task_->Stop()' and reset here. It's better
> if you turn this around, i.e. call this ctor from the other ctor with
> QualityScaler(observer, thresholds, kMeasureMs) and make this ctor where you
> explicitly set all members.
> 
> Also, move |check_qp_task_| to the ctor initialization list and make the
pointer
> const, i.e. 'CheckQPTask*const'.

Thanks, good point. Done.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:125: check_qp_task_ =
nullptr;
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> no need to set it to nullptr in the dtor.

Done.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:130: return
sampling_period_ms_;
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> Can you make |sampling_period_ms_| const and return 'fast_rampup_ ?
> sampling_period_ms_ : (sampling_period_ms_ * kSamplePeriodScaleFactor)' here?

Done.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:150: const auto drop_rate
= framedrop_percent_.GetAverage();
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> Don't use auto here.

Done.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.h (right):

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:65: void
ReportDroppedFrame() const;
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> I don't think this function or ReportQP should be const.

Acknowledged.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:69: // This method declared
virtual to help with testing.
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> It's not virtual anymore and not used for testing, so make it private.

Done.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:91: mutable MovingAverage
average_qp_ GUARDED_BY(&task_checker_);
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> I don't think these should be mutable unless you have a really good reason.

I made these mutable and the report{QP, DroppedFrame} functions const because I
thought of the MovingAverage as an implementation detail, and the fact that
those functions don't actually change the _observable_ state of the class.

I've changed it back and dropped const from the methods as per your comments, as
I guess it's a matter of interpretation whether they really are const.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:48: // Override
GetSamplingPeriodMs to speed up the tests.
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> Update comment, you pass it in the ctor instead.

Done.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:106:
std::unique_ptr<rtc::TaskQueue> q;
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> Can't this be a 'rtc::TaskQueue q;' instead? I.e. do you need the unique_ptr
for
> something?

Sure, Just done for symmetry with other ivars.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:106:
std::unique_ptr<rtc::TaskQueue> q;
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> nit: I guess this should be called 'q_'.

Done.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:114:
EXPECT_EQ(observer_->scaled_down, 1);
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> It should be the expected value first when using EXPECT_EQ (don't ask me why),
> i.e. EXPECT_EQ(1, ...).

😫

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:800: void ViEEncoder::ScaleDown(ScaleReason reason)
{
On 2016/11/10 13:07:00, magjed_webrtc wrote:
> Wouldn't it be easier to send |current_pixel_count| as an argument? Wouldn't
> that make Quality and CPU scaling more independent and avoid races?

The QualityScaler and CpuOveruseDetector don't know (and probably shouldn't
know) the current_pixel_count. 
Also, They aren't independent, as we maintain different counters, so even though
we have scaled down for quality reasons we might still let the cpu adapter scale
down further.
The stats also have to be updated depending on the scale reason.

magjed_webrtc

lgtm Please update the CL description. It looks like you need to rebase again... https://codereview.webrtc.org/2398963003/diff/520001/webrtc/DEPS ...

4 years, 1 month ago (2016-11-10 19:29:07 UTC) #68

lgtm

Please update the CL description.
It looks like you need to rebase again...

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/DEPS#newcode31
webrtc/DEPS:31: "+webrtc/modules/video_coding/utility", # TODO(kthelgason): move
this
On 2016/11/10 15:38:27, kthelgason wrote:
> On 2016/11/10 13:06:59, magjed_webrtc wrote:
> > In what file is this used? Can't you move this to a DEPS file close to where
> > it's used?
> This is used in webrtc/video_encoder.h so unfortunately it can't really be any
> closer.

Then it might be better to put the QPThresholds in webrtc/video_encoder.h and
update the DEPS for quality scaler instead. But you have a TODO so I'm willing
to leave this.

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2398963003/diff/520001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:800: void ViEEncoder::ScaleDown(ScaleReason reason)
{
On 2016/11/10 15:38:28, kthelgason wrote:
> On 2016/11/10 13:07:00, magjed_webrtc wrote:
> > Wouldn't it be easier to send |current_pixel_count| as an argument? Wouldn't
> > that make Quality and CPU scaling more independent and avoid races?
> 
> The QualityScaler and CpuOveruseDetector don't know (and probably shouldn't
> know) the current_pixel_count. 
> Also, They aren't independent, as we maintain different counters, so even
though
> we have scaled down for quality reasons we might still let the cpu adapter
scale
> down further.
> The stats also have to be updated depending on the scale reason.

QualityScaler used to store the resolution in 'Resolution target_res_' and
OveruseDetector stores it in |int num_pixels_| so it should be possible for them
to know what pixel count they are on. My point is that those pixel counts might
be out of sync with the pixel count here (last_frame_width_/last_frame_height_)
since these are asynchronous events that take a few ms to propagate. What
happens if e.g. QP scaler decides to call ScaleDown one frame after CPU scaler
has called ScaleDown? It looks like we will scale down twice then, if the
|last_frame_| resolution was updated between. I don't think this is a problem in
practice, so I'm willing to leave this code as is.

kthelgason

Description was changed from ========== Move usage of QualityScaler to ViEEncoder. This brings QualityScaler much ...

4 years, 1 month ago (2016-11-11 07:29:19 UTC) #69

kthelgason

ping perkj & sprang. Care to take another look? I want to try and land ...

4 years, 1 month ago (2016-11-11 10:39:15 UTC) #70

perkj_webrtc

This looks like an improvement. I just looked at ViEEncoder and the StatsProxy. Note that ...

4 years, 1 month ago (2016-11-14 07:21:16 UTC) #71

sprang_webrtc

looks good, but please address the video to screenshare and back switched that perkj@ mentioned. ...

4 years, 1 month ago (2016-11-14 10:25:39 UTC) #72

looks good, but please address the video to screenshare and back switched that
perkj@ mentioned. Can you add a test case for that?
Also, if you expected any changes in how stats are reported, notify people
and/or include a not in the cl so that perf sheriffs don't blindly revert.

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.h (right):

https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.h:28: enum ScaleReason {
kQuality, kCpu };
On 2016/10/31 10:24:31, språng wrote:
> On 2016/10/28 13:20:15, kthelgason wrote:
> > On 2016/10/27 13:43:48, språng wrote:
> > > prefer enum class
> > 
> > I get that strongly-typed enums have several benefits, however I'm using
this
> > enum as an array index and thus the implicit integer conversion is actually
> > desired behaviour.
> > I can change that of course, if you think it's unsafe.
> 
> I'd still go with an enum class, and be more explicit. Something like:
> enum class ScaleReason : size_t { kQuality = 0, kCpu = 1 };
> If you really want to use them as index then just static_cast<size_t>()

No?

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:26: static const
size_t kDefaultTimeout = 1000;
nit: kDefaultTimeoutMs (I guess?)

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (left):

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:621:
encoded_image.adapt_reason_.quality_resolution_downscales = 1;
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> So GetStats previously reported bw_limited_resolution when quality scaling
> kicked in.  I guess that make sense since if bw had been enough the quality
> should be good enough. 
> So please make sure this behaviour is not changed or start a discussion with
> Åsa, Stefan and Pbos. 

+1, so you don't get this reverted because of a perceived stats regression.

kthelgason

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_coding/utility/quality_scaler_unittest.cc File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right): https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_coding/utility/quality_scaler_unittest.cc#newcode26 webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:26: static const size_t kDefaultTimeout = 1000; On 2016/11/14 10:25:38, ...

4 years, 1 month ago (2016-11-14 14:36:45 UTC) #73

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:26: static const
size_t kDefaultTimeout = 1000;
On 2016/11/14 10:25:38, språng wrote:
> nit: kDefaultTimeoutMs (I guess?)

Done.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:52: const auto scale_reason_ =
ScalingObserverInterface::ScaleReason::kCpu;
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> nit: / / This is not a member, just a const so name accordingly. 

done. Used to be a member, that's why it's named like that.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (left):

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:621:
encoded_image.adapt_reason_.quality_resolution_downscales = 1;
On 2016/11/14 10:25:39, språng wrote:
> On 2016/11/14 07:21:16, perkj_webrtc wrote:
> > So GetStats previously reported bw_limited_resolution when quality scaling
> > kicked in.  I guess that make sense since if bw had been enough the quality
> > should be good enough. 
> > So please make sure this behaviour is not changed or start a discussion with
> > Åsa, Stefan and Pbos. 
> 
> +1, so you don't get this reverted because of a perceived stats regression.

This behaviour has not changed, only how it's reported. This used to be done via
a hack, setting this property on the |encoded_image| that eventually finds it's
way to the stats proxy. This was necessary because scaling happened all the way
down in the encoders. Now we can just report this directly to the stats_proxy
without having to stick a note on the frame.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:550: if (scaling_settings.enabled &&
source_proxy_->IsResolutionScalingEnabled()) {
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> should you delete the quality scaler if this is not true? 
> 
> ie- switching from camera to screen share? 

Good catch, thanks. It would not have actually scaled, as we check that scaling
is enabled for the current source before scaling, but it's a waste of CPU
cycles.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:721: if (quality_scaler_)
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> quality_scaler is still set here if you switch from camera to screen share.

Acknowledged.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:824: ++scale_counter_[reason];
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> Please add a LOG(LS_INFO) with the info about why we go up/down in resolution
> and include the value of both reasons.

Done.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:833: // Request higher resolution if we are cpu
restricted and the the current
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> cpu ? 

Done.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:852:
source_proxy_->RequestHigherResolutionThan(current_pixel_count);
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> And a log here too please.

Done.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:206: // restricted by CPU usage. It is reset if
|source_| is changed.
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> This comment does not seem to be correct which is good.  ie,  camera - > cPU
> restricted -> switch to screen share -> not cpu restricted -> switch back to
the
> camera -> should still be cpu restricted.

Done.

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:29: static const int kMaxCpuDowngrades = 2;
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> Why not keep this in ViEEncoder then?  You can even make it protected if you
> want.

For some reason I did this because I had seen it done in another file. I
actually hate this, and I'm very glad you commented on it!

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:544: TEST_F(ViEEncoderTest,
SwitchingSourceKeepsAdaptation) {
On 2016/11/14 07:21:16, perkj_webrtc wrote:
> This looks like the same test as StatsTracksAdaptationStatsWhenSwitchingSource
> and  StatsTracksQualityAdaptationStatsWhenSwitchingSource except that you also
> check that
> EXPECT_FALSE(stats.cpu_limited_resolution) is always false ? Can you just add
> that check to StatsTracksQualityAdaptationStatsWhenSwitchingSource instead? 

Ah, that's a rebase artifact. The test got duplicated for some reason. Thanks!

kthelgason

Thanks for taking a look sprang@! On 2016/11/14 10:25:39, språng wrote: > https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_coding/utility/quality_scaler.h#newcode28 > webrtc/modules/video_coding/utility/quality_scaler.h:28: ...

4 years, 1 month ago (2016-11-14 14:39:51 UTC) #74

Thanks for taking a look sprang@!

On 2016/11/14 10:25:39, språng wrote:
>
https://codereview.webrtc.org/2398963003/diff/300001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/utility/quality_scaler.h:28: enum ScaleReason {
> kQuality, kCpu };
> On 2016/10/31 10:24:31, språng wrote:
> > On 2016/10/28 13:20:15, kthelgason wrote:
> > > On 2016/10/27 13:43:48, språng wrote:
> > > > prefer enum class
> > > 
> > > I get that strongly-typed enums have several benefits, however I'm using
> this
> > > enum as an array index and thus the implicit integer conversion is
actually
> > > desired behaviour.
> > > I can change that of course, if you think it's unsafe.
> > 
> > I'd still go with an enum class, and be more explicit. Something like:
> > enum class ScaleReason : size_t { kQuality = 0, kCpu = 1 };
> > If you really want to use them as index then just static_cast<size_t>()
> 
> No?

Sorry, I feel pretty strongly against this. I did implement your suggestion but
I feel that it clutters up the code in several places quite a bit. I'm not sold
that the benefits in this case make up for it. I don't expect this to be used
anywhere else.

>
https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_cod...
> File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:26: static
const
> size_t kDefaultTimeout = 1000;
> nit: kDefaultTimeoutMs (I guess?)
> 
>
https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/send_statis...
> File webrtc/video/send_statistics_proxy_unittest.cc (left):
> 
>
https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video/send_statis...
> webrtc/video/send_statistics_proxy_unittest.cc:621:
> encoded_image.adapt_reason_.quality_resolution_downscales = 1;
> On 2016/11/14 07:21:16, perkj_webrtc wrote:
> > So GetStats previously reported bw_limited_resolution when quality scaling
> > kicked in.  I guess that make sense since if bw had been enough the quality
> > should be good enough. 
> > So please make sure this behaviour is not changed or start a discussion with
> > Åsa, Stefan and Pbos. 
> 
> +1, so you don't get this reverted because of a perceived stats regression.

Good point. Most of the behaviour will be kept the same, but we should sync
anyway, as there is probably something I've missed there.

kthelgason

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

4 years, 1 month ago (2016-11-14 14:41:49 UTC) #75

sprang_webrtc

On 2016/11/14 14:39:51, kthelgason wrote: > Thanks for taking a look sprang@! > > On ...

4 years, 1 month ago (2016-11-14 14:58:28 UTC) #77

perkj_webrtc

https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video_encoder.h File webrtc/video_encoder.h (right): https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video_encoder.h#newcode156 webrtc/video_encoder.h:156: virtual QualityScaler::Settings GetQPThresholds() const { Move QualityScaler::Settings to VideoEncoder::QualityThresholds ...

4 years, 1 month ago (2016-11-15 08:53:32 UTC) #78

kthelgason

On 2016/11/15 08:53:32, perkj_webrtc wrote: > https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video_encoder.h > File webrtc/video_encoder.h (right): > > https://codereview.webrtc.org/2398963003/diff/560001/webrtc/video_encoder.h#newcode156 > ...

4 years, 1 month ago (2016-11-16 15:20:47 UTC) #79

sprang_webrtc

lgtm https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/2398963003/diff/560001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc#newcode485 webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:485: } This is an artifact of a rebase, ...

4 years, 1 month ago (2016-11-16 16:25:50 UTC) #80

perkj_webrtc

https://codereview.webrtc.org/2398963003/diff/620001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2398963003/diff/620001/webrtc/video/vie_encoder.cc#newcode308 webrtc/video/vie_encoder.cc:308: // This can happen since |degradation_preference_| is set on ...

4 years, 1 month ago (2016-11-16 20:56:13 UTC) #81

kthelgason

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

4 years, 1 month ago (2016-11-17 10:39:19 UTC) #82

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/640001

4 years, 1 month ago (2016-11-17 10:39:27 UTC) #83

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-17 10:40:56 UTC) #84

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/builds/18100) android_compile_x86_rel on ...

4 years, 1 month ago (2016-11-17 10:40:58 UTC) #85

kthelgason

On 2016/11/16 20:56:13, perkj_webrtc wrote: > https://codereview.webrtc.org/2398963003/diff/620001/webrtc/video/vie_encoder.cc > File webrtc/video/vie_encoder.cc (right): > > https://codereview.webrtc.org/2398963003/diff/620001/webrtc/video/vie_encoder.cc#newcode308 > ...

4 years, 1 month ago (2016-11-17 10:42:38 UTC) #86

magjed_webrtc

magjed@webrtc.org changed reviewers: - sprang@google.com

4 years, 1 month ago (2016-11-17 13:49:54 UTC) #87

stefan-webrtc

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (right): https://codereview.webrtc.org/2398963003/diff/680001/webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc#newcode473 webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:473: } else { I'd rewrite this: if (NumberOfStreams(codec_) != ...

4 years, 1 month ago (2016-11-17 16:12:57 UTC) #88

perkj_webrtc

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2398963003/diff/680001/webrtc/video/vie_encoder.cc#newcode315 webrtc/video/vie_encoder.cc:315: scaling_enabled_ = Why not store degradation_preference ? https://codereview.webrtc.org/2398963003/diff/680001/webrtc/video/vie_encoder.cc#newcode576 webrtc/video/vie_encoder.cc:576: ...

4 years, 1 month ago (2016-11-17 21:23:39 UTC) #89

stefan-webrtc

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2398963003/diff/680001/webrtc/video/vie_encoder.cc#newcode663 webrtc/video/vie_encoder.cc:663: if (max_pixel_count_ && current_pixel_count >= *max_pixel_count_) { On 2016/11/17 ...

4 years, 1 month ago (2016-11-21 13:02:57 UTC) #90

kthelgason

Thanks for taking a look stefan, I replied to your comments and fixed most of ...

4 years, 1 month ago (2016-11-21 13:06:53 UTC) #91

perkj_webrtc

https://codereview.webrtc.org/2398963003/diff/700001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2398963003/diff/700001/webrtc/video/vie_encoder.cc#newcode411 webrtc/video/vie_encoder.cc:411: LOG(LS_INFO) << "Initializing quality scaler."; nit: I think you ...

4 years, 1 month ago (2016-11-22 19:04:16 UTC) #92

kthelgason

https://codereview.webrtc.org/2398963003/diff/700001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2398963003/diff/700001/webrtc/video/vie_encoder.cc#newcode411 webrtc/video/vie_encoder.cc:411: LOG(LS_INFO) << "Initializing quality scaler."; On 2016/11/22 19:04:16, perkj_webrtc ...

4 years ago (2016-11-23 12:05:54 UTC) #93

perkj_webrtc

VIE encoder lgtm https://codereview.webrtc.org/2398963003/diff/720001/webrtc/video/vie_encoder_unittest.cc File webrtc/video/vie_encoder_unittest.cc (right): https://codereview.webrtc.org/2398963003/diff/720001/webrtc/video/vie_encoder_unittest.cc#newcode691 webrtc/video/vie_encoder_unittest.cc:691: int frame_width = 1280; Can we ...

4 years ago (2016-11-24 08:46:51 UTC) #94

stefan-webrtc

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/modules/video_coding/utility/quality_scaler.cc File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/2398963003/diff/680001/webrtc/modules/video_coding/utility/quality_scaler.cc#newcode65 webrtc/modules/video_coding/utility/quality_scaler.cc:65: class QualityScaler::CheckQPTask : public rtc::QueuedTask { On 2016/11/21 13:06:52, ...

4 years ago (2016-11-24 09:36:55 UTC) #95

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/quality_scaler.cc (right):

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:65: class
QualityScaler::CheckQPTask : public rtc::QueuedTask {
On 2016/11/21 13:06:52, kthelgason wrote:
> On 2016/11/17 16:12:57, stefan-webrtc (holmer) wrote:
> > Could you add a comment somewhere why we decide to complicate the code by
> using
> > a task queue? This class is fairly simple and it's not obvious to me from
> > reading the code what the task queue actually adds, e.g., why are we not
> running
> > CheckQP on every reported QP?
> 
> Well, it seems logical that the QualityScaler and the CpuOveruseAdaptor should
> use the same interface for adjusting the source resolution. That interface is
> asynchronous and so this class needs to run on the right task queue to use it.

> 
> Seems easier to complicate the QualityScaler code and keep ViEEncoder the
same,
> than to introduce another path to set scaling preferences.
> 
> Anyway, this is mentioned in the CL description to some extent, perhaps not
> clearly.

I'm mostly asking if we should comment on why somewhere here to make it a bit
more clear?

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/quality_scaler.cc:149: ReportQPHigh();
On 2016/11/21 13:06:52, kthelgason wrote:
> On 2016/11/17 16:12:57, stefan-webrtc (holmer) wrote:
> > This is a bit weird I think, why do we report high qp if the frame drop rate
> is
> > high? Should probably rename the method.
> 
> You're right. This is actually unused now, as framedrop is never reported to
the
> QualityScaler. Perhaps it would be better to just remove the related code for
> now.

Does that mean this is a regression compared to when the QS was integrated with
the encoder wrappers?

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/video_encoder.h
File webrtc/video_encoder.h (right):

https://codereview.webrtc.org/2398963003/diff/680001/webrtc/video_encoder.h#n...
webrtc/video_encoder.h:185: virtual void OnDroppedFrame() {}
On 2016/11/21 13:06:53, kthelgason wrote:
> On 2016/11/17 16:12:57, stefan-webrtc (holmer) wrote:
> > TODO remove.
> 
> Should this be removed from the interface? If so, I don't think it should be
> done as part of this CL.

I thought it was only used for quality scaling? If it's not used for anything
else I don't see a reason to keep it.

kthelgason

On 2016/11/24 09:36:55, stefan-webrtc (holmer) wrote: > I'm mostly asking if we should comment on ...

4 years ago (2016-11-24 13:45:47 UTC) #96

stefan-webrtc

On 2016/11/24 13:45:47, kthelgason wrote: > On 2016/11/24 09:36:55, stefan-webrtc (holmer) wrote: > > I'm ...

4 years ago (2016-11-24 14:00:41 UTC) #97

kthelgason

On 2016/11/24 14:00:41, stefan-webrtc (holmer) wrote: > Seems like it's called when ever a frame ...

4 years ago (2016-11-25 08:25:16 UTC) #98

stefan-webrtc

Mostly looks good now, a test might be is missing though. https://codereview.webrtc.org/2398963003/diff/740001/webrtc/modules/video_coding/video_coding_impl.h File webrtc/modules/video_coding/video_coding_impl.h (right): ...

4 years ago (2016-11-25 10:45:25 UTC) #99

kthelgason

https://codereview.webrtc.org/2398963003/diff/740001/webrtc/modules/video_coding/video_coding_impl.h File webrtc/modules/video_coding/video_coding_impl.h (right): https://codereview.webrtc.org/2398963003/diff/740001/webrtc/modules/video_coding/video_coding_impl.h#newcode116 webrtc/modules/video_coding/video_coding_impl.h:116: EncodedImageCallback *post_encode_callback_; On 2016/11/25 at 10:45:25, stefan-webrtc (holmer) wrote: ...

4 years ago (2016-11-25 12:34:01 UTC) #100

stefan-webrtc

On 2016/11/25 12:34:01, kthelgason wrote: > https://codereview.webrtc.org/2398963003/diff/740001/webrtc/modules/video_coding/video_coding_impl.h > File webrtc/modules/video_coding/video_coding_impl.h (right): > > https://codereview.webrtc.org/2398963003/diff/740001/webrtc/modules/video_coding/video_coding_impl.h#newcode116 > ...

4 years ago (2016-11-28 15:07:23 UTC) #101

kthelgason

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

4 years ago (2016-11-29 09:22:39 UTC) #102

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/780001

4 years ago (2016-11-29 09:22:46 UTC) #103

commit-bot: I haz the power

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

4 years ago (2016-11-29 09:38:20 UTC) #104

kthelgason

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, sprang@webrtc.org, perkj@webrtc.org, stefan@webrtc.org ...

4 years ago (2016-11-29 09:42:06 UTC) #107

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2398963003/780001

4 years ago (2016-11-29 09:42:11 UTC) #108

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 780001, "attempt_start_ts": 1480412525596580, "parent_rev": "d03c8460a29b6c21c61fed75b19d82a83ca87ad4", "commit_rev": "01b33f67f50cffd5a44d57f12596a67908b7dd63"}

4 years ago (2016-11-29 09:44:13 UTC) #109

commit-bot: I haz the power

Description was changed from ========== Move usage of QualityScaler to ViEEncoder. This brings QualityScaler much ...

4 years ago (2016-11-29 09:44:19 UTC) #110

commit-bot: I haz the power

Description was changed from ========== Move usage of QualityScaler to ViEEncoder. This brings QualityScaler much ...

4 years ago (2016-11-29 09:44:31 UTC) #112

commit-bot: I haz the power

4 years ago (2016-11-29 09:44:33 UTC) #113

Message was sent while issue was closed.

Patchset 37 (id:??) landed as
https://crrev.com/876222f77dfc640c852d5bb14f0aacf0f74e5690
Cr-Commit-Position: refs/heads/master@{#15286}

Issue 2398963003: Move usage of QualityScaler to ViEEncoder. (Closed)

Description

Patch Set 1 #

Patch Set 2 : fix strange bug #

Patch Set 3 : delete log statement #

Patch Set 4 : re-add QP parsing to the MediaCodec implementation #

Patch Set 5 : fix videotoolbox overridden QP thresholds #

Patch Set 6 : Code review comments #

Patch Set 7 : code review #

Patch Set 8 : Make no scaling the default #

Patch Set 9 : Add back important stuff for videotoolbox encoder #

Patch Set 10 : implement getQPThresholds for various wrappers #

Patch Set 11 : Add back some logic to disable scaling in some cases #

Patch Set 12 : Refactor QualityScaler::Settings #

Patch Set 13 : tie qualityscaler lifetime to vieencoder #

Patch Set 14 : Stats now properly reflect scaling status #

Patch Set 15 : Code review #

Patch Set 16 : Code review round II #

Patch Set 17 : Rebase onto master #

Patch Set 18 : rebase more #

Patch Set 19 : remove nonexistant include #

Patch Set 20 : Fix rebase-introduced errors #

Patch Set 21 : rebase even more #

Patch Set 22 : fix build errors #

Patch Set 23 : Fix failing tests #

Patch Set 24 : Fix flaky test #

Patch Set 25 : Code review #

Patch Set 26 : rebase #

Patch Set 27 : Code review #

Patch Set 28 : code review and rebase #

Patch Set 29 : fix android build #

Patch Set 30 : Don't scale if source does not support it #

Patch Set 31 : rebase and fix issues #

Patch Set 32 : prevent data race #

Patch Set 33 : Code review #

Patch Set 34 : Actually test something sensible #

Patch Set 35 : Report dropped frames to ViEEncoder #

Patch Set 36 : Code review nit #

Patch Set 37 : rebase #

Messages