Remove VCMPacketizationCallback

Remove VideoCodingModule::VCMPacketizationCallback

And move encoder name cb to VCMSendStatisticsCallback.

BUG=webrtc:5687
Committed: https://crrev.com/376b192ea3137bf25f606d6d6e72e5794f3116d3
Cr-Commit-Position: refs/heads/master@{#12596}

[perkj_webrtc, 2016-04-20 10:08:17]

Patchset #1 (id:1) has been deleted

[perkj_webrtc, 2016-04-20 10:24:40]

perkj@webrtc.org changed reviewers:
+ pbos@webrtc.org, stefan@webrtc.org

[perkj_webrtc, 2016-04-20 10:24:41]

pbos- please first pass. 
stefan please.

[perkj_webrtc, 2016-04-20 11:46:21]

Description was changed from

==========
Remove VCMPacketizationCallback

BUG=
==========

to

==========
Remove VideoCodingModule::VCMPacketizationCallback

And move encoder name cb to VCMSendStatisticsCallback.

BUG=webrtc:5687
==========

[pbos-webrtc, 2016-04-20 13:48:26]

https://codereview.webrtc.org/1900193004/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/1900193004/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:61: rtc::CritScope
cs(&encoder_crit_);
I don't like that ::Process starts blocking on encoder_crit_, can we update
implementation name in VideoSender after encoding instead?

[perkj_webrtc, 2016-04-21 07:53:05]

ptal- avoid holding locks in cb and minimize string copy.

I look forward for a way of posting events on a thread instead of this hack ....

[perkj_webrtc, 2016-04-21 07:53:08]

ptal- avoid holding locks in cb and minimize string copy.

I look forward for a way of posting events on a thread instead of this hack ....

[perkj_webrtc, 2016-04-21 16:40:11]

Can you please take a new look?

[pbos-webrtc, 2016-04-22 15:51:10]

bot failures look real?

[stefan-webrtc, 2016-04-26 08:51:58]

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:82: std::string &&
encoder_name) = 0;
Not sure about the formatting for this. Have you run git cl format?

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:345: void
SendStatisticsProxy::OnOutgoingRate(uint32_t framerate,
Is there a better name for this method now that it also reports an encoder name?
OnEncoderStatsUpdate?

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:150:
statistics_proxy_->OnOutgoingRate(encode_fps, media_bitrate_bps, "");
Why not test the encoder name here too?

[perkj_webrtc, 2016-04-27 09:01:24]

ptal

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:82: std::string &&
encoder_name) = 0;
On 2016/04/26 08:51:58, stefan-webrtc (holmer) wrote:
> Not sure about the formatting for this. Have you run git cl format?

Well- lint does not recognize std::string&& encoder_name and git cl format does
not object.

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:345: void
SendStatisticsProxy::OnOutgoingRate(uint32_t framerate,
On 2016/04/26 08:51:58, stefan-webrtc (holmer) wrote:
> Is there a better name for this method now that it also reports an encoder
name?
> OnEncoderStatsUpdate?

Done.

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:150:
statistics_proxy_->OnOutgoingRate(encode_fps, media_bitrate_bps, "");
On 2016/04/26 08:51:58, stefan-webrtc (holmer) wrote:
> Why not test the encoder name here too?

Done.

[perkj_webrtc, 2016-04-27 09:02:51]

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/1900193004/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:82: std::string &&
encoder_name) = 0;
On 2016/04/27 09:01:24, perkj_webrtc wrote:
> On 2016/04/26 08:51:58, stefan-webrtc (holmer) wrote:
> > Not sure about the formatting for this. Have you run git cl format?
> 
> Well- lint does not recognize std::string&& encoder_name and git cl format
does
> not object.

This comment is wrong. 
git cl lint does not like std::string&&  and git cl format does not like
std::string &&.

The suggestion is to skip this lint check for now:
https://codereview.webrtc.org/1917223003/

[pbos-webrtc, 2016-05-02 00:16:06]

looks great, thanks, lgtm

