Movable support for VideoReceiveStream::Config and avoid copies

Movable support for VideoReceiveStream::Config and avoid copies.

Instead of the default copy constructor, the Copy() method has to be used.  In this CL, the number of copies has been reduced significantly in production code. One case in the video engine remains, where we need to restart a video stream.  Even in that case, I'm sure we could avoid it, but for this particular CL, I decided against it to keep things simple (and it's also an edge case).  Most importantly, creating copies is made harder and the interface encourages ownership transfers.

R=mflodman@webrtc.org, pbos@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/733b5478dd7a150cafa0e5796519ed89d43a78e6

[tommi, 2016-06-04 21:54:16]

git cl format

[tommi, 2016-06-04 22:01:17]

Description was changed from

==========
Movable support for VideoReceiveStream::Config and avoid copies

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

to

==========
Movable support for VideoReceiveStream::Config and avoid copies.

Instead of the default copy constructor, the Copy() method has to be used.  I've
trimmed down use of copies in production code down to one case in the video
engine where we need to restart the stream.

For that case, I'm sure we could even avoid it, but for this particular CL, I
decided against it to keep things simple.

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

[tommi, 2016-06-05 08:07:19]

Description was changed from

==========
Movable support for VideoReceiveStream::Config and avoid copies.

Instead of the default copy constructor, the Copy() method has to be used.  I've
trimmed down use of copies in production code down to one case in the video
engine where we need to restart the stream.

For that case, I'm sure we could even avoid it, but for this particular CL, I
decided against it to keep things simple.

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

to

==========
Movable support for VideoReceiveStream::Config and avoid copies.

Instead of the default copy constructor, the Copy() method has to be used.  In
this CL, the number of copies has been reduced significantly in production code.
One case in the video engine remains, where we need to restart a video stream. 
Even in that case, I'm sure we could avoid it, but for this particular CL, I
decided against it to keep things simple (and it's also an edge case).  Most
importantly, creating copies is made harder and the interface encourages
ownership transfers.
==========

[tommi, 2016-06-05 08:07:19]

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

[tommi, 2016-06-05 08:07:36]

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

[commit-bot: I haz the power, 2016-06-05 08:07:38]

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

[tommi, 2016-06-05 08:11:46]

Worth mentioning that this changes the public interface in a way that requires
callers of CreateVideoReceiveStream to either pass an Rvalue config object or
call the .Copy() method to get one.
The type itself remains unchanged.

[commit-bot: I haz the power, 2016-06-05 08:43:46]

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

[commit-bot: I haz the power, 2016-06-05 08:43:47]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2016-06-06 16:15:33]

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

[pbos-webrtc, 2016-06-06 16:15:34]

Sorry this might be a bit confusing since I'm very shaky on rvalue references,
but shouldn't the constructors take rvalue references instead of constructed
objects?

Also we could still use const references instead of pointers, right?

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/call.h#newcode117
webrtc/call.h:117: VideoReceiveStream::Config configuration) = 0;
Should this take a rvalue reference? I'm not sure how these things work, but
this looks like it requires a copy.

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2676:
EXPECT_EQ(kUlpfecCodec.id, config.rtp.fec.ulpfec_payload_type);
Replace config.rtp.. with GetConfig().rtp in this file since config is not
reused as a variable later.

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:40: ReceiveStatisticsProxy(const
VideoReceiveStream::Config* config,
Shouldn't this still be a const reference?

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.h (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.h:63: const VideoReceiveStream::Config* config,
const ref?

[tommi, 2016-06-08 09:49:40]

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/call.h#newcode117
webrtc/call.h:117: VideoReceiveStream::Config configuration) = 0;
On 2016/06/06 16:15:34, pbos-webrtc wrote:
> Should this take a rvalue reference? I'm not sure how these things work, but
> this looks like it requires a copy.

This accepts an rvalue or a copy.  I've modified the type so that creating a
copy will always need to be an explicit function call.  If the caller uses
std::move() or passes a value that's already rvalue (e.g. the return value from
a function), it will be passed as rvalue.

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:2676:
EXPECT_EQ(kUlpfecCodec.id, config.rtp.fec.ulpfec_payload_type);
On 2016/06/06 16:15:34, pbos-webrtc wrote:
> Replace config.rtp.. with GetConfig().rtp in this file since config is not
> reused as a variable later.

Done.

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:40: ReceiveStatisticsProxy(const
VideoReceiveStream::Config* config,
On 2016/06/06 16:15:34, pbos-webrtc wrote:
> Shouldn't this still be a const reference?

It could.  What I want to do is to make a distinction between these two cases:

* how const& passing is used in general, which is that it's assumed that the
const& may be a temporary, a variable on the stack or a variable who's lifetime
does not have guarantees.
* pass a pointer to an object that will be shared between the caller and callee
for a known period of time.

The way the second case is usually implemented is by passing a non-const
pointer.  However, in our case here, it's still appropriate to have the object
const.

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/rtp_stream_r...
File webrtc/video/rtp_stream_receiver.h (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/rtp_stream_r...
webrtc/video/rtp_stream_receiver.h:63: const VideoReceiveStream::Config* config,
On 2016/06/06 16:15:34, pbos-webrtc wrote:
> const ref?

(see other answer)

[tommi, 2016-06-08 09:49:42]

Address comments

[pbos-webrtc, 2016-06-08 11:24:05]

Can you make the const-reference->const ptr change elsewhere under webrtc/video
too? This could be done separately.

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:40: ReceiveStatisticsProxy(const
VideoReceiveStream::Config* config,
On 2016/06/08 09:49:39, tommi-webrtc wrote:
> On 2016/06/06 16:15:34, pbos-webrtc wrote:
> > Shouldn't this still be a const reference?
> 
> It could.  What I want to do is to make a distinction between these two cases:
> 
> * how const& passing is used in general, which is that it's assumed that the
> const& may be a temporary, a variable on the stack or a variable who's
lifetime
> does not have guarantees.
> * pass a pointer to an object that will be shared between the caller and
callee
> for a known period of time.
> 
> The way the second case is usually implemented is by passing a non-const
> pointer.  However, in our case here, it's still appropriate to have the object
> const.

I think we have quite a bit of video-related code that uses const references for
this, if we should start using pointers for these arguments I'd like for us to
do that as a cross-webrtc/video change instead of having this discrepancy. Does
that make sense?

E.g. in webrtc/video/ we usually handle the second case by having const
references, right now. I'm aboard with changing that, but we should do it across
the directory instead of having a style inconsistency.

[tommi, 2016-06-08 13:28:41]

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:40: ReceiveStatisticsProxy(const
VideoReceiveStream::Config* config,
On 2016/06/08 11:24:05, pbos-webrtc wrote:
> On 2016/06/08 09:49:39, tommi-webrtc wrote:
> > On 2016/06/06 16:15:34, pbos-webrtc wrote:
> > > Shouldn't this still be a const reference?
> > 
> > It could.  What I want to do is to make a distinction between these two
cases:
> > 
> > * how const& passing is used in general, which is that it's assumed that the
> > const& may be a temporary, a variable on the stack or a variable who's
> lifetime
> > does not have guarantees.
> > * pass a pointer to an object that will be shared between the caller and
> callee
> > for a known period of time.
> > 
> > The way the second case is usually implemented is by passing a non-const
> > pointer.  However, in our case here, it's still appropriate to have the
object
> > const.
> 
> I think we have quite a bit of video-related code that uses const references
for
> this, if we should start using pointers for these arguments I'd like for us to
> do that as a cross-webrtc/video change instead of having this discrepancy.
Does
> that make sense?
> 
> E.g. in webrtc/video/ we usually handle the second case by having const
> references, right now. I'm aboard with changing that, but we should do it
across
> the directory instead of having a style inconsistency.

I would prefer to keep this as is actually.  The semantics of how |config| and
|clock| are being passed, are comparable.

Furthermore, the other code is not following the style guide.  So I don't see a
reason for bulking up those sort of fixes when we can help with that work in
changelists like this one.

See: https://google.github.io/styleguide/cppguide.html#Reference_Arguments

[pbos-webrtc, 2016-06-08 14:39:51]

On 2016/06/08 13:28:41, tommi-webrtc wrote:
>
https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
> File webrtc/video/receive_statistics_proxy.h (right):
> 
>
https://codereview.webrtc.org/2042603002/diff/20001/webrtc/video/receive_stat...
> webrtc/video/receive_statistics_proxy.h:40: ReceiveStatisticsProxy(const
> VideoReceiveStream::Config* config,
> On 2016/06/08 11:24:05, pbos-webrtc wrote:
> > On 2016/06/08 09:49:39, tommi-webrtc wrote:
> > > On 2016/06/06 16:15:34, pbos-webrtc wrote:
> > > > Shouldn't this still be a const reference?
> > > 
> > > It could.  What I want to do is to make a distinction between these two
> cases:
> > > 
> > > * how const& passing is used in general, which is that it's assumed that
the
> > > const& may be a temporary, a variable on the stack or a variable who's
> > lifetime
> > > does not have guarantees.
> > > * pass a pointer to an object that will be shared between the caller and
> > callee
> > > for a known period of time.
> > > 
> > > The way the second case is usually implemented is by passing a non-const
> > > pointer.  However, in our case here, it's still appropriate to have the
> object
> > > const.
> > 
> > I think we have quite a bit of video-related code that uses const references
> for
> > this, if we should start using pointers for these arguments I'd like for us
to
> > do that as a cross-webrtc/video change instead of having this discrepancy.
> Does
> > that make sense?
> > 
> > E.g. in webrtc/video/ we usually handle the second case by having const
> > references, right now. I'm aboard with changing that, but we should do it
> across
> > the directory instead of having a style inconsistency.
> 
> I would prefer to keep this as is actually.  The semantics of how |config| and
> |clock| are being passed, are comparable.
> 
> Furthermore, the other code is not following the style guide.  So I don't see
a
> reason for bulking up those sort of fixes when we can help with that work in
> changelists like this one.
> 
> See: https://google.github.io/styleguide/cppguide.html#Reference_Arguments

gotcha, lgtm

[tommi, 2016-06-08 14:48:14]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2016-06-08 14:48:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042603002/40001

[tommi, 2016-06-08 14:50:40]

The CQ bit was unchecked by tommi@webrtc.org

[tommi, 2016-06-10 11:03:11]

On 2016/06/08 14:48:18, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/2042603002/40001

pinging magnus

[mflodman, 2016-06-10 14:40:55]

lgtm

https://codereview.webrtc.org/2042603002/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2042603002/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:132: if
(config_.rtp.fec.ulpfec_payload_type != -1) {
Currently is this the only place we sue 'config_' after constructing an instance
of ReciveStatisticsProxy, so we could store only the ulpfec pl type instead of
config if we'd like. We might need more parts of the config in the future, so I
think it's fine either way.

[tommi, 2016-06-10 15:42:37]

https://codereview.webrtc.org/2042603002/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2042603002/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:132: if
(config_.rtp.fec.ulpfec_payload_type != -1) {
On 2016/06/10 14:40:55, mflodman wrote:
> Currently is this the only place we sue 'config_' after constructing an
instance
> of ReciveStatisticsProxy, so we could store only the ulpfec pl type instead of
> config if we'd like. We might need more parts of the config in the future, so
I
> think it's fine either way.

That's a good point.  For now, I'll add a note in the header that points out
that we really don't need the full config pointer and what changes could be
made.

[tommi, 2016-06-10 15:42:54]

Add TODO

[tommi, 2016-06-10 15:43:03]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2016-06-10 15:43:03]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2042603002/#ps60001 (title:
"Add TODO")

[commit-bot: I haz the power, 2016-06-10 15:43:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042603002/60001

[commit-bot: I haz the power, 2016-06-10 15:50:16]

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

[commit-bot: I haz the power, 2016-06-10 15:50:17]

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

[commit-bot: I haz the power, 2016-06-10 15:58:19]

Description was changed from

==========
Movable support for VideoReceiveStream::Config and avoid copies.

Instead of the default copy constructor, the Copy() method has to be used.  In
this CL, the number of copies has been reduced significantly in production code.
One case in the video engine remains, where we need to restart a video stream. 
Even in that case, I'm sure we could avoid it, but for this particular CL, I
decided against it to keep things simple (and it's also an edge case).  Most
importantly, creating copies is made harder and the interface encourages
ownership transfers.
==========

to

==========
Movable support for VideoReceiveStream::Config and avoid copies.

Instead of the default copy constructor, the Copy() method has to be used.  In
this CL, the number of copies has been reduced significantly in production code.
One case in the video engine remains, where we need to restart a video stream. 
Even in that case, I'm sure we could avoid it, but for this particular CL, I
decided against it to keep things simple (and it's also an edge case).  Most
importantly, creating copies is made harder and the interface encourages
ownership transfers.

R=mflodman@webrtc.org, pbos@webrtc.org

Committed: https://crrev.com/733b5478dd7a150cafa0e5796519ed89d43a78e6
Cr-Commit-Position: refs/heads/master@{#13102}
==========

[tommi, 2016-06-10 15:58:20]

Description was changed from

==========
Movable support for VideoReceiveStream::Config and avoid copies.

Instead of the default copy constructor, the Copy() method has to be used.  In
this CL, the number of copies has been reduced significantly in production code.
One case in the video engine remains, where we need to restart a video stream. 
Even in that case, I'm sure we could avoid it, but for this particular CL, I
decided against it to keep things simple (and it's also an edge case).  Most
importantly, creating copies is made harder and the interface encourages
ownership transfers.

R=mflodman@webrtc.org, pbos@webrtc.org

Committed: https://crrev.com/733b5478dd7a150cafa0e5796519ed89d43a78e6
Cr-Commit-Position: refs/heads/master@{#13102}
==========

to

==========
Movable support for VideoReceiveStream::Config and avoid copies.

Instead of the default copy constructor, the Copy() method has to be used.  In
this CL, the number of copies has been reduced significantly in production code.
One case in the video engine remains, where we need to restart a video stream. 
Even in that case, I'm sure we could avoid it, but for this particular CL, I
decided against it to keep things simple (and it's also an edge case).  Most
importantly, creating copies is made harder and the interface encourages
ownership transfers.

R=mflodman@webrtc.org, pbos@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/733b5478dd7a150cafa0e5796...
==========

[commit-bot: I haz the power, 2016-06-10 15:58:21]

Patchset 4 (id:??) landed as
https://crrev.com/733b5478dd7a150cafa0e5796519ed89d43a78e6
Cr-Commit-Position: refs/heads/master@{#13102}

[tommi, 2016-06-10 15:58:22]

Committed patchset #4 (id:60001) manually as
733b5478dd7a150cafa0e5796519ed89d43a78e6 (presubmit successful).

[mflodman, 2016-06-12 05:55:19]

https://codereview.webrtc.org/2042603002/diff/40001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.cc (right):

https://codereview.webrtc.org/2042603002/diff/40001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.cc:132: if
(config_.rtp.fec.ulpfec_payload_type != -1) {
On 2016/06/10 15:42:37, tommi-webrtc wrote:
> On 2016/06/10 14:40:55, mflodman wrote:
> > Currently is this the only place we sue 'config_' after constructing an
> instance
> > of ReciveStatisticsProxy, so we could store only the ulpfec pl type instead
of
> > config if we'd like. We might need more parts of the config in the future,
so
> I
> > think it's fine either way.
> 
> That's a good point.  For now, I'll add a note in the header that points out
> that we really don't need the full config pointer and what changes could be
> made.

SGTM