Sorry for the slow review. :)

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:80: virtual int32_t
SendStatistics(const uint32_t bitRate,
remove consts for these please

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:82: std::string&&
encoder_name) = 0;
I prefer taking the hit of const std::string& encoder_name here, since it's a
weird to move the string here. WDYT?

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_sender.cc:51:
_encodedFrameCallback.SetMediaOpt(&_mediaOpt);
Can we make this a ctor param to _encodedFrameCallback?

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_sender.cc:60: // VideoSender::Process() will
be called in an infinit loop.
infinite

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:58: std::string&& encoder_name);
pref const std::string&, but at your discretion.

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:151: "a name");
"encoder name" or something descriptive

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:436: stats_proxy_->OnEncoderStatsUpdate(frame_rate,
bit_rate,
{}s if this stays multiline

[stefan-webrtc, 2016-05-02 10:36:11]

No other comments than pbos'. lgtm after those are fixed.

[perkj_webrtc, 2016-05-02 12:02:12]

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/include/video_coding_defines.h (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/include/video_coding_defines.h:80: virtual int32_t
SendStatistics(const uint32_t bitRate,
On 2016/05/02 00:16:06, pbos-webrtc wrote:
> remove consts for these please

Done.

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_sender.cc:51:
_encodedFrameCallback.SetMediaOpt(&_mediaOpt);
On 2016/05/02 00:16:06, pbos-webrtc wrote:
> Can we make this a ctor param to _encodedFrameCallback?

Done.

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_sender.cc:60: // VideoSender::Process() will
be called in an infinit loop.
On 2016/05/02 00:16:06, pbos-webrtc wrote:
> infinite

Done.

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:58: std::string&& encoder_name);
On 2016/05/02 00:16:06, pbos-webrtc wrote:
> pref const std::string&, but at your discretion.

Done.

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:151: "a name");
On 2016/05/02 00:16:06, pbos-webrtc wrote:
> "encoder name" or something descriptive

Done.

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1900193004/diff/180001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:436: stats_proxy_->OnEncoderStatsUpdate(frame_rate,
bit_rate,
On 2016/05/02 00:16:06, pbos-webrtc wrote:
> {}s if this stays multiline

Done.

[perkj_webrtc, 2016-05-02 12:02:29]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-05-02 12:02:29]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1900193004/#ps200001 (title:
"Addressed review comments.")

[commit-bot: I haz the power, 2016-05-02 12:02:43]

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

[commit-bot: I haz the power, 2016-05-02 12:23:10]

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

[commit-bot: I haz the power, 2016-05-02 12:23:12]

Try jobs failed on following builders:
  win_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/8910)

[perkj_webrtc, 2016-05-02 15:13:48]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-05-02 15:13:49]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1900193004/#ps220001 (title:
"Rebased and fixed SendStatisics const uint.")

[commit-bot: I haz the power, 2016-05-02 15:13:59]

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

[commit-bot: I haz the power, 2016-05-02 17:14:25]

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

[commit-bot: I haz the power, 2016-05-02 17:14:27]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pbos-webrtc, 2016-05-02 17:17:40]

The CQ bit was checked by pbos@webrtc.org

[commit-bot: I haz the power, 2016-05-02 17:17:54]

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

[commit-bot: I haz the power, 2016-05-02 18:35:28]

Description was changed from

==========
Remove VideoCodingModule::VCMPacketizationCallback

And move encoder name cb to VCMSendStatisticsCallback.

BUG=webrtc:5687
==========

to

==========
Remove VideoCodingModule::VCMPacketizationCallback

And move encoder name cb to VCMSendStatisticsCallback.

BUG=webrtc:5687
==========

[commit-bot: I haz the power, 2016-05-02 18:35:29]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-05-02 18:35:39]

Description was changed from

==========
Remove VideoCodingModule::VCMPacketizationCallback

And move encoder name cb to VCMSendStatisticsCallback.

BUG=webrtc:5687
==========

to

==========
Remove VideoCodingModule::VCMPacketizationCallback

And move encoder name cb to VCMSendStatisticsCallback.

BUG=webrtc:5687

Committed: https://crrev.com/376b192ea3137bf25f606d6d6e72e5794f3116d3
Cr-Commit-Position: refs/heads/master@{#12596}
==========

[commit-bot: I haz the power, 2016-05-02 18:35:41]

Patchset 11 (id:??) landed as
https://crrev.com/376b192ea3137bf25f606d6d6e72e5794f3116d3
Cr-Commit-Position: refs/heads/master@{#12596